Decouple cache tags from cache bins

I'm reluctant to add a dependency between the cache system and the module system, which will quickly be circular thanks to the various caches leveraged by the module system.

Can you elaborate on the use case?

Reassigning.

subscribing

Tagging issues not yet using summary template.

I'm demoting this to normal, although I think it is probably won't fix for D8 (at least I hope we can get cache tags in and I plan to work on that). Introducing a hook in the cache system would introduce another circular dependency.

In my original implementation there was a new class for cache tags which was not coupled with bins, and I made a point of being able to invalidate across all bins, with pluggable (non-bin) storage. I'm not sure why that got shot down. I quit Drupal a while back partly because I got tired of the epic ego battles necessary to get anything done.

If it helps at all, here's the original:

http://drupal.org/node/636454#comment-5157150

Having a single cache tag manager across all bins makes it harder to write an implementation that clears tags directly rather than using checksum (i.e. MongoDB could do that efficiently enough). However the current situation isn't great either so maybe we should just switch to that.

I'm confused by this issue though since tag invalidation already happens for all cache backends: http://api.drupal.org/api/drupal/core%21includes%21cache.inc/function/ca...

OK I think I get what sun is after, it's just that this misses the current implementation in the SQL backend already works across all bins.

Trying to remember the various arguments from the original issue:

- the current architecture where the tags are coupled to the bin is bad. What we currently do in the SQL backend is make the tags coupled to the backend rather than the bins via using a single table for invalidation and static class properties, but it's messy.

- switching from that to a single location for all cache bins looks much better architecturally, however limiting a site to a single cache tags implementation with potentially multiple cache implementations has real practical limitations:

