Ensure token replacements have cacheability + attachments metadata and that it is bubbled in any case

AFAICT this must be critical. E.g. a node with Hello, [user:name] token needs to bubble the user cache context, but currently won't. This needs to be fixed.

why not

Posting broad concept for review before I start converting hook_token implementations.

Basic concept, new TokenReplacementResult which extend from CacheableMetadata and wraps the raw replacement text in a value object with cacheability data.

Thoughts?

#4: wow, excellent first patch! :)

1. +++ b/core/lib/Drupal/Core/Utility/TokenReplacementResult.php
   @@ -0,0 +1,116 @@
   + * need to pass cacheability information to the caller. E.g. using the
   + * [user:name] token in a node-body needs to add cacheability information for
   

   s/information/metadata/

2. +++ b/core/lib/Drupal/Core/Utility/TokenReplacementResult.php
   @@ -0,0 +1,116 @@
   + * the user to the node's cacheability data.
   

   s/data/metadata/

3. +++ b/core/lib/Drupal/Core/Utility/TokenReplacementResult.php
   @@ -0,0 +1,116 @@
   + *   any of those cache tags is invalidated, the replacement text should also be
   

   s/is/are/

4. +++ b/core/lib/Drupal/Core/Utility/TokenReplacementResult.php
   @@ -0,0 +1,116 @@
   + * - Declare cache context to vary by, e.g. 'language' to do language-specific
   ...
   + *   $result->setCacheContexts(['language']);
   

   s/language/languages:language_interface/
   s/language-specific/interface language-specific/

5. +++ b/core/lib/Drupal/Core/Utility/TokenReplacementResult.php
   @@ -0,0 +1,116 @@
   + *   tokening.
   

   Is that a word? :D

6. +++ b/core/lib/Drupal/Core/Utility/TokenReplacementResult.php
   @@ -0,0 +1,116 @@
   + * - Declare a maximum age for the replacement text.
   

   […] E.g. 300, if the text will become outdated after 5 minutes.

7. +++ b/core/lib/Drupal/Core/Utility/TokenReplacementResult.php
   @@ -0,0 +1,116 @@
   +class TokenReplacementResult extends CacheableMetadata {
   

   +1 for the similarity (even in docs) to \Drupal\filter\FilterProcessResult :)

8. +++ b/core/lib/Drupal/Core/Utility/TokenReplacementResult.php
   @@ -0,0 +1,116 @@
   +   * Gets the processed text.
   

   s/processed/replacement/

9. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -44,10 +46,13 @@
   + *   string, or if cacheability information must be provided, an instance of
   

   s/information/metadata/

10. +++ b/core/lib/Drupal/Core/Utility/token.api.php
    @@ -86,7 +93,10 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
    +          $result->merge(CacheableMetadata::createFromObject($node));
    +          $result->merge(CacheableMetadata::createFromObject($account));
    

    ::merge() returns a new instance; it does not update the object it is invoked upon.

    So, these need to be:

    $result = $result->merge(…);
    

#5:

Not sure we should have that.

Good point — but FilterProcessResult also has this; so whatever we decide here, we should also apply there.

Re-rolling with changes from #6

#8: an interdiff seems to be missing; and it looks like there's a whole bunch of unrelated changes in your patch :(

Ah &%£$%, git branch fail.

Try again to update with comments from #6

+++ b/core/lib/Drupal/Core/Utility/token.api.php
@@ -44,10 +46,13 @@
+ *   strings from the original text. Replacement values can either be a raw
+ *   string, or if cacheability information must be provided, an instance of

We should not allow a raw string; because that means certain token providers would not be forced to think about cacheability metadata.

This is also the reason we require filters to return FilterProcessResults.

We discussed this for quite some time in a call with @WimLeers, @dawehner, @alexpott, @catch, @effulgentsia and @GaborHoitsy, @Fabianx.

Trying to quickly summarize what we discussed.

Unlike the filter API which has a handful of classes and limited amount of things that need to be changed, hook_tokens() consists of *huge* switch statements and with the API in the above patches, every single one of them would have to change. That's a big API change and a very painful one, since there is a lot of repetitive code around what we have now.

We were basically discussing two conceptual things here:

1) How to officially/globally treat token replacement in terms of rendering and cacheability. We basically have three options:
a) Enforce that everything returns cache metadata
b) Go with a best effort approach of making core return cacheability metadata and documenting it as such and that everyone who uses it and is concerned about always returning correct data should consider using placeholders.
c) Always treat token replacements as uncacheable (max-age: 0)

The decision is b) best effort. The reason for this is that we have a few ideas to auto-detect many dependencies and API's to make it relatively easy to add additional ones. Core itself does not use it anywhere in rendered output and we believe that security issues are going to be rare (e.g. combined with the fact that we want to make user.permissions a required context).

As for the other options, we were concerned about the disruption of a) and we want to avoid forcing that token replacements are non-cacheable, when in many cases they will be. (e.g. node related tokens in a rendered node).

2) We also discussed how to design the API here and support token implementations. And we basically came up with 3 things:

a) We check each entry in $data for CacheableDependencyInterface and add the metadata automatically. So if you pass in ['node' => $node], we will automatically add the cache tag for it, and if you have a token like node:author:name that re-enters the replacement system, we'll also add the tag for the user. It will however not work when using node:author directly.

b) To simplify that, instead of forcing them to create a response object as implemented by the previous patches and return it, we pass along a $cacheablity_metadata as a new optional argument. So node:author can do $cacheable_metadata = $cacheable_metadata->merge(CacheableMetadata::createFromObject($account).

c) We're also considering an declarative way to define cacheablity metadata in hook_token_info(), which would basically look like this:

$site['name'] = array(
    'name' => t("Name"),
    'description' => t("The name of the site."),
    'cache' => ['tags' => ['config:system.site']]
  );

That will only work for static dependencies, but it is especially useful for config dependencies like this.

We think that those three things combined and making sure that core implements it properly to a) actually work and b) serve as a good example will result in contrib/custom code picking this up as well.

(e.g. combined with the fact that we want to make user.permissions a required context).

For that, see #2493033: Make 'user.permissions' a required cache context.

This is a starter with a) and b)

Looks like a great first patch!

+++ b/core/lib/Drupal/Core/Utility/token.api.php
@@ -87,6 +89,7 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
+          $cache_metadata->merge(\Drupal\Core\Cache\CacheableMetadata::createFromObject($account));

+++ b/core/modules/taxonomy/taxonomy.tokens.inc
@@ -135,6 +136,7 @@ function taxonomy_tokens($type, $tokens, array $data = array(), array $options =
+            $cache_metadata->merge(CacheableMetadata::createFromObject($parent));

Needs to be assigned to $cache_metadata, otherwise the merged data is lost. You got it right in the many more additional places where merging happens :)

This should be reivewed now...

Looking great!

1. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
   @@ -219,7 +219,7 @@ public static function createFromObject($object) {
   -      $meta->maxAge = $object->getCacheMaxAge();
   +      $meta->maxAge = (int) $object->getCacheMaxAge();
   

   Hrm not sure I like this; the casting shouldn't be necessary if it'd comply with the interface.

2. +++ b/core/tests/Drupal/Tests/Core/Utility/TokenTest.php
   @@ -63,6 +63,13 @@ class TokenTest extends UnitTestCase {
   +   * The context manager.
   ...
   +  protected $contextManager;
   

   The context manager is something different :) Let's write "cache context manager" instead.

There we go.

Just one more nit. This IMO now needs review from Berdir.

+++ b/core/tests/Drupal/Tests/Core/Utility/TokenTest.php
@@ -63,11 +63,11 @@ class TokenTest extends UnitTestCase {
+   * The cache context manager.
...
    * @var \Drupal\Core\Cache\Context\CacheContextsManager
...
+  protected $cacheContextManager;

The class says "contexts", plural, yet here it is singular.

1. +++ b/core/core.libraries.yml
   @@ -56,7 +56,6 @@ drupalSettings:
          baseUrl: null
   -      scriptPath: null
          pathPrefix: null
          currentPath: null
   

   huh?

2. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -49,7 +51,7 @@
     * @see hook_tokens_alter()
     */
   -function hook_tokens($type, $tokens, array $data = array(), array $options = array()) {
   +function hook_tokens($type, $tokens, array $data = array(), array $options = array(), \Drupal\Core\Cache\CacheableMetadata &$cache_metadata) {
      $token_service = \Drupal::token();
   

   missing docs for the new method, also for replace().

   I was wondering if we should ensure that the cachableMetadata object is not lost by storing it on the token service and re-using it on re-entrance in replace().

   I see you solved that by making it required. We can have API changes here I guess so that's OK as well.

3. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -87,6 +89,7 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
              $account = $node->getOwner() ? $node->getOwner() : User::load(0);
              $replacements[$original] = $sanitize ? SafeMarkup::checkPlain($account->label()) : $account->label();
   +          $cache_metadata->merge(\Drupal\Core\Cache\CacheableMetadata::createFromObject($account));
              break;
   

   missing assignemnt ?

4. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -120,10 +123,12 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
   + * @param \Drupal\Core\Cache\CacheableMetadata $cache_metadata
   + *   The cache metadata.
   

   I guess this needs a bit more documentation on how it is supposed to be used. possibly even a merge code snippet.

5. +++ b/core/modules/statistics/statistics.tokens.inc
   @@ -31,7 +31,7 @@ function statistics_token_info() {
     */
   -function statistics_tokens($type, $tokens, array $data = array(), array $options = array()) {
   +function statistics_tokens($type, $tokens, array $data = array(), array $options = array(), \Drupal\Core\Cache\CacheableMetadata &$cache_metadata) {
   

   full namespace here but not elsewhere.

This looks great to me. We have a bunch of kernel tests for token replacements, we could expand them a bit to also collect this and assert it a bit? to test the actual implementations. And I think it would be great to have something like a test controller that actually prints tokens out and cares about cacheability.. we could then also use that code as an example for the change record.

system_tokens() also has date that default to NOW if no date is given. In that (and only that) case, we need to disable caching. I think.

user_tokens() has a current-user token. If that is used, then we should add the user cache context.

Thank you berdir for the review!

And I think it would be great to have something like a test controller that actually prints tokens out and cares about cacheability.. we could then also use that code as an example for the change record.

That sounds like a great plan.

user_tokens() has a current-user token. If that is used, then we should add the user cache context.

And if it prints the current user's name then we need to also associate the User entity's cache tag.

And if it prints the current user's name then we need to also associate the User entity's cache tag.

That happens automatically again, current-user just re-enters it with 'user' tokens and provides the user entity for it. Which then triggers the generic code to get cacheability metadata from it. :)

While this looks great, there have been not 0 bugs in the existing patch.

Down to 5 failures, yay!

That happens automatically again, current-user just re-enters it with 'user' tokens and provides the user entity for it. Which then triggers the generic code to get cacheability metadata from it. :)

Right :) Great!

While this looks great, there have been not 0 bugs in the existing patch.

Love the sarcasm here.

+++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
@@ -34,6 +34,13 @@ class CacheableMetadata implements CacheableDependencyInterface {
+  public function __construct(array $cache_contexts = [], array $cache_tags = [], $max_age = CacheBackendInterface::CACHE_PERMANENT) {
+    $this->contexts = $cache_contexts;
+    $this->tags = $cache_tags;
+    $this->maxAge = $max_age;
+  }

Ugh, no. We used to have a constructor and removed it for good reasons.

Ugh, no. We used to have a constructor and removed it for good reasons.

Well yeah, I feared I need that for easier readable test coverage, but it turned out, this was not the case.

Less failures, less code

Back to green.

To be clear, this is in a reviewable state.

This looks awesome, just some nits and a request for some extra docs.

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -153,18 +156,22 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   * Text with tokens replaced.
   

   nit: wrong indent

2. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -41,6 +41,11 @@
   + * @param \Drupal\Core\Cache\CacheableMetadata $cacheablity_metadata
   
   @@ -49,7 +54,7 @@
   +function hook_tokens($type, $tokens, array $data, array $options, \Drupal\Core\Cache\CacheableMetadata &$cacheablity_metadata) {
   

   %s/ablity/ability

3. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -41,6 +41,11 @@
   + *   The cache metadata. By default Drupal pulls of the metadata from the passed
   

   of or off or from?

4. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -120,10 +126,13 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
   + *   cacheablity metadata, you need to take that here into account.
   

   nit: %s/here into account/into account here

5. +++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
   @@ -71,12 +73,45 @@ function testCommentTokenReplacement() {
   +    $metadata_tests['[comment:created]'] = $cache_metadata->addCacheTags(['rendered']);
   
   +++ b/core/modules/file/src/Tests/FileTokenReplaceTest.php
   @@ -57,12 +58,31 @@ function testFileTokenReplacement() {
   +    $metadata_tests['[file:created]'] = $cache_metadata->addCacheTags(['rendered']);
   

   Should this also include the cache tags from the medium date format? Or is that what 'rendered' is? In which case can we add a comment to that effect.

6. +++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
   @@ -71,12 +73,45 @@ function testCommentTokenReplacement() {
   +    debug($cache_metadata->addCacheTags(['comment:2'])->getCacheTags());
   

   leftover debug?

7. +++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
   @@ -71,12 +73,45 @@ function testCommentTokenReplacement() {
   +      $cacheablity_metadata = new CacheableMetadata();
   +      $output = $token_service->replace($input, array('comment' => $comment), array('langcode' => $language_interface->getId()), $cacheablity_metadata);
          $this->assertEqual($output, $expected, format_string('Sanitized comment token %token replaced.', array('%token' => $input)));
   +      $this->assertEqual($cacheablity_metadata, $metadata_tests[$input]);
   

   nice

8. +++ b/core/modules/node/src/Tests/NodeTokenReplaceTest.php
   @@ -76,12 +77,35 @@ function testNodeTokenReplacement() {
   +    $metadata_tests['[node:created:since]'] = $cache_metadata->setCacheMaxAge(0);
   

   should we add a test for the raw node:created to check the medium date format tags like the other entity types - or is that overkill?

9. +++ b/core/modules/system/src/Tests/System/TokenReplaceWebTest.php
   @@ -0,0 +1,40 @@
   +   * Tests a token replacement on an actula website.
   

   %s/actula/actual

10. +++ b/core/modules/system/src/Tests/System/TokenReplaceWebTest.php
    @@ -0,0 +1,40 @@
    +    $this->drupalGet('token-test/' . $node->id());
    +    $this->assertText("Tokens: {$node->id()} {$account->id()}");
    +    $this->assertCacheTags(['node:1', 'rendered', 'user:2']);
    
    +++ b/core/modules/system/tests/modules/token_test/src/Controller/TestController.php
    @@ -0,0 +1,67 @@
    +    $build['#markup'] = $this->token->replace('Tokens: [node:nid] [current-user:uid]', ['node' => $node], [], $cacheablity_metadata);
    ...
    +    $cacheablity_metadata->applyTo($build);
    

    sweet - this serves as a great example - can we add this to the docblock of Token::replace, in an @code block - so others know the best way to collect and apply the metadata? I'd expect api.drupal.org for Token::replace will be the place most will goto in the future, rather than the change notice.

11. +++ b/core/modules/system/tests/modules/token_test/token_test.info.yml
    @@ -0,0 +1,3 @@
    +name: Token test
    +type: module
    +core: 8.x
    

    nit: should depend on node and user

12. +++ b/core/modules/user/src/Tests/UserTokenReplaceTest.php
    @@ -8,6 +8,8 @@
    +use Drupal\Core\Datetime\Entity\DateFormat;
    

    doesn't appear to be used?

working on change record

Change notice draft: https://www.drupal.org/node/2528662

Fixed all from #34 except 5 & 8. Also updated the issue summary since some of the typos existed there also.

Sorry for this..

@larowlan++
@laurii++

These changes look great! Do you think we need something more?

Minor nits - other than that rtbc in my books

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -129,6 +129,16 @@
   +   * therefor be applied e.g to a render array.
   

   therfore » therefore

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -129,6 +129,16 @@
   +   *  // Token::replace takes $cacheability_metadata as an reference so that it
   

   an » a
   for » to

Good point. Sure, we can do that.

@berdir and I discussed that and though we should just do that inside a render context. Outside of that, there are good usecases
to not care about cacheablity, like tokens for a generated email.

The helper is presented by #2351015: URL generation does not bubble cache contexts

This time with a patch

Maybe that

It is amazing, if things actually work as they are supposed to do.

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -115,17 +126,30 @@ class Token {
   +   *   The renderer
   

   Nit: missing trailing period.

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -153,18 +177,23 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   * @param \Drupal\Core\Cache\CacheableMetadata $cacheability_metadata
   +   *   The cacheablity metadata.
   

   Missing the ampersand.
   Missing the |null.

   Also: I think this should mention that if none is given (i.e. NULL is passed), that the cacheability metadata for tokens will then be bubbled using the render system, if this token replacement is happening as part of the render system

3. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -153,18 +177,23 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   * @return string Text with tokens replaced.
       *   Text with tokens replaced.
   

   Oopsie :)

   Also: we add a \n before @return.

4. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -41,6 +41,11 @@
   + * @param \Drupal\Core\Cache\CacheableMetadata $cacheability_metadata
   

   Missing ampersand.

5. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -41,6 +41,11 @@
   + *   The cache metadata. By default Drupal pulls off the metadata from the
   

   s/cache/cacheability

6. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -41,6 +41,11 @@
   + *   take into account cacheable metadata from related objects, like the
   

   s/cacheable/cacheability/

7. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -41,6 +41,11 @@
   + *   system.site config object, for the site-name token.
   

   Let's put single quotes around system.site and site-name for clarity.

8. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -120,10 +126,13 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
   + * @param \Drupal\Core\Cache\CacheableMetadata $cacheability_metadata
   

   Missing ampersand.

9. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -120,10 +126,13 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
   + *   The cache metadata. In case you alter an existing token based upon
   

   s/cche/cacheability/

10. +++ b/core/modules/statistics/statistics.tokens.inc
    @@ -4,6 +4,7 @@
      */
    +use Drupal\Core\Cache\CacheableMetadata;
    

    Should have a \n in between.

11. +++ b/core/modules/system/tests/modules/token_test/src/Controller/TestController.php
    @@ -0,0 +1,67 @@
    +   * The token.
    ...
    +   *   The token.
    

    The token replacement system.

12. +++ b/core/modules/system/tests/modules/token_test/src/Controller/TestController.php
    @@ -0,0 +1,67 @@
    +  public function tokenReplace(NodeInterface $node) {
    +    $build = [
    +      '#markup' => '',
    +    ];
    +    $cacheablity_metadata = new CacheableMetadata();
    +    $build['#markup'] = $this->token->replace('Tokens: [node:nid] [current-user:uid]', ['node' => $node], [], $cacheablity_metadata);
    +
    +    $cacheablity_metadata->applyTo($build);
    +
    +    return $build;
    +  }
    

    This is a test for the correct/intended usage.

    But we also want to repeat this test where we don't collect cacheability metadata. And it should have the same end result, thanks to the render context detection/integration.

13. +++ b/core/tests/Drupal/Tests/Core/Utility/TokenTest.php
    @@ -60,6 +63,20 @@ class TokenTest extends UnitTestCase {
    +   * The cache context manager.
    

    Nit: cache contextS manager — plural.

14. +++ b/core/modules/node/src/Tests/NodeTokenReplaceTest.php
    index b3ab9c9..fd3fc01 100644
    --- a/core/modules/node/src/Tests/Views/FrontPageTest.php
    
    --- a/core/modules/node/src/Tests/Views/FrontPageTest.php
    +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php
    
    +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php
    +++ b/core/modules/node/src/Tests/Views/FrontPageTest.php
    @@ -260,12 +260,12 @@ protected function assertFrontPageViewCacheTags($do_assert_views_caches) {
    
    @@ -260,12 +260,12 @@ protected function assertFrontPageViewCacheTags($do_assert_views_caches) {
           $view,
           $empty_node_listing_cache_tags,
           $do_assert_views_caches,
    -      $empty_node_listing_cache_tags
    +      Cache::mergeTags($empty_node_listing_cache_tags, ['config:system.site'])
         );
         $this->assertPageCacheContextsAndTags(
           Url::fromRoute('view.frontpage.page_1'),
           $cache_contexts,
    -      Cache::mergeTags($empty_node_listing_cache_tags, ['rendered', 'config:user.role.anonymous'])
    +      Cache::mergeTags($empty_node_listing_cache_tags, ['rendered', 'config:user.role.anonymous', 'config:system.site'])
         );
    

    Yay! :) :) Finally!

But we also want to repeat this test where we don't collect cacheability metadata. And it should have the same end result, thanks to the render context detection/integration.

So yeah this was useful, as it uncovered a bug.

Just nits now. Assigning to Berdir for review.

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -168,7 +168,7 @@ protected function renderPlaceholder($placeholder, array $elements) {
      public function hasRenderContext() {
   -    return isset(static::$context) && !static::$context->isEmpty();
   +    return isset(static::$context);
      }
   

   Great, that's what I thought I remembered from #2351015: URL generation does not bubble cache contexts (which is the origin of this code) :)

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -177,9 +177,10 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   * @param \Drupal\Core\Cache\CacheableMetadata &$cacheability_metadata|null
   +   *   (optional) The cacheablity metadata.
   

   Great, but still missing the "also" from #49.2.

3. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -41,11 +41,11 @@
   + *   The cacheability metadata. By default Drupal pulls off the metadata from the
   

   Should be By default we gather cacheability metadata from the

   Also: 80 col violation.

This looks very close to me. Very nice test coverage, @dawehner++

1. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -41,6 +41,11 @@
   + *   The cacheability metadata. By default Drupal pulls off the metadata from the
   + *   passed $data objects, but for global tokens the module developer needs to
   

   I'd just say something like the implementation instead of the module developer.

2. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -120,10 +126,13 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
   + * @param \Drupal\Core\Cache\CacheableMetadata &$cacheability_metadata
   + *   The cacheability metadata. In case you alter an existing token based upon
   + *   cacheability metadata, you need to take that into account.
     *
   

   Probably also avoid "you" here but use it or the alter hook?

3. +++ b/core/modules/comment/comment.tokens.inc
   @@ -105,7 +107,7 @@ function comment_token_info() {
   +function comment_tokens($type, $tokens, array $data, array $options, CacheableMetadata &$cacheablity_metadata) {
   

   I like cacheablity :)

   As discussed, should be cacheable_metadata everywhere instead of cacheablity_metadata/cache_metadata.

4. +++ b/core/modules/system/system.tokens.inc
   @@ -137,6 +154,8 @@ function system_tokens($type, $tokens, array $data = array(), array $options = a
   +      // We depend on the current request, so the token aren't cacheable at all.
   +      $cacheablity_metadata->setCacheMaxAge(0);
        }
   

   current request *time*.

5. +++ b/core/modules/system/tests/modules/token_test/src/Controller/TestController.php
   @@ -0,0 +1,85 @@
   +  public function tokenReplaceWithoutPassedCacheabilityMetadata(NodeInterface $node) {
   +    $build = [
   +      '#markup' => '',
   +    ];
   +    $build['#markup'] = $this->token->replace('Tokens: [node:nid] [current-user:uid]', ['node' => $node], []);
   

   can we add a comment here somewhere that we explicitly don't apply the cacheability metadata?

   Why initialize #markup if we always set it?

6. +++ b/core/modules/system/tests/modules/token_test/src/Controller/TestController.php
   --- /dev/null
   +++ b/core/modules/system/tests/modules/token_test/token_test.info.yml
   

   token.module already has a token_test.module, should we maybe name this one token_render? or token_test_render.

Great, but still missing the "also" from #49.2.

Oh missed that ....

token.module already has a token_test.module, should we maybe name this one token_render? or token_test_render.

Too bad for token module I'd say :)

Verified that all feedback in #52 and #53 has been addressed. Assigning to Berdir, because he wanted to take another look before RTBC'ing.

+++ b/core/modules/system/system.tokens.inc
@@ -154,7 +154,7 @@ function system_tokens($type, $tokens, array $data, array $options, CacheableMet
+      // We depend on the current request time, so the token aren't cacheable at all.

80 cols. Can be fixed on commit.

Why is it optional for generate?

Is that public API?

Should we throw an Exception if that is used wrongly?

Well, that is a good question. Its used to traverse into nested tokens, like node:author:uid. So yes we could require the cacheablity metadata as an earlier parameter, as you could make less mistakes.

Just a reroll for now.

Updated the issue summary. Reviewing the change record next.

Updated the change record, also included the hook changes (which will affect more people).

To be able to RTBC this myself, I'd like to one more detailed review. I won't find time for this during the day, maybe tonight.

We've had a number of reviews from different people, all agreeing with the approach and implementation. We've also had plenty of "nitpick reviews". What I've seen so far and before is also fine.

So it's more about being confident enough to actually set it. If someone else feels bold enough then that's fine with me too.

I understand the push to get Drupal 8 released and now I know there has not been anyone for a long time enforcing the document gate. Still, you can not, simply can not document hasRenderContext as it is documented here. It would be better to have no doxygen because it's much easier to find those with automated tools than these fake "yay we have doxygen" things. Seriously, what does the doxygen for hasRenderContext tell you that the function name does not?? At least crosslink to RenderContext itself.

I'm curious about changing the signature of generate to:

public function generate($type, array $tokens, CacheableMetadata &$cacheable_metadata, array $data = [], array $options = array()) {

There we go.

That should fix it

Yet one more "nitpick review":

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -153,18 +177,25 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
       *     scrubbing functions before displaying data to users.
       *
   +   * @param \Drupal\Core\Cache\CacheableMetadata &$cacheable_metadata|null
   

   Remove empty line.

2. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -41,6 +41,11 @@
     * @param $options
     *   (optional) An associative array of options for token replacement; see
     *   \Drupal\Core\Utility\Token::replace() for possible values.
   + * @param \Drupal\Core\Cache\CacheableMetadata &$cacheable_metadata
   + *   The cacheability metadata. By default we gather cacheability metadata from
   + *   the passed $data objects, but for global tokens the implementation needs
   + *   to take into account cacheable metadata from related objects, like the
   + *   'system.site' config object, for the 'site:name' token.
     *
     * @return
     *   An associative array of replacement values, keyed by the raw [type:token]
   @@ -49,7 +54,7 @@
   
   @@ -49,7 +54,7 @@
     * @see hook_token_info()
     * @see hook_tokens_alter()
     */
   -function hook_tokens($type, $tokens, array $data = array(), array $options = array()) {
   +function hook_tokens($type, $tokens, array $data, array $options, \Drupal\Core\Cache\CacheableMetadata &$cacheable_metadata) {
   

   I think you made the hook signature different because the values are always passed on anyway, right? If so, the docs need to be changed above.

Fixes #68 and makes the example hook implementation more valid.

This looks great, I few questions/feedback below. More than fine to move most of it to non-critical follow-ups.

Setting to needs work for some of comment things and what can be addressed here. I can open follow-ups later for some of the suggestions below if you agree with doing them.

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -152,19 +176,25 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
       *     scrubbing functions before displaying data to users.
   +   * @param \Drupal\Core\Cache\CacheableMetadata &$cacheable_metadata|null
   +   *   (optional) The cacheablity metadata. In case its not specified it will
   +   *   bubble up to the render context.
   

   I'm wondering what we should do for places that use token replace for non-output, like sent mails (which *is* the primary use case for it at least in core). That shouldn't be added to the render context even if there is one. Should that maybe explicitly be passed and then discarded? That seems a bit weird...

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -152,19 +176,25 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +    $passed_in_cacheable_metadata = (bool) $cacheable_metadata;
   

   $passed_in_cacheable_metadata sounds like the actual object, not a boolean. Don't have a better idea though.

3. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -173,12 +203,19 @@ public function replace($text, array $data = array(), array $options = array())
   +    // Ensure that tokens end up on the render context.
   +    if (!$passed_in_cacheable_metadata && $this->renderer->hasRenderContext()) {
   

   It's not the tokens that end up there but the cacheability metadata of the token replacements.

4. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -254,9 +294,24 @@ public function scan($text) {
   +    $cacheable_metadata = $cacheable_metadata ?: new CacheableMetadata();
   

   This line can be removed now that the argument is required.

5. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -49,7 +54,7 @@
   -function hook_tokens($type, $tokens, array $data = array(), array $options = array()) {
   +function hook_tokens($type, $tokens, array $data, array $options, \Drupal\Core\Cache\CacheableMetadata &$cacheable_metadata) {
   

   note to myself: need to check the change record whether this is also updated there.

6. +++ b/core/modules/system/src/Tests/System/TokenReplaceWebTest.php
   @@ -0,0 +1,44 @@
   +    $this->drupalGet('token-test/' . $node->id());
   +    $this->assertText("Tokens: {$node->id()} {$account->id()}");
   +    $this->assertCacheTags(['node:1', 'rendered', 'user:2']);
   

   We should also check for the cache contexts of this page, which should contain the user context now.

   Wondering if we should do a second request with a different user. That wouldn't really do anything right now, but it would be additional test coverage for smartcache. We probably should add quite a few additional tests there for cases like this.

7. +++ b/core/modules/system/system.tokens.inc
   @@ -145,19 +164,26 @@ function system_tokens($type, $tokens, array $data = array(), array $options = a
            case 'since':
              $replacements[$original] = \Drupal::service('date.formatter')->formatTimeDiffSince($date, array('langcode' => $langcode));
   +          $cacheablity_metadata->setCacheMaxAge(0);
              break;
   

   There's an issue to use JS in since formatting e.g. in views so that it can be cached. obviously not possible here.

8. +++ b/core/modules/views/views.tokens.inc
   @@ -83,6 +84,8 @@ function views_tokens($type, $tokens, array $data = array(), array $options = ar
        /** @var \Drupal\views\ViewExecutable $view */
        $view = $data['view'];
    
   +    $cacheablity_metadata = $cacheablity_metadata->merge(CacheableMetadata::createFromObject($view->storage));
   

   Interesting, this doesn't work through ViewExecutable. Should we maybe open an issue to implement that interface and pass calls to the storage, maybe even invoke something else?

   Can see that people would do createFromObject($view).

I'm wondering what we should do for places that use token replace for non-output, like sent mails (which *is* the primary use case for it at least in core). That shouldn't be added to the render context even if there is one. Should that maybe explicitly be passed and then discarded? That seems a bit weird...

On CLI, this does not matter, as you are outside of a rendering context.

On uncacheable request (Cron is probably one of them), it also doesn't matter.
For usecases like form submissions, it also doesn't matter because you are dealing with a POST request.

$passed_in_cacheable_metadata sounds like the actual object, not a boolean. Don't have a better idea though.

Went with a slightly better variable name.

Interesting, this doesn't work through ViewExecutable. Should we maybe open an issue to implement that interface and pass calls to the storage, maybe even invoke something else?

#2530572: Implement cacheable dependency interface for ViewExecutable

+++ b/core/modules/user/user.tokens.inc
@@ -136,6 +136,7 @@ function user_tokens($type, $tokens, array $data, array $options, CacheableMetad
     $account = User::load(\Drupal::currentUser()->id());
     $cacheablity_metadata = $cacheablity_metadata->merge(CacheableMetadata::createFromObject($account));
+    $cacheablity_metadata->addCacheContexts(['user']);
     $replacements += $token_service->generate('user', $tokens, array('user' => $account), $options, $cacheablity_metadata);

Oh, I'm not sure why I missed this :)

You actually don't need the first line, generate() should add that automatically, only the user cache context.

Quickfix for that and then RTBC :)

Good point.

Ok, I think this is ready.

We've had plenty of reviews, both architecture and nitpicks, lots and lots of tests, updated issue summary and change record.

dawehner++

Some minor nits that can be corrected on commit.

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -245,6 +282,9 @@ public function scan($text) {
   +   * @param \Drupal\Core\Cache\CacheableMetadata &$cacheable_metadata
   +   *    The cacheablity metadata. This is passed to lower token implementations
   +   *     can attach their metadata.
   

   Missing "so they"

   "This is passed to lower token implementations so they can attach their metadata."

2. +++ b/core/modules/system/system.tokens.inc
   @@ -137,6 +154,8 @@ function system_tokens($type, $tokens, array $data = array(), array $options = a
   +      // We depend on the current request time, so the token aren't cacheable at all.
   

   s/token/tokens

By the way I am no big fan of the use of contractions in formal documentation.

I know this is nothing to do with this issue, and so I am sorry about the noise.

Maybe I will open an issue for 8.1.x or something, to go through and change all those to the full English.

I.E. aren't = are not

And so on.

A quote from everythingenglishblog.com

Now that we know what a contraction is, we must determine when we should avoid them or use them. The answer lies in the formality of the document that you are preparing. If you are engaged in formal writing, I would suggest that you avoid using all contractions. This includes cover letters, résumés, theses, essays, etc. Because the use of contractions seems more informal, you should avoid them in any instance in which you want to portray a professional, respected image.

Emphasis is mine.

+++ b/core/modules/comment/comment.tokens.inc
@@ -176,20 +178,26 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
+          $cacheablity_metadata = $cacheablity_metadata->merge(CacheableMetadata::createFromObject($date_format));

Nit - looks like a typo - it should be $cacheability_metadata or $cacheable_metadata to follow token.api.php, isn't it? There are several instances of that.

I thought we had fixed that, missed that.

Yes, as written above, always $cacheable_metadata AFAIK.

Let's fix that and the two nitpicks above.

Meh.

@dawehner thanks for also changing the aren't to are not.

I did not intend for that to be a change here, was just noting my aversion to contractions in documentation.

But the change is welcome.

I have opened an 8.1.x issue for this to be done throughout the code.

Remove usage of contractions in all documentation

Mh.

Make sure there there's no cacheablity left in the patch :)

Back to RTBC.

:)

+++ b/core/modules/comment/comment.tokens.inc
@@ -176,20 +178,26 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
+          $cacheable_metadata = $cacheable_metadata->merge(CacheableMetadata::createFromObject($date_format));

Is it worth providing a method on CacheableMetadata that operates directly on $this so we don't have to overwrite the variable?

Like CacheableMetadata::combine() or CacheableMetadata::addCacheableMetadata()?

Yeah especially because it creates an object and then throws it away, its kinda a level of indirection which makes the code harder to read.

What if we add addCacheableDependency() to CacheableMetadata? We're actually doing the same to AccessResult in #2524082: Config overrides should provide cacheability metadata I believe. With another similar method for a render array or maybe even the same?

#86: Or even just add it to \Drupal\Core\Cache\RefinableCacheableDependencyInterface!

Knocking back to CNR. Those changes are a big part of the patch and it's what contrib modules will need to do to update for the API addition, so might as well make the change once.

Makes sense.

+1 to adding it to RefinableInterface in general but we're not actually implementing that yet, so I think we should do that in a separate issue, otherwise we'll have to change Entity here as well. We can just implement it in a consistent way?

We could even consider to not pass it by reference then, that would result in fewer changes in the token service to support the by-reference thing?

+1 to adding it to RefinableInterface in general but we're not actually implementing that yet, so I think we should do that in a separate issue, otherwise we'll have to change Entity here as well. We can just implement it in a consistent way?

+1

We could even consider to not pass it by reference then, that would result in fewer changes in the token service to support the by-reference thing?

I'll leave that to the Token system experts. I trust your judgment.

Well, indeed CacheableMetadata has a todo for RefineableInterface, but well, this would not really help the point mentioned by catch in #84

I went with mergeInto() for now but feel free to discuss a better name. This was just the first one I came up one and it was short enough.

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -294,12 +294,12 @@ public function scan($text) {
       if ($object instanceof CacheableDependencyInterface) {
-        $cacheable_metadata = $cacheable_metadata->merge(CacheableMetadata::createFromObject($object));
+        $cacheable_metadata->mergeInto(CacheableMetadata::createFromObject($object));
       }
     }

Forgot about RefinableCAcheableDependencyInterface for this issue. That said, we should definitely go with addCacheableDependency() here, that saves us another object and makes it even simpler:

$cacheable_metadata->addCacheableDependency($object);

Mh, the problem is that once we deal with interface, the optimization of CacheMetadata::merge() is no longer as straightforward as you need to call the methods at least

Yeah this is indeed a good point.

ups

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -254,9 +294,22 @@ public function scan($text) {
-    $replacements = $this->moduleHandler->invokeAll('tokens', array($type, $tokens, $data, $options));
+
+    foreach ($data as $object) {
+      if ($object instanceof CacheableDependencyInterface) {
+        $cacheable_metadata->addCacheableDependency($object);
+      }
+    }
+
+    $replacements = [];
+    $modules = $this->moduleHandler->getImplementations('tokens');
+    foreach ($modules as $module) {
+      $function = $module . '_tokens';
+      $result = $function($type, $tokens, $data, $options, $cacheable_metadata);
+      $replacements = NestedArray::mergeDeep($replacements, $result);
+    }

I think we could now use invokeAll() again since we no longer have a special by reference argument and save a few lines of code there.

Looks great otherwise, I really like how addCacheableDependency() simplifies the code. We might want to open a normal follow-up task to convert existing code to this method when possible.

Looks great otherwise, I really like how addCacheableDependency() simplifies the code. We might want to open a normal follow-up task to convert existing code to this method when possible.

+1

1. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
   @@ -133,6 +133,41 @@ public function setCacheMaxAge($max_age) {
   +   * Merges the values of another CachableMetadata object into this one.
   

   This is wrong; it's not only CacheableMetadata objects!

   Let's change this to: Adds a dependency on an object: merges its cacheability metadata. — this is what \Drupal\Core\Render\RendererInterface::addCacheableDependency's docs and \Drupal\Core\Cache\CacheableResponseInterface::addCacheableDependency's docs say.

2. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
   @@ -133,6 +133,41 @@ public function setCacheMaxAge($max_age) {
   +   * @param \Drupal\Core\Cache\CacheableDependencyInterface $other_object
   +   *   The cacheable dependency object.
   

   Are we sure we don't want to do

      * @param \Drupal\Core\Cache\CacheableDependencyInterface|mixed $dependency
      *   The dependency. If the object implements CacheableDependencyInterface,
      *   then its cacheability metadata will be used. Otherwise, the passed in
      *   object must be assumed to be uncacheable, so max-age 0 is set.
   

   … just like those two other places?

3. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
   @@ -133,6 +133,41 @@ public function setCacheMaxAge($max_age) {
   +  public function addCacheableDependency(CacheableDependencyInterface $other_object) {
   +
   +    // This is called many times per request, so avoid merging unless absolutely
   

   Nit: unnecessary \n.

There we go.

Thank you for the good reviews!

+++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
@@ -133,6 +133,42 @@ public function setCacheMaxAge($max_age) {
+   * @param \Drupal\Core\Cache\CacheableDependencyInterface|mixed $other_object
+   *   The dependency. If the object implements CacheableDependencyInterface,
+   *   then its cacheability metadata will be used. Otherwise, the passed in
+   *   object must be assumed to be uncacheable, so max-age 0 is set.

This doesn't actually match the implementation or the forced type hint. You need to remove that and do that inline and in else, set max age to 0.

If that's what we want to do?

Given that we want to make the API as easy as possible we probably should check inside the method?

This does the instanceof check and should work the same way as createFromObject().

I moved the optimization to the addCacheContexts()Tags() methods. I'm not sure we do that here since it means we e.g. don't run the cache context validation in some cases and then you'll only get an exception when something else is merged in or it is merged into something else.

Can also provide a patch without that, or we can do it in the trait in the follow-up to use that in CacheableMetadata/AccessResult.

I think this is totally fine, given that the actual cost of those objects has been in the validation (see previous profiling on the assert issue)
Sorry for not finding the motivation to work on it.

Might be a moot point since it's RTBC, but tagging as a beta target since it has some contrib impact and would be nice to get in by then.

I moved the optimization to the addCacheContexts()Tags() methods. I'm not sure we do that here since it means we e.g. don't run the cache context validation in some cases and then you'll only get an exception when something else is merged in or it is merged into something else.

+1

I think this is totally fine, given that the actual cost of those objects has been in the validation (see previous profiling on the assert issue)

So this means a worse DX for performance. Worse so than https://www.drupal.org/node/2259531, which still allows the better DX to be enabled; here you cannot enable a better DX.

I see the reasoning, but I think doing that optimization belongs in a separate issue with its own discussion; it shouldn't be done as part of a critical.

OTOH, \Drupal\Core\Cache\CacheableMetadata::merge() already follows this pattern. IOW: we're already inconsistent about this principle in the area of cacheability; we're basically blocked on #2408013: Adding Assertions to Drupal - Test Tools. to provide a better solution, that does maintain good DX.

Thought you'd say that ;)

I actually think it makes sense to do this, if we want to we can still do the validation in the optimized case. Even if some of that logic is moved to an assert then we're still doing a bunch of method calls, sorting and so on even if we're merging nothing with nothing or only one side has something.

But I agree with not doing it here. We can optimize it without losing the validation in the methods in the trait when we use that everywhere in the follow-up.

Her'es a patch without that optimization.

RTBC +1

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -152,19 +176,25 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   bubble up to the render context.
   

   Nit: s/to the render context./to the render context, if any./

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -245,6 +282,9 @@ public function scan($text) {
   +   *     so they can attach their metadata.
   

   Nit: indented one space too many.

3. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -35,12 +35,17 @@
   + *   to take into account cacheable metadata from related objects, like the
   

   Nit: s/cacheable/cacheability/

   (For consistency.)

RTBC +1

Only fixed Wim's Nits from #110

Back to RTBC if green.

@arlinsandbulte always provide an interdiff, if possible. Here is one

1. The current CR is a bit confusing I think it needs work to explain what token does automatically for you and what it does not and to make the code example more inline with the examples in the text.

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -7,12 +7,16 @@
   +use Drupal\Component\Utility\NestedArray;
   

   Not used.

3. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -35,12 +35,17 @@
   + * @param \Drupal\Core\Cache\CacheableMetadata $cacheable_metadata
   + *   The cacheability metadata. By default we gather cacheability metadata from
   + *   the passed $data objects, but for global tokens the implementation needs
   + *   to take into account cacheability metadata from related objects, like the
   + *   'system.site' config object, for the 'site:name' token.
   

   It's not just global tokens - it's related objects... like authors of nodes.

Thank you alex.

Addressed the review, I hope

1. Done: https://www.drupal.org/node/2528662/revisions/view/8673794/8675792
2. Fixed.
3. Done; hope you like it. Definitely not perfect yet though.

EDIT: cross-post with dawehner… I'll just post mine too, then we can choose which we prefer.

Looks like testbot likes Wim's version more, doesn't want to test the patch from @dawehner :) I think it's slightly better explain in #117.

Change record looks better. Wondering if we want to move more of the explanations into the intro chapter above the long examples.

@Berdir: the longer explanations seem to explain with concepts in the code. So either way, people need to read them with the code. Reading it, I think its fine now below.

+1 for #117

Given #118 + #119 + #120, and the fact that it's just a very minor reroll, re-RTBC'ing.

1. I think we have some missing dependencies
   • [comment:mail] can depend on the user entity
   • [comment:author] can depend on the user entity or config if the user is anonymous
   • [taxonomy:vocabulary] depends on the vocabulary entity
   • [user:name] can depend on config if the user is anonymous
2. What to do about things like entity:comment-count and other -count tokens... are these even cacheable?

3. $replacements += $token_service->generate('url', $url_tokens, array('path' => $url->getInternalPath()), $options);
   

   in views.tokens.inc is not passing the cacheability metadata along and looks broken and untested. Hmmm relies on token module... now that views is in core I think the token module needs to provide this.

What to do about things like entity:comment-count and other -count tokens... are these even cacheable?

Comment count is currently broken for node links as well, but we should be able to use the same approach as #2530846: Fix and improve comment cache tag usage - have to invalidate the entity cache tag when a comment is posted.

Nit on the patch:

+ * (optional) The cacheablity metadata. In case its not specified it will

s/cacheablity/cacheability/

I think @Fabianx is right.

Trying to address some of the feedback

Regarding the following tokens: [views:total-rows] [views:items-per-page] ... this would ideally get the cacheability metadata from the View object itself, which tracks them over time,
like for example in \Drupal\views\ViewExecutable::setItemsPerPage

I agree, in case you want to bubble up placeholders you need bubbleable metadata ... the question is, where does that stop? Don't all objects potentially want to bubble up placeholders?

I agree, in case you want to bubble up placeholders you need bubbleable metadata ... the question is, where does that stop? Don't all objects potentially want to bubble up placeholders?

No: because not all objects are renderable.

E.g. access results never render anything, you just can't render them; it doesn't make sense. Hence just cacheability metadata is fine.

E.g. tokens do render into something. Hence bubbleable metadata (cacheability + attachments) makes sense.

+++ b/core/modules/user/user.tokens.inc
@@ -93,6 +94,10 @@ function user_tokens($type, $tokens, array $data, array $options, CacheableMetad
+          if ($account->isAnonymous()) {
+            $cacheable_metadata->addCacheableDependency(\Drupal::config('user.settings'));
+          }
+          $cacheable_metadata->addCacheableDependency($account);

I think that adding the account is not necessary - as the token system should handle everything in $data - no?

I think that adding the account is not necessary - as the token system should handle everything in $data - no?

Thank you for your actual helpful comment!

Thank you wim for your kind comment.

No: because not all objects are renderable.

Well, while you think nodes aren't renderable, but views are. What blocks nodes from implementing \Drupal\Core\Render\AttachmentsInterface.

Other examples are for example date formats. What I'm actually not worried about is core but rather contrib and customland.

What blocks nodes from implementing \Drupal\Core\Render\AttachmentsInterface.

Oh, heh, that is very interesting :)

Other examples are for example date formats.

Yep, think "time ago" formatting that uses JS to keep it in sync with the current time.

I think @dawehner's comments basically mean "+1 to #124" ?

yeah for sure.

Alright, let me big renaming start ...

Bubble bubble bubble

This could fix more than 0 of the failures.

Just some random nits I found while waiting for a train

1. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -95,6 +96,19 @@ public static function createFromObject($object) {
   +   * @inheritDoc
   

   I guess the standard is {@inheritdoc}

2. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -34,22 +34,29 @@
   + *   An associative array of data objects to be used when generating
     *   replacement values, as supplied in the $data parameter to
   

   replacement fits now on the first line

3. +++ b/core/modules/system/tests/modules/token_test/token_test.info.yml
   @@ -0,0 +1,6 @@
   +name: Token test
   +type: module
   +core: 8.x
   +dependencies:
   + - user
   + - node
   

   This is missing at least package: testing, but maybe we want to add also version?

Thank you for the quick review laurii!

Let's reupload the patch, looks like a random test failure ...

Great, thanks @dawehner!

1. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\Asset\AttachedAssetsInterface;
   

   Unused interface.

2. +++ b/core/lib/Drupal/Core/Render/BubbleableMetadata.php
   @@ -95,6 +96,19 @@ public static function createFromObject($object) {
   +  public function addCacheableDependency($other_object) {
   

   BubbleableMetadata::addCacheableDependency()
   -> kind of unfortunate naming, but also not the worst.

   Just addDependency() would've been better probably. Thoughts?

3. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -173,12 +202,19 @@ public function replace($text, array $data = array(), array $options = array())
   +    // Ensure that cacheability metadata ends up on the render context.
   

   s/cacheability/bubbleable/

4. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -254,9 +293,16 @@ public function scan($text) {
   +      if ($object instanceof CacheableDependencyInterface) {
   +        $bubbleable_metadata->addCacheableDependency($object);
   +      }
   

   Shouldn't this now check both instanceof CacheableDependencyInterface and instanceof AttachmentsInterface?

5. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -34,22 +34,29 @@
   + *   The bubbleable metadata. By default we gather cacheability metadata from
   + *   the passed $data objects. For related objects such as authors of nodes, the
   + *   hook implementation needs to take into account cacheability of those
   + *   related objects as well. And finally, for global tokens, it is necessary to
   + *   take into account cacheability of the underlying object as well, like the
   + *   'system.site' config object for the 'site:name' token.
   
   @@ -120,10 +128,13 @@ function hook_tokens($type, $tokens, array $data = array(), array $options = arr
   + *   The bubbleable metadata. In case you alter an existing token based upon
   + *   cacheability metadata, the implementation needs to take that into account.
   

   s/cacheability/bubbleable/

6. +++ b/core/modules/comment/comment.tokens.inc
   @@ -138,6 +140,11 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
   +          // Add the user cacheablity metadata in case the author of the comment
   
   @@ -170,26 +177,37 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
   +          // Add the user cacheablity metadata in case the author of the comment
   

   These are correct!

7. +++ b/core/modules/system/tests/modules/token_test/src/Controller/TestController.php
   @@ -0,0 +1,81 @@
   +    // Note: We explicitly don't pass the cacheability metadata in order to
   

   s/cacheability/bubbleable/

8. +++ b/core/modules/views/src/Tests/TokenReplaceTest.php
   @@ -54,9 +55,25 @@ function testTokenReplacement() {
   +      $cacheable_metadata = new BubbleableMetadata();
   

   s/$cache/$bubble/

9. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheableMetadataTest.php
   @@ -43,6 +43,28 @@ public function testMerge(CacheableMetadata $a, CacheableMetadata $b, CacheableM
   +   * @covers ::addCacheableDependency
   ...
   +  public function testAddCacheableDependency(CacheableMetadata $a, CacheableMetadata $b, CacheableMetadata $expected) {
   

   We want a similar test in BubbleableMetadataTest now.

Argh, as the component maintainer, could someone have pinged me about this at least?

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -152,19 +175,25 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +    $bubbleable_metadata_is_passed_in = (bool) $bubbleable_metadata;
   +    $bubbleable_metadata = $bubbleable_metadata ?: new BubbleableMetadata();
   +
   

   This seems like a code smell to me. Why can't individual hook_tokens() handle if $bubbleable_metadata is set or not. Also, why isn't this passed in via $options?

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -245,6 +281,9 @@ public function scan($text) {
   +   * @param \Drupal\Core\Render\BubbleableMetadata &$bubbleable_metadata
   

   Objects don't need to be marked with & in documentation.

BubbleableMetadata::addCacheableDependency()
-> kind of unfortunate naming, but also not the worst.

Just addDependency() would've been better probably. Thoughts?

Well, in other words ::mergeInto() was not the worst name to start with :)

Shouldn't this now check both instanceof CacheableDependencyInterface and instanceof AttachmentsInterface?

It seems to be that this should be ideally an OR?

These are correct!

Nope, they have a typo.

We want a similar test in BubbleableMetadataTest now.

Ha, I thought about introducing one ...

Fixed #146.2 ... We no longer pass it by reference, but in difference places I have seen both and someone asked in this issue earlier.

This seems like a code smell to me. Why can't individual hook_tokens() handle if $bubbleable_metadata is set or not. Also, why isn't this passed in via $options?

Mh, I think passing it as options would make it less visible than it should be, given that callers should ideally pass it in. In options, it would be way easier to miss it.
Especially for ->generate() it would also make the api a bit harder to use.

@davereid
Do you want to give more feedback or are you more like: Meh okay its fine?
It seems to be that this issue feels ready for the rest of us, so we hope we can move forward, because waiting is a little bit pointless :)

I still think it should go in $options because we do that with other optional things like langcode/language, etc. But it feels like I'm being overridden by this being a critical and just needing a fix, so this should probably just get in as is.

Thank you for your quick response. No, its not that you are overridden, I think getting feedback from you is really valueable.
Yes I agree that ideally all that cacheablity should have been first designed and not put on top of everything afterwards.

In the following I try to make my previous point a bit better.

I still think it should go in $options because we do that with other optional things like langcode/language, etc. But it feels like I'm being overridden by this being a critical and just needing a fix, so this should probably just get in as is.

We have two kinda similar APIs out there which already take that object as parameter:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21PathProce...

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21RouteProc...

Another point I try to make is that we should rice awareness of it, if possible. If you forgot to add cacheabilitity metadata, you can easily get into bugs (by having old data in the cache entries) or even worse security problems by serving cache entries for users, which aren't supposed to

$options would have been a good solution if we had to add this in 8.1.x and make it as BC compatible as possible.

But a separate argument has a number of advantages IMHO:

a) $options in a hook (or $context or stuff like that) is actually really bad for DX. Just look at our implementations, almost all of them need identical ~10 lines of code to initialize stuff based on options, check if it's set or not. It's even worse for objects. A separate argument can be type hinted and then it is clear that it is always there and what it is.
b) As discussed before, It forces implementations to think about it when the implement that hook. The chance that contrib/custom modules, especially those that are upgraded from 7.x would consider it would be close to 0 if it would be just another thing in $options that they don't have to care about.
c) As mentioned, it's consistent with how we do it in other places now. Although we also have other patterns like return value objects, but that would be way too painful hook_tokens().

I've made some small changes to this issue but that was quite some iterations ago and most of that changed again or was reviewed in detail by others. So I think it's OK if set it back to RTBC.

However, I haven't checked yet if the feedback from @alexpott in #122 has been addressed yet. I don't think we need to do everything here, I think we should open a follow-up to check some of the remaining special tokens since there is no reason to block this issue on having perfect cacheability metadata for every single token.

The feedback #122.1 and #122.3 has been addressed.

What to do about things like entity:comment-count and other -count tokens... are these even cacheable?

Well, they should be ideally cacheable by the comment list tag for example, right?

Ok. I've opened #2536638: Dynamic tokens don't have correct cacheability, let's see if @alexpott agrees with getting this in and then look into those special tokens.

As commented over there, if we wait a bit for that comment issue then we can improve cacheablity a lot (or add a @todo on the other issue if we don't want to wait).