Add caching to \Drupal\Core\Utility\Token::getInfo()

The problem is token caching in contrib doesn't fly without a way to discover when the cache should be cleared. And if all modules implementing tokens would have to do it for token.module, token.module would become a requirement - thus a kind of extension of drupal core ;)
See #1211608: Caching requires modules to implement token.module API.

So let's better do it directly in core.

#2: 1211864-token-info-cache.patch queued for re-testing.

Let's get #1969540: Convert token.inc to a service in first, so we can inject the cache service into it.

Re-rolling patch for current dev branch on 7.x

When a site is using the entity.module, with an above average number of content types or fields, entity_token_tokens() and various token alters are expensive. entity.module itself was sucking up 50ms of processing time, as per a sample of cachegrinds.

While i've submitted a patch to entity.module for one part of their code, the fact token_info() isn't cached still remains.

This is a highly important speed increase for any drupal site with the entity module, or modules exposing a stack of tokens, which in reality is a now a lot of D7 sites.

I'd like to see this in 7.x before it's final days, as it seems the 8.x token solution is vastly different and wouldn't ever be "backported" to core.

As per the guidelines, this will have to be fixed in the most recent major version first.

I'm sprinting here at Drupalcon anyway. Will post a patch.

2: 1211864-token-info-cache.patch queued for re-testing.

Re-roll with a PHPUnit test.

14: drupal_1211864_14.patch queued for re-testing.

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -280,8 +304,16 @@ public function findWithPrefix(array $tokens, $prefix, $delimiter = ':') {
   +        $this->cache->set($cache_id, $this->tokenInfo, CacheBackendInterface::CACHE_PERMANENT, array(static::CACHE_TAG));
   

   Every unique cache tag adds performance overhead when loading caches.

   In DefaultPluginManager, I have been able to optimize this by instead of using a tag, loop over the languages and do a deleteMultiple, see DefaultPluginManager::clearCachedDefinitions()

2. +++ b/core/tests/Drupal/Tests/Core/Utility/TokenUnitTest.php
   @@ -0,0 +1,115 @@
   +
   +    // Get the information for the first time. The cache should be got, the
   +    // hooks invoked, and the cache should be set.
   

   The cache should be got sounds a bit strange, maybe it's easier to write this in a more direct way, something like it will try to fetch from the cache?

I rewrote some of the code comments, and added PHPUnit annotations. I also replaced the cache tags with a dedicated cache bin for tokens, so we can just flush the entire bin instead.

18: drupal_1211864_18.patch queued for re-testing.

18: drupal_1211864_18.patch queued for re-testing.

18: drupal_1211864_18.patch queued for re-testing.

18: drupal_1211864_18.patch queued for re-testing.

18: drupal_1211864_18.patch queued for re-testing.

#28 looks awesome. The only thing I'm missing is test coverage for checking any concrete token, but I guess this is already tested with WebTests and moving that to PHPUnit tests is a completely different thing. So RTBCing this.

Berdir told me having a separate cache bin is a no-go these days, so here is a patch that makes token info share the cosy cache.cache bin.

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -280,8 +309,16 @@ public function findWithPrefix(array $tokens, $prefix, $delimiter = ':') {
+        $this->cache->set($cache_id, $this->tokenInfo, CacheBackendInterface::CACHE_PERMANENT, array(static::TOKEN_INFO_CACHE_TAG));

@@ -305,5 +342,6 @@ public function setInfo(array $tokens) {
+    $this->cache->deleteTags(array(static::TOKEN_INFO_CACHE_TAG));

The format for the cache tag should be array(tag_name => TRUE), or array(tag_name => array(1, 2, 3)) when you have ID's.

30: drupal_1211864_30.patch queued for re-testing.

I guess we can use the new cache.discovery bin for this?

Thanks, now I'm happy :)

This feels like more of a task than a feature request.

Looks fine to me, but assigning to catch since it's performance-y.

Speaking of which: Benchmarks?

Not sure if we can do useful benchmarking as this will only become a bigger problem in contrib, when tokens will grow a lot due to field tokens and so on.

Gotcha. Ok, thanks!

Yeah this is a bit borderline since it only becomes a problem with contrib modules. However we have plenty of data that contrib implementations are expensive, so I think it's OK to add the cache here. Committed/pushed to 8.x, thanks!