- Cache tags can be implemented in Varnish using custom headers - this means that clearing the page (or blocks if they're cached with ESI) cache bin would make a call to the varnish backend to clear the tags via regexp. We need to allow for that implementation to co-exist with database/memcache/redis for everything else.

- Some cache backends such as Redis or MongoDB are able to store cache tags with the cache objects and clear them with a DELETE query efficiently, as opposed to relying on the checksum implementation that core ships with. Contrib backends should be free to experiment with the implementation and site admins will still want to mix and match backends in some cases (say APC for cache_bootstrap and Redis for everything else).

So...

What about if we just allow for multiple cache tag implementations - a separate interface to the backends, and each one is fired on every tag clear? Then if you have database + varnish both get invoked and there's no problem compared to now, but we get rid of the bin coupling.

The only issue then is that we have a cache interface which allows tags to be set, but doesn't allow them to be cleared, so we'll need to document the interdependency of the two somewhere, but that's not such a horrible problem to have.

And bumping to major, this is quite confusing https://drupal.org/node/1971158#comment-7618133

Will be an API change but 1. People should only call Cache::deleteTags() anyway, otherwise there's a bug 2. The real API change is for cache backend authors and there's only a handful of those (and it'll make it easier rather than harder for them).

Ok, here is a start on this. I've added:

- A CacheTagInterface containing invalidateTags and deleteTags methods. We might want to add something for actually getting whether a tag is invalidted or not but I haven't got there yet (the db cache bin currently just calls checksumTags() on the cache tag service atm).
- A CacheTagBase class, Mainly for the flattenTags method at this stage. We might want to just add this to our interface too? I think most implementations can share the current flattenTags, but should be on the interface I think.
- A DatabaseTag service; this basically just moved the invalidate/deleteTag logic from the DatabaseBackend to here.

Adding a method to check if tags are valid seems sensible - for some cache tag implementation that invalidate tags directly it'll be a null op so would have to always return TRUE (if there were mixed backends in use on a site) but I don't see a way around that.

We should have a factory similar to cache backends and pass that to the database cache backend as an argument rather than the specific database service. There's no hard dependency between the database cache + tags - you could use database tags with memcache cache, or memcache tags with database cache (not that you'd want to, but it'd work). For the varnish use case it'll be necessary to have two tags implementations running simultaneously - one for the 'real' cache and one for Varnish. If it's simple to have multiple tags services all called we could do that here, but potentially contrib could implement a chained backend that combines two or more as well.

Added the simplest factory I think is possible.

Not sure how we want to handle tests though? The generic cache backend test obviously tests tag logic, should we just keep that there? add unit tests only for the new tag classes?

#27: 918538-27.patch queued for re-testing.

#27: 918538-27.patch queued for re-testing.

Trying to change the Cache class, let's see how this goes.

Re-roll. It installs OK locally. Let's see what bot thinks about that and proceed from there.

It looks that change in Cache::deleteTags() and Cache:invalidateTags() causes this fails. I'm guessing that it is related to MemoryBackend, which implements tags in the backend class instead of using separate cache tags service.

For now I just reverted Cache::deleteTags() and Cache::invalidateTags() as it seem to work. I was thinking about implementing MemoryTag service, but I was unable to do it with current implementation. Since cache items are stored in MemoryBackend class tags service would need to get access to it. This results in a circular dependency. We could decouple "memory cache storage" in a new service which would be accessed by both cache bachend service and tags service. This seemed a bit of overengineering to me and I didn't go with that solution.

Yeah, this is the issue. Me and catch have spoken about this a couple of times. This issue is that we have cache tags that invalidate and cache tags, like memory that just remove the item directly.

I have a couple of ideas around this which I will do when I have time.

Either way, I think reverting the cache class Stuff in #37 doesn't really help us I'm afraid. As we just need to fix the current implementation anyway. Getting a patch green doesn't fix our problem :)

@damiankloip: Isn't it the same whether we clear tags via bins or directly via cache tags? The result should be the same, right? What do you think about the MemoryBackend change I proposed in #37? What other solutions do you see here?

Anyway... I found the source of that fail in #37. There are two drupal_static_reset() calls in test classes, which are clearing statically cached tags. Since we changed that with a variable on class it was not working correctly any more. I was trying to implement clear function on class itself and call it on the same places via service container, but it was still failing. Reverting back to drupal_static, just to confirm it really fixes this problem. Hopefully someone with better understanding of testing framework figures out a better solution.

This would fail if we'd revert in #37 or not, as the problem stays there in both implementations.

I was thinking about implementing MemoryTag service, but I was unable to do it with current implementation

Yes, this is why we need a slightly different/better solution for this, this stuff I had so far is not really adequate. I think we can maybe modify the memory backend slightly instead by injecting a new memory tag service (still not sure about this though). We really need to pass the cache item (I think) to the tag service to also get a chance to prepare the item. So some of that logic will just move there.

Isn't it the same whether we clear tags via bins or directly via cache tags?

No, the reason I was saying about the reverting back to looping through cache bins is that this is one of the main things we want to get away from (Same goes for the drupal_static_reset() that was added back in). So the fact that it does not pass either way doesn't matter really, as the whole patch needs a make over. So what we had before was a better base to work from IMO, as we'll just need to revert the reverted changes anyway...

#37 is potentially a way out, how about we just use the key value store for that? So we could inject the keyvalue factory for tests, and this could be used as this decoupled storage mechanism? Then the memory cache tag service would get passed the key value store too.

Bumping this to critical. Right now we have a method on the cache backend interface which is deleteTags(), but you should never call that directly. Code that does is not clearing the cache tag, but only for the bin (which might or might not delete it), since it's an API change needs to happen before release. Don't think it blocks beta.

Working on this...

@slashrsm, any joy with this?

Some joy and some pain. I've achieved some progress (created MemoryTag, implemented prepare function approach for cache items, ...).

I'm still trying to figure out how to fix installation problems, that were there in #35. I'm currently debugging my way through it.

This is based on #35. Added MemoryTag service and cache item prepareGet() function that is responsible for tags-related validation of cache item.

Let's try again....

50: 918538_50.patch queued for re-testing.

Fixed some failing tests. I removed tests that check deleteTags() and invalidateTags() on chain backend as cache backends don't implement those any more. We will need to add some tags test coverage anyway, which should cover that also. I'd be happy to write those tests, but first I'd like to see existing tests passing and some level of consensus about this patch.

Reroll after #2194273: Avoid repeated cache tag deletions.

Forgot to attach file.

Is this a straight reroll, or did you make functional changes?

No real functional changes. Only moved dupl. tag deleted tracking from cache backed service to tag service.

Looking at the test fails, it looks like all DUBT tests are broken which use the memory cache backend.

@Berdir: DUBT stands for?

http://api.drupal.org/api/search/8/DrupalUnitTestBase

With some other tests I saw problem with two instances of MemoryTag class being initiated. This way some tag delete requests went to one and others to the other. This caused cache items not being correctly invalidated/deleted. I am suspecting same problem also with this tests, but I was not able to find the actual source of the problem.

Does this seems to be reasonable explanation? Any idea how to debug/fix it?

I understand what this is after (making it more efficient to clear tags across bins), but the current implementation doesn't make much sense to me.

It is clear from the code and the interfaces that bins and tags are strongly bound together. Arbitrarily splitting them doesn't make much sense.

What would make sense, on the other hand is to split bins and *backend*. When you ask for a cache bin, you currently get a CacheBackendInterface... and that clearly doesn't make much sense.

Actually, there is already some sense of a weird separation between backend and bin, it's the CacheFactoryInterface. So one easy way to move this patch forward is to just:

• Rename CacheBackendInterface into CacheBinInterface
• Add a CacheBackendInterface extends CacheFactory, and move the deleteTags and invalidateTags methods there
• Move the concrete implementations of deleteTags and invalidateTags to their corresponding factory classes (ie. CacheBackendFactory, MemoryBackendFactory)
• Add a BackendChainFactory, given the current state of the code, it doesn't seem like the backend chain is actually usable because it is missing a factory

I can understand arguments that were pointed out in #72, but I still thing it would make sense to have tags storage and bin storage separated.

Posting another patch to see if I can get it closer to green.

77: 918538_76.patch queued for re-testing.

This has been brought up in many discussions, and I think it's relevant here: cache tag invalidation/deletion should be triggered not via method invocation, but via events. Having read the issue summary and the last few comments, it sounds like that could help with untangling architectural problems here.

But to be able to do that, we need to untangle this mess first. It is more a case of being able to clear tags and not have the whole static tags mess on the bins, which is actually the same thing but present in all these cache bin instances. That is basically a higher level than this is tackling IMO. Also from an API level, people are not sure (and frequently get wrong) what the difference is between Cache::deleteTags and $cache_bin->deleteTags(). Here we need to decouple the logic and functionality as much as we can to manage that.

How we actually implement tag clearing is another discussion I think.

I added a @todo to #2216543: Fill in topic/@defgroup docs for Cache overview to update the cache tag related documentation in here, I hope that will land asap (reviews welcome). The cache API really deserves some accurate and up to date documentation :)

Ok, was just trying to help :)

ok, thanks :) It is certainly some I would like to see. Hopefully this can happen in this release cycle!

