Make Token module a required dependency

Mike, enable the contributed Token module to see the token browser at the top.

How could we make it easy for admins to discover this? Should we show a message at the top of the screen?

@juampynr: That's where I think the Overview page would be useful - it could list topics like this. Also, a message in hook_install() and hook_requirements().

@damien, how about the Tour module? Would it help to create a tour on what is available?

It looks like some tokens are enabled even without Token enabled (that's what I'm seeing). Is it possible to show a list of what core tokens are available? ideally, it would also direct users to the token module if they additional options

Yes Mike, core supports a good amount of tokens. Damien, the Drupal 7 version has Token module as a dependency. Should we do the same for Drupal 8? Core does not have the browser which leads to a poor experience.

Yeah, now that we have support for the browser, it might be worth making it a requirement.

Here is a patch that makes Token module as required and adjusts the browser and replacement logic accordingly.

Nice. Out of interest, in D8-world are there any caches that should be cleared for a change like this?

Adding token module to the tests so they pass.

Nice. Out of interest, in D8-world are there any caches that should be cleared for a change like this?

I don't think so. I tested the code with Token module uninstalled and it did not raise any errors. There were also no warnings saying that metatag was installed but its dependency wasn't. I think that it is a manual step for the developer to add Token module before upgrading. What do you think, Damien?

I was more thinking of the changes to the info file and the services file, how do we tell Drupal those need to be reloaded?

(I'm looking at this from the POV of: how would a site react if we ran "drush upatedb --cache-clear=0"?)

To be honest, I have no idea :-D

Tests are failing because the testbot can't find contrib's Token module. Could this be because there is no stable release yet?

I asked this at #drupal-contrib. Here is the feedback:

<cweagans> juampy: benjy was running into the same issue not long ago. I'm not sure how it works when you're adding a dependency in a patch, because project dependencies are built/cached in a separate operation.

I think it's that old problem that you can't update the list of required modules for a module's tests via a patch, the dependencies have to be added to the branch first.

@DamienMcKenna, would it be enough if the release description points out that you should download and install Token module? An alternative would be to add hook_requirements().

IIRC hook_requirements is triggered after the module's dependencies are reviewed. We can add a note to the release notes about it, but I want to just confirm what happens when someone updates from beta3 to having this patch and *doesn't* have Token installed.

I want to just confirm what happens when someone updates from beta3 to having this patch and *doesn't* have Token installed.

Warnings will be logged when Metatag tries to render the Token Browser, as the theme function does not exist in core. Apart from that, nothing else. I tested this myself.

Re-testing.

Now I remember, this fails because Token module is not part of 8.x-1.x yet.

@DamienMcKenna, what do you think of my comment at #27? I could not reproduce any bugs/errors by not clearing caches after adding the Token dependency in a site with beta-2.

I'm getting an error after applying the enclosed patch any time I try to run a DrupalConsole command:

Recoverable fatal error: Argument 1 passed to Drupal\metatag\MetatagToken::__construct() must be an instance of Drupal\Core\Utility\Token, instance of Drupal\Core\Extension\ModuleHandler given, called in core/lib/Drupal/Component/DependencyInjection/Container.php on line 269 and defined in Drupal\metatag\MetatagToken->__construct() (line 29 of modules/contrib/metatag/src/MetatagToken.php).

Thoughts?

Patch re-rolled for 8.x-1.x.

The reroll didn't cover everything, this patch is a more thorough reroll. And all tests pass:

$ simpletest8 --verbose Metatag

Drupal test run
---------------

Tests to be run:
  - Drupal\metatag\Tests\MetatagAdminTest
  - Drupal\metatag\Tests\MetatagConfigTranslationTest
  - Drupal\metatag\Tests\MetatagFieldTest

Test run started:
  Wednesday, March 9, 2016 - 22:23

Test summary
------------

Drupal\metatag\Tests\MetatagAdminTest                        165 passes                           46 messages
Drupal\metatag\Tests\MetatagConfigTranslationTest             84 passes                           22 messages
Drupal\metatag\Tests\MetatagFieldTest                        114 passes                           34 messages

Test run duration: 3 min 22 sec

I committed a single change to add Token as a requirement so that this patch will work.

Trying to fix the last failure.

Committed. There are some weird problems showing up because the testbots are running against 8.2.x whereas the code is written for 8.0.x.