Reroll.

Missing use.

This is at least a beta target, potentially a beta blocker.

Working on a re-roll, which is going to take some time, so assigning to me.

Initial re-roll, fixed unit tests and tested the installer with drush, let's see how this goes.

95: 918538_95.patch queued for re-testing.

Just trying to get the installer to pass, have some ideas to simplify this.

Working on clean-up and simplifying things. Only one cache tag backend is now supported, so removed the factory and directly registering the service, removed the cache.tags tag, directly calling deleteTags() and invalidateTags() on the service now. Based on what @slashrsm said, this might result in same new test fails but those will hopefully be fixable.

Also fixed some tests.

Ups, this will hopefully work a bit better, some more clean-up, removed drupal_static and cleaned up usage of cache.tag.database.

Thanks for picking this up again - totally forgot about this issue!

I think we were using a single cache tag implementation way back. I just don't think this is an option though :/ Passing the DB tag implementation into all backends...

This is exactly what the patch already did, the factory wasn't a factory, it was just a service locator for another service that was globally configured and passed the same implementation into all cache backends.

Yeah, I'm not 100% sure this will work, but the ability to provide an optimized cache tag implementation for your specific backend is more or less gone anyway when you get another service injected with a common API. I'll ask @znerol, he's working on a Redis cache backend implementation, which is one that is able to do better.

Now web tests should basically pass again.

The cache tag factory that this issue had was certainly flawed, that didn't make sense.

What *might* be an option is to limit the cache tag interface to deleting and invalidating cache tags (and clearCache for static caches), then each cache backend is free to implement the actual checking and implementation they use however they want (they could even keep it in the same class and only service-tag one cache backend, for example). They would all have to provide their own implementation and extend it in whatever way they see fit for their specific backend.

I feel that we first should explore the ideas from #72. I think that the tight coupling of tag and backend implementation is not much of a problem, but the lack of separation between cache-bin and cache-backend.

My suggestion is actually very similar to that, but it's not enough, not with the database cache backend implementation that we have now with the deletion/invalidation optimizations, cache backends need to tell the whatever class contains deletedTags/invalidatedTags arrays if they write something with it and I do not want to keep the current drupal_static() approach ;)

Indeed. So the generic approach seems sane to me. I think we may want to introduce a cache_tag service capable of dispatching the calls to the appropriate backend at some point.

+++ b/core/lib/Drupal/Core/Cache/CacheTagBackendInterface.php
@@ -0,0 +1,74 @@
+  /**
+   * Returns the sum total of validations for a given set of tags.
+   *
+   * @param array $tags
+   *   Associative array of tags.
+   * @param bool $set_context
+   *   TRUE if called from set context.
+   *
+   * @return array
+   *   Array with two items (invalidations, deletions) providing sum of all
+   *   invalidations/deletions over all provided tags.
+   */
+  public function checksumTags(array $tags, $set_context);

It will not be a surprise for you that I do not like this. The checksuming is an implementation detail. In fact the implementation in #2233413: Port Redis to Drupal 8 does not use tag-checksums but server-side set operations. I have plans to extend it even further and only use server-side lua-script for the more complex operations. What about introducing CacheTagBackendInterface::prepareSet($item), this also would correspond better with CacheTagBackendInterface::prepareGet($item)?

Yeah, the suggested approach would remove flattenTags, prepareItem() and checksumTags() from the public interface completely. The database implementation could then keep using that internally but others are free to do it differently.

#111 sounds great.

Going to do some work on this one.

Discussed this with @Berdir last week and finally got some time to look into this. I really like the direction we're going and really hope we can push this forward.

@slashrsm Please update summary with #111 and what you discuss with @Berdir

It turns out summary already describes cache tag service approach.

The IS really needs to be updated, because this hasn't been true in a long, long time:

Cache bins and cache tags are currently tightly coupled. In order to delete or invalidate cache items based on tags you need to do that via cache bin. Since there can be different cache backends for different bins one needs to be aware of that and delete/invalidate tags on both backends.

In HEAD, you call Cache::invalidateTags(), not $cache_bin->invalidateTags(). Cache::invalidateTags() iterates over all cache bins.

This does mean that it's still possible to not use Cache::invalidateTags() and just call $cache_bin->invalidateTags() directly, and that is definitely problematic, because it can easily cause very hard to debug problems.

I was referring to the approach discussed in last set of comments not the other things.

Just a note that despite what I said earlier, I actually found a perfectly valid reason for having multiple listeners/handlers/whatever on cache tag deletions/invalidations: Sending them to varnish.

I was able to do that pretty easily with the current API, by registering an arbitrary service that is tagged with cache.bin. That is then called automatically and is a null/no-op backend except deleteTags() and invalidateTags(), which send a http BAN request to varnish.

Whatever we end up doing here, we need to be able to support something like that.

Tagging this with D8 upgrade path. Not because we'll need an upgrade path, but it's going to at least require a rebuild.php after updating the code base due to service definitions being different.

Working on this for now.

Provide a service that uses the service_collector pattern to interact with both kinds of services. (It seems to me that one service can have multiple tags of the same name.)

We only need to interact with services implementing CacheTagInvalidationInterface generically.

The database cache backend can inject the database implementation of CacheTagInvalidationStorageInterface as part of the services.yml definition. We never need to call all CacheTagInvalidationStorageInterface at once since the return values of those should be identical.

Otherwise issue summary looks spot on now.

ksenzee, is anything going on with this?

ksenzee has working code in a sandbox. The approach looks good so far.

Do we have any links etc... to this sandbox? A patch here would be cool too :)

http://cgit.drupalcode.org/sandbox-ksenzee-1134904/log/ is the sandbox :).

Working a bit on this.

Ok, rerolled what @ksenzee was working on and copied the old cache tag implementation for the database backend, this installs again for me with that. (Did not without that).

Here's a patch for that, still needs a lot of work, obviously.

Small steps, tags storage backend really needs both interfaces, database factory does not need the invalidation. Did not touch the other backends yet.

Now the statics were different, trying again. Will have to change, but I honestly don't know how yet, given the implementation here.

Fixing the unit test fatals to get more test feedback.

Still very unsure about a lot of stuff.

I might need to switch to a test issue soon.

Updated the crazy drupal statics (they will go away, but first I want to make it pass) everywhere and fixed some kernel tests

This might be green....

Which doesn't mean it's ready for review, but now I can start refactoring the internals for the database cache backend and tags storage.

Still many open questions, but I might be getting there...

This should fix the cache tests, real work starting now :)

Now with 100% more patches.

drupal_static()'s, begone!

Clarification in the IS.

1. +++ b/core/core.services.yml
   @@ -32,6 +32,15 @@ services:
   +      - { name: cache_tags }
   

   I think this tag is not descriptive enough, makes it harder to understand this patch than necessary.

   Perhaps something like cache_tags_invalidator would be better?

2. +++ b/core/lib/Drupal/Core/Cache/CacheTagHandler.php
   @@ -0,0 +1,38 @@
   +   * Holds an array of cache tag invalidators.
   

   These are the services tagged with cache_tags.

3. +++ b/core/lib/Drupal/Core/Cache/MemoryBackendFactory.php
   @@ -7,13 +7,40 @@
   +  public function invalidateTags(array $tags) {
   +    foreach ($this->bins as $bin) {
   +      $bin->deleteTags($tags);
   +    }
   +  }
   

   Isn't it a problem that this still keeps a (public) invalidateTags() method on CacheBackendInterface? Why does CacheBackendInterface even have to implement CacheTagInvalidationInterface? Why can't it just let the cache back-end factories take care of this? Because an individual cache back-end implementation might react differently? If that's why, then I think we need to rename the methods. In fact, I think we need to rename them anyway.

   /**
    * Defines methods required for classes that respond to cache tag invalidations.
    */
   interface CacheTagInvalidationInterface {
   

   This says that implementations of this method react to changes. But the method names pretend they make the changes:

     public function deleteTags(array $tags);
     public function invalidateTags(array $tags);
   

   What about this instead?

     public function deletedTags(array $tags);
     public function invalidatedTags(array $tags);
   

4. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1148,12 +1148,7 @@ protected function resetAll() {
   -    // @todo Replace drupal_static() usage within classes and provide a
   -    //   proper interface for invoking reset() on a cache backend:
   -    //   https://www.drupal.org/node/2311945.
   -    drupal_static_reset('Drupal\Core\Cache\CacheBackendInterface::tagCache');
   -    drupal_static_reset('Drupal\Core\Cache\DatabaseBackend::deletedTags');
   -    drupal_static_reset('Drupal\Core\Cache\DatabaseBackend::invalidatedTags');
   +    \Drupal::service('cache_tag_storage')->reset();
   

   Nice :)

Do we want to get rid of deleteTags() and keep only invalidateTags() in this issue as well? It'd make the patch more elegant/easier to review also IMHO, because currently this:

/**
 * Defines methods required for classes that respond to cache tag invalidations.
 */
interface CacheTagInvalidationInterface {

makes "tags deleted" sound like something that doesn't belong on that interface at all.

Agreed on removing cache tag deletions.

It adds a lot of complexity to the implementation - have to keep track of two things (or ignore invalidations and just delete).

Also it's up to code getting cache items whether they use $allow_invalid or not, when invalidating tags it's either valid or not.

I committed the patch that added this, but didn't properly think it through at the time.

This removes deleteTags(), @catch, @WimLeers, @Fabianx and myself all agree that there is no use case for this. Further work should be easier with that gone, so doing it here, as we have only half the amount of methods to write/test/rewrite and simpler API's.

As of #151/#152, this now also greatly enhances DX, by removing the extremely confusing difference between "deleting tags" and "invalidating tags".

Further work should be easier with that gone, so doing it here, as we have only half the amount of methods to write/test/rewrite and simpler API's.

Yay!

The #152 interdiff looks awesome.

Now that we have that, it's time to start cleaning this patch up. Berdir and I discussed my review in #150, he's going to use a more meaningful service tag, going to remove the need for CacheBackendInterface implementations to still implement an invalidateTags() method (but still keep that possibility open for the "special snowflake" cache back-ends that need it, like ChainedFastBackend and MemoryBackend). That should make this patch a lot cleaner and easier to review.
Marking NW for that :)

Ok....

A few changes :)

- Renamed CacheTagHandler to CacheTagsInvalidator (+ interface)
- Removed the null implementation and install override for that
- Removed CacheTagInvalidationInterface, now using the above interface also for the listeners. This needs to be discussed, we might want to have a different name of the two methods. I quite like it, only one interface means you can't accidentally implement the wrong one.
- Changed the Invalidator to also call the method on all bins that implement the interface
- Removed it from all backends that don't need it
- Using the cache tag storage in the apcu and php backends
- Updated unit and generic cache backend tests
- Moved the cache tag invalidation into the Invalidator, moved the unit test for it

Go testbot

Ok, so we need a null implementation but only when there is no settings.php already.

I decided to make a null implementation of the tags storage instead, that makes a bit more sense to me.

Wow, huge progress! Will review.

Actually, we don't need the null implementation, we can just be sneaky and remove the tag. The failing tests gave me that idea, as we didn't even register the null implementation. Fixed that now, also fixed two invalidateTags() calls that still called the backend directly.

The patch is looking better and better :)

1. +++ b/core/lib/Drupal/Core/Cache/CacheTagsInvalidationStorageInterface.php
   @@ -0,0 +1,40 @@
   +   * @param array $tags
   ...
   +   * @param array $tags
   

   string[]

   (It's correct everywhere else.)

2. +++ b/core/lib/Drupal/Core/Cache/CacheTagsInvalidator.php
   @@ -0,0 +1,71 @@
   +    // Notify all added cache tag validators.
   

   s/validator/invalidators/

3. +++ b/core/lib/Drupal/Core/Cache/CacheTagsInvalidator.php
   @@ -0,0 +1,71 @@
   +    }
   +
   +  }
   

   Extraneous newline.

4. +++ b/core/lib/Drupal/Core/Cache/CacheTagsInvalidator.php
   @@ -0,0 +1,71 @@
   +   * @return \Drupal\Core\Cache\CacheBackendInterface
   +   *  An array of cache backend objects keyed by cache bins.
   

   @return statement needs a trailing "[]" to indciate it's an array.

   The doc line needs one extra leading space.

5. +++ b/core/lib/Drupal/Core/Cache/ChainedFastBackendFactory.php
   @@ -53,7 +53,7 @@ public function __construct(Settings $settings = NULL, $consistent_service_name
   -    if (!isset($fast_service_name) && function_exists('apc_fetch')) {
   +    if (FALSE && !isset($fast_service_name) && function_exists('apc_fetch')) {
   

   Needs to be fixed still (I know this is just temporary, to see if the overall approach is viable).

6. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -35,19 +35,29 @@ class DatabaseBackend implements CacheBackendInterface {
   +   *   The object that stores cache tag invalidations.
   

   s/The object that stores/Storage/

7. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -174,27 +182,13 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array
   -      'checksum_invalidations' => $checksum['invalidations'],
   -      'checksum_deletions' => $checksum['deletions'],
   +      'checksum_invalidations' => $checksum,
   
   @@ -229,7 +220,7 @@ public function setMultiple(array $items) {
   -        ->fields(array('cid', 'data', 'expire', 'created', 'serialized', 'tags', 'checksum_invalidations', 'checksum_deletions'));
   +        ->fields(array('cid', 'data', 'expire', 'created', 'serialized', 'tags', 'checksum_invalidations'));
   
   @@ -237,32 +228,21 @@ public function setMultiple(array $items) {
   -          'checksum_invalidations' => $checksum['invalidations'],
   -          'checksum_deletions' => $checksum['deletions'],
   +          'checksum_invalidations' => $checksum,
   

   Perhaps it makes sense to rename 'checksum_invalidations' to just 'checksum' or just 'invalidations'?

8. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -516,14 +406,16 @@ protected function ensureBinExists() {
   +   * If the table does not yet exist, that's fine, but if the table
       * exists and yet the query failed, then the cache is stale and the
       * exception needs to propagate.
   

   80 col rule.

9. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -552,10 +444,10 @@ protected function normalizeCid($cid) {
   +   * Defines the schema for the {cache_*} bin table.
   

   s/table/tables/

10. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsStorage.php
    @@ -0,0 +1,196 @@
    +use Drupal\Core\Database\SchemaObjectExistsException;
    

    Missing leading backslash.

11. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsStorage.php
    @@ -0,0 +1,196 @@
    +   * Constructs a DatabaseBackend object.
    

    DatabaseCacheTagsStorage

12. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsStorage.php
    @@ -0,0 +1,196 @@
    +      // Create the cache table, which will be empty. This fixes cases during
    +      // core install where a cache table is cleared before it is set
    +      // with {cache_render} and {cache_data}.
    

    Copy/pasted comment is wrong.

13. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsStorage.php
    @@ -0,0 +1,196 @@
    +    // Remove tags that were already invalidated during this request/ from the
    

    s#request/#request#

14. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsStorage.php
    @@ -0,0 +1,196 @@
    +      // If another process has already created the cache tags table, attempting to
    +      // recreate it will throw an exception. In this case just catch the
    +      // exception and do nothing.
    

    80 cols.

    Comment seems in the wrong place too?

15. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsStorage.php
    @@ -0,0 +1,196 @@
    +      'description' => 'Cache table for tracking cache tags related to the cache bin.',
    

    Not related to "the" cache bin, but to all cache bins?

16. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsStorage.php
    @@ -0,0 +1,196 @@
    +  }
    +}
    

    Needs newline in between.

17. +++ b/core/lib/Drupal/Core/Cache/PhpBackend.php
    @@ -122,6 +132,12 @@ protected function prepareItem($cache, $allow_invalid) {
    +    // Check if invalidateTags() has been called with any of the entry's tags.
    

    s/entry/item/

18. --- a/core/modules/field/src/Entity/FieldStorageConfig.php
    +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
    

    This doesn't seem to belong in this patch? :)

19. +++ b/core/modules/system/core.api.php
    @@ -496,7 +496,7 @@
      * // Delete or invalidate all cache items with certain tags.
    - * \Drupal\Core\Cache\Cache::deleteTags(array('node:1'));
    + * \Drupal\Core\Cache\Cache::invalidateTags(array('node:1'));
      * \Drupal\Core\Cache\Cache::invalidateTags(array('user:1'));
    

    s/Delete or invalidate/Invalidate/

20. +++ b/core/modules/views/tests/src/Unit/ViewsDataTest.php
    @@ -26,6 +26,13 @@ class ViewsDataTest extends UnitTestCase {
    +   * The mocked cache tag invalidator.
    
    +++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.php
    @@ -21,7 +21,14 @@ class CacheCollectorTest extends UnitTestCase {
    +   * The cache tag invalidator.
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -63,7 +63,14 @@ class EntityManagerTest extends UnitTestCase {
    +   * The cache tag invalidator.
    

    s/tag/tags/

21. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.php
    @@ -21,7 +21,14 @@ class CacheCollectorTest extends UnitTestCase {
    +   * @var \PHPUnit_Framework_MockObject_MockObject
    

    Missing the "OR cache tags invalidator interface" part.

22. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheTagsInvalidatorTest.php
    @@ -0,0 +1,31 @@
    +class CacheTagsInvalidatorTest extends UnitTestCase {
    

    This probably needs more test coverage.

23. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheTest.php
    @@ -117,24 +117,4 @@ public function testBuildTags($prefix, array $suffixes, array $expected) {
    -  /**
    -   * @covers ::invalidateTags
    -   *
    -   * @expectedException \LogicException
    -   * @expectedExceptionMessage Cache tags must be strings, array given.
    -   */
    -  public function testInvalidateTags() {
    -    Cache::invalidateTags(['node' => [2, 3, 5, 8, 13]]);
    -  }
    

    I don't think we actually want to remove this?

24. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityUnitTest.php
    @@ -75,9 +75,9 @@ class EntityUnitTest extends UnitTestCase {
        * The mocked cache backend.
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/KeyValueStore/KeyValueEntityStorageTest.php
    @@ -70,9 +70,9 @@ class KeyValueEntityStorageTest extends UnitTestCase {
        * The mocked cache backend.
    

    s/backend/tags invalidator/

25. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
    @@ -121,7 +121,7 @@ protected function setUp() {
    -    $cache_backend = $this->getMock('Drupal\Core\Cache\CacheBackendInterface');
    +    $cache_backend = $this->getMock('Drupal\Core\Cache\CacheTagsInvalidatorInterface');
    

    Requires an update of the property docs.

26. +++ b/core/tests/Drupal/Tests/UnitTestCase.php
    @@ -199,22 +198,18 @@ public function getStringTranslationStub() {
    -  protected function getContainerWithCacheBins(CacheBackendInterface $backend) {
    +  protected function getContainerWithCacheBins(CacheTagsInvalidatorInterface $cache_tags_handler) {
    

    The docs for this function are now wrong/outdated. And probably even the name is now wrong-ish?

Fixing this, when I don't comment on some points then it is fixed.

5. Oh, I forgot about that, let's see if it is still working :) See also the patch I posted in the fast backend & cache tags issue, that would help a lot with the (hopefully small) regression introduced here.
7. Not sure about the naming around checksum/invalidations, also the method.
9. @xjm agreed, changed.
10. No, use should never have \, the problem was that the one above had, not that this didn't :)
14. Moved down, same as DatabaseBackend now, also changed to cachetags, then it's exactly 80 characters.
18. Ha, no, it doesn't belong in any patch, was just trying some for a discussion here
20. Fixed all of those that I could find.
22. Yes, added another test, not perfect unit test as we call multiple methods but IMHO fine.
23. Yes, we do, because the validation is now in CacheTagsInvalidator, so I moved it to CacheTagsInvalidatorTest that you reviewed in 22 :)
25. not sure what you mean with property docs it is not a propery, I did rename the local variable name
26. Yes, I think we should just drop that method anyway and use a real Container like we do in many other tests. Updated/renamed.

I thought you reviewed this :p

I think we should switch the generic tests to use their factories, at least were we can.

Time to go home if I'm doing embarrassing stuff like that.. Re-uploaded the old patches.

Generally, this is looking fantastic! Thanks for picking this up again berdir.

I didn't have a chance to actually test yet (will do a bit later) but here are a few points:

1. +++ b/core/core.services.yml
   @@ -32,6 +32,17 @@ services:
   +  cache_tags_storage:
   

   I think this should have database in the name too?

2. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
   @@ -163,18 +163,13 @@ protected function prepareItem($cache, $allow_invalid) {
   +    $checksum = $this->cacheTagsStorage->checksumTags($cache->tags);
   

   Similar here, diff context might be lacking, but do we need the $context variable?

3. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
   @@ -189,16 +184,19 @@ protected function prepareItem($cache, $allow_invalid) {
   +    if ($tags) {
   +      Cache::validateTags($tags);
   +      $this->cacheTagsStorage->onCacheTagsWrite($tags);
   +    }
   

   Is it worth a short comment here that this is first, so checksums below are updated from this?

4. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
   @@ -189,16 +184,19 @@ protected function prepareItem($cache, $allow_invalid) {
   +    $checksum = $this->cacheTagsStorage->checksumTags($tags);
   +    $cache->checksum_invalidations = $checksum;
   

   This can just be assigned directly.

5. +++ b/core/lib/Drupal/Core/Cache/Cache.php
   @@ -94,43 +94,13 @@ public static function buildTags($prefix, array $suffixes) {
   +    \Drupal::service('cache_tags_invalidator')->invalidateTags($tags);
   

   Now we're talking, we've wanted to get to something like this for a WHILE :)

6. +++ b/core/lib/Drupal/Core/Cache/CacheTagsInvalidator.php
   @@ -0,0 +1,72 @@
   +    // Notify all added cache tags invalidators.
   +    foreach ($this->invalidators as $invalidator) {
   +      $invalidator->invalidateTags($tags);
   +    }
   

   Howcome we want to do this, then we are still allowing invalidateTags() on a specific bin, which is a big reason to separate this stuff in the first place (even though this will just proxy to the tag storage) :/

   I thought this would be the other way round maybe - if it is some self container invalidation bin, call it?

   This will then still loop over all cache tag invalidators and bins when we invalidate tags?

7. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -116,18 +126,13 @@ protected function prepareItem($cache, $allow_invalid) {
   +    $checksum = $this->cacheTagStorage->checksumTags($cache->tags);
   

   Again, can this var be removed?

8. +++ b/core/modules/config/src/Tests/Storage/CachedStorageTest.php
   @@ -90,7 +90,8 @@ public function containerBuild(ContainerBuilder $container) {
   diff --git a/core/modules/field/src/Entity/FieldStorageConfig.php b/core/modules/field/src/Entity/FieldStorageConfig.php
   
   diff --git a/core/modules/field/src/Entity/FieldStorageConfig.php b/core/modules/field/src/Entity/FieldStorageConfig.php
   index 9ae0fc4..a82f2ba 100644
   
   index 9ae0fc4..a82f2ba 100644
   --- a/core/modules/field/src/Entity/FieldStorageConfig.php
   
   --- a/core/modules/field/src/Entity/FieldStorageConfig.php
   +++ b/core/modules/field/src/Entity/FieldStorageConfig.php
   

   Is this file meant to be here?

9. +++ b/core/modules/system/src/Tests/Cache/ChainedFastBackendUnitTest.php
   @@ -25,8 +25,8 @@ class ChainedFastBackendUnitTest extends GenericCacheBackendUnitTestBase {
   +    $consistent_backend = new DatabaseBackend($this->container->get('database'), \Drupal::service('cache_tags_storage'), $bin);
   +    $fast_backend = new PhpBackend($bin, \Drupal::service('cache_tags_storage'));
   

   We should use \Drupal::service or $this->container

#161:
10. oops :)
18. :)
25. oops.

damiankloip already did a more architecture-level review. So here's another review with only nitpicks, that I already started, but meant to expand to do the review that Damian now already did :)

1. +++ b/core/lib/Drupal/Core/Cache/CacheTagsInvalidator.php
   @@ -55,15 +52,19 @@ public function addInvalidator(CacheTagsInvalidatorInterface $invalidator) {
   +   *   An array of cache backend objects that implement the invalidator interface,
   

   80 cols.

2. +++ b/core/modules/system/core.api.php
   @@ -495,8 +495,7 @@
   + * // nvalidate all cache items with certain tags.
   

   s/nvalidate/Invalidate/

3. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheTagsInvalidatorTest.php
   @@ -23,9 +24,46 @@ class CacheTagsInvalidatorTest extends UnitTestCase {
   +    // This does not actually implement,
   +    // \Drupal\Cache\Cache\CacheBackendInterface but we can not mock from two
   

   Misplaced comma.

4. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheTagsInvalidatorTest.php
   @@ -23,9 +24,46 @@ class CacheTagsInvalidatorTest extends UnitTestCase {
   +    // We do not have to define that invalidateTags() is never called as the
   +    // interface does not define that method, trying to call it would result in
   +    // a fatal error.
   

   Hah, creative :)

5. +++ b/core/tests/Drupal/Tests/UnitTestCase.php
   @@ -196,15 +196,15 @@ public function getStringTranslationStub() {
   +  protected function getContainerWithCacheTagsInvalidator(CacheTagsInvalidatorInterface $cache_tags_handler) {
   

   s/$cache_tags_handler/$cache_tags_invalidator/

Overall looks great.

From a clean state architecture this is what I would expect:

- There are n cache tags storages in the system and m cache tags invalidators. [YES, in addition any cache_tags.storage can subscribe to invalidation 'events']
- Cache Backends get the cache tags storage injected. [YES]
- There is a cache_tags.storage default cache tags storage [YES, here called cache_tags_storage]
- Both Cache Tags storages and cache tags invalidators are "subscribed" to cache tag invalidation 'events'. [YES]
- Cache Tags storages in addition can be queried for cache tags (cache tag checksums) [YES]

I would love from a naming to be more like:

- cache_tags (central service used for any operations, like invalidations)
- cache_tags.storage (the default storage injected in cache backends)

e.g.

\Drupal::service('cache_tags')->invalidateTags();

In addition to this the patch does:

- Allow any cache bin to subscribe to invalidation events and while that is for now the simplest implementation (with the least diff), it also leaves a lot to be desired in terms of de-coupling:

* For ACPU backend, there is no need to get the bin instance, the bin name is usually enough to invalidate the cache centrally.
* For redis backend the name might be enough or it might need to get the actual bins via container itself.

I am not convinced making the default implementation container aware is good here and giving bins / backends this capability.

I can see however that given the size of the patch, this might need to be a major follow-up instead.

OPEN QUESTIONS:

Operations like checksums should be done by the cache_tags service and not the storage service, so that storage can be exchanged.

The storage should IMHO just be responsible for getting and retrieving tag data (e.g. timestamps, increment counters, whatever) - nothing else.

After speaking with berdir here comes the hard part: Naming things! :)

Proposal was to use:

CacheTagsChecksumProvider instead of CacheTagsInvalidationStorage.

That it is stored somewhere is an implementation detail.

Use cache_tags and just have to learn that \Drupal::service('cache_tags')->invalidateTags() won't work (Exception: Method not found), but have to use \Drupal::service('cache_tags_invalidator')->invalidateTags() instead. (or Cache::invalidateTags).

Also do use:

$this->cacheTags->checksum($tags);

to have less redundant tags in there.

+++ b/core/lib/Drupal/Core/Cache/CacheTagsInvalidator.php
@@ -0,0 +1,72 @@
+
+    // Additionally, notify each cache bin if it implements the service.
+    foreach ($this->getInvalidatorCacheBins() as $bin) {
+      $bin->invalidateTags($tags);
+    }

Also and I am not sure how others feel about it, I think we could move that to a cache_tags_invalidator.cache_bin service.

This would enable us to make this service not container aware, but only the one using the cache bins.

And such remove the special case, but just have cache_bin service be one invalidation service in addition to the others that is called in the invalidateTags above.

Also and I am not sure how others feel about it, I think we could move that to a cache_tags_invalidator.cache_bin service.

Or maybe implement a compiler pass which collects all cache bins implementing the CacheTagsInvalidatorInterface and registers it with the cache_tags_invalidator service?

Catching up with reviews (only commenting if there is something relevant to say):

#166:

1. As discussed, there's only one storage, so no, I don't think so.
2. Removed where possible, also renamed checksum_invalidations to just checksum
3. I just tried to put all the $tags "massaging" together. After thinking it through, the order does not matter. The count will be the same, unless a different server triggered a cache tag invalidation in the meantime. And we also don't invalidate the cache of that, we just remove the flag that we already invalidated that specific cache tag in this request (if we did), so that if there is another invalidation later in the request, we make sure that it counts. And this is documented inside onCacheTagsWrite() and not outside, as it is an implementation detail.
6. As discussed. We both have explicit invalidator registrations, those are not cache bins. And we also check all bins if they implement the interface and notify them as well. We need to do it, backends can separate out the invalidation to separate services if they are capable, but some implemenations will never be able to do that and the chains also need to account for that.
8. Uhm, I thought I already removed that above.

#167: Fixed.

#168-168: Not changed anything yet, need to think more about it. Would like to hear what others think about the storage name change and how we should call the services. I don't like the idea of a cache_tags service that both publishes cache tag invalidations and manages checksums (with a storage backing it), which means it is an invalidation subscriber as well. That seems to contradict your own argument about one service should only do one thing ;) I'm fine with moving the bin notifications to a separate service if others agree with #170, but for me, that belongs together.

#171: No, that is not possible. Cache backend settings are in $settings, so that they can be changed without rebuilding the container, and then what we expect it to be might no longer be true. It could also depend on the environment, for example, if you do a cache clear through apache, you get a fast chained backend that implements the service, if you do it with drush, you directly get the database backend which doesn't implement the service. And it would mean that we have to instantiate all bins during a container rebuild, which is something we should try to avoid if possible I think.

This also needs more code documentatio, I will try to update the chapter in core.api.php among other things.

More stuff that doesn't belong in there...

#2392235: ChainedFastBackend shouldn't write cache tags to the fast cache back-end got in, that solves the possible regression when the apcu cache backend uses the common cache tags storage and therefore the need for profiling IMHO. It would still be a problem if apcubackend is used directly, but that is not common as then not even drush can be used as the shared memory can not be shared with it.

Updated the issue summary, please review.

On the naming issue I'd go for checksum.

I also don't love the idea of still invalidating the bins sometimes, but I'm not sure there's a better alternative.

The main bug that the interface encouraged direct invalidations at bin level is gone.

If we wanted to not allow the bin storage to implement that interface, backends like memory would need an additional service that handles storage and invalidations, injected into each bin instance. That doesn't really feel like an improvement.

Right now, 1. and 2. are the same API, if we were to split that up, we could make 3. part of 1.

This is tempting.

Ok, cache_tags.checksums or cache_tags.checksum_provider? And CacheTagsChecksumProviderInterface or without Provider, or something else?

On 3., any suggestions for method names for 1. and 2. ? right now it is checksumTags(). I have no idea what would make sense.. checksumTagsForSet/Write() for 1? Should the method for 2. then just return TRUE/FALSE, something like $this->checksumProvider->isValid($tags, $checksum)?

Only partially related to this issue, but I noticed in #2395143: YAML parsing is very slow, cache it with FileCache that we do thousands and thousands of markAsOutdated() queries and most of them due to deleteTags()/invalidateTags(). 6100 calls in total in standard install, 3800 of those from delete/invalidate tags.

I think we should look into not using chained fast during the installer and maybe even just using a memory implementation. That makes up 10% of the whole install runtime.

Or a null implementation: don't cache anything during install, would be even more consistent I think. Depends on the cache items if it is being reused during the same request or not I guess.

If we do $this->checksumProvider->isValid($tags, $checksum)? then can we just leave checkSumTags() as the other method name?

On the class/interface naming I'd skip provider I think.