Updated: Comment #175

Problem/Motivation

Drupal 8 uses "cache tags" to help determining whether a cached item is valid or not. If a piece of rendered HTML contains node 1 then the change of node 1 (tag: node:1 will need to invalidate that piece of HTML -- but also the author of the node (user:123) and any taxonomy terms on the node. On the other hand, changing node 1 also needs to invalidate the path alias cache for that etc -- so cache tag invalidations are not restricted to a single bin.

Currently, cache bins and cache tags are 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. Drupal 8 has had Cache::invalidateTags() for many months now, which automatically iterates over all cache back-ends, but the problem is that it's still possible to only invalidate tags on a single cache back-end. It should not be possible at all.

The mismatch between the fact that cache tags are a global concept but every cache backend/bin must provide it's own implementation results in more complexity and duplicated and/or inconsistent code. Some backends then have a global implemenation like the database but are still called per-bin, so there is a lot of code to avoid invalidating cache tags multiple times in a row in the database backend, but the ApcuBackend does not do that. And the PhpBackend loads all files, checks them for cache tags and invalidates them.

The common way to implement cache tag based invalidation is to use checksums and compare them when loading cache entries. When they don't match, the cache item is invalid. However, some backends implement different concepts. The fast chained backend does not propagate cache tags to the fast backend but invalidates it in case a cache tag is invalidated as that must be distributed to other servers anyway. The memory backend can simply loop over all entries and invalidate them and a redis implementation can use server-side set operations to delete cache tags.

Proposed resolution

Two new concepts are introduces:

Cache tags invalidation storage/checksum provider
Currently named invalidation storage, but will probably be renamed to CacheTagsChecksumProvider (cache_tags.checksums?), so that it is named after what it provides and not how it does it. Is responsible for providing the checksum for a set of tags, the default implementation is what was in DatabaseBackend, but cleaned up (no drupal_static() anymore) and simplified.

Cache tags invalidators
Services tagged with cache_tags_invalidator, they react to cache tag invalidations. There is a central service (currently cache_tags_invalidator, possibly cache_tags.invalidator) that notifies all other services. Those include the checksum provider, other use cases are varnish/nginx integrations that ban cached pages based on their cache tags, or a logger that tracks cache tag invalidations.

Additionally to explicitly registered services, all cache bins where the backend implementation implements the CacheTagsInvalidatorInterface are also notified about any invalidations, to support the mentioned use cases.

Backends that use the checksum implementation can just inject the central checksum provider and all have to do is is:
1. On cache set, get the current checksum for the provided cache tags and store them together with the list of cache tags.
2. On a cache get, get the current checksum for the stored tags and compare it, on mismatch, invalidate the cache item.
3. Notify the service about cache tags so that optimized implementations (like the one we have now) that do not repeatedly invalidate the same cache tag know that they have to increment the invalidations counter once more.

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

The following backends do that now: database, apcu, php.

Additionally, the confusing method "deleteTags()" is completely removed. It does not "delete tags", it just invalidates tags without the possibility to get the the stale cache item with $allow_invalid = TRUE. Only special cache backends could hope to have a better implementation that use the same as for cache tag invalidations, for others, it adds more complexity and developers do not understand when to use deleteTags() and invalidateTags()

Remaining tasks

- Agree on names for concepts/interfaces/class/service.
- Update documentation
- Reviews

User interface changes

-

API changes

- deleteTags() is completely removed
- invalidateTags() removed from CacheBackendInterface (although specific implementations may still have it)
- A new cache tags invalidator service, Cache::invalidateTags() still works but is now a simple wrapper around the service, so it is possible to inject this into services if they want. Cache::invalidateTags() will not be deprecated/removed in Drupal 8.
- A new cache tags storage/checksums service that cache backends can use.

[Original report removed, as it had nothing left in common with current goals of this issue, it was started *before* cache tags existed and as alternative implementation]

Files: 
CommentFileSizeAuthor
#213 cache-tags-918538-213-interdiff.txt1.22 KBBerdir
#213 cache-tags-918538-213.patch121.03 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,390 pass(es). View
#205 cache-tags-918538-205-interdiff.txt1.28 KBBerdir
#205 cache-tags-918538-205.patch120.86 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,361 pass(es). View
#201 cache-tags-918538-201-interdiff.txt664 bytesBerdir
#201 cache-tags-918538-201.patch120.73 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,251 pass(es). View
#194 cache-tags-918538-194-interdiff.txt1.97 KBBerdir
#194 cache-tags-918538-194.patch120.4 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,679 pass(es). View
#190 cache-tags-918538-190-interdiff.txt17.57 KBBerdir
#190 cache-tags-918538-190.patch119.38 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#187 cache-tags-918538-187-interdiff.txt966 bytesBerdir
#187 cache-tags-918538-187.patch119.12 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,694 pass(es). View
#185 cache-tags-918538-185-interdiff.txt10.05 KBBerdir
#185 cache-tags-918538-185.patch119.12 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php. View
#183 cache-tags-918538-183-interdiff.txt2.29 KBBerdir
#183 cache-tags-918538-183.patch118.89 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,640 pass(es), 6 fail(s), and 116 exception(s). View
#181 cache-tags-918538-181-interdiff.txt25.62 KBBerdir
#181 cache-tags-918538-181.patch118.66 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#174 cache-tags-918538-174-interdiff.txt579 bytesBerdir
#174 cache-tags-918538-174.patch118.32 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,418 pass(es). View
#172 cache-tags-918538-172-interdiff.txt13.66 KBBerdir
#172 cache-tags-918538-172.patch118.59 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#164 cache-tags-918538-164-interdiff.txt1.39 KBBerdir
#164 cache-tags-918538-164.patch121.25 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,326 pass(es). View
#163 cache-tags-918538-161-interdiff.txt19 KBBerdir
#163 cache-tags-918538-161.patch121.07 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#161 cache-tags-918538-161-interdiff.txt19 KBBerdir
#161 cache-tags-918538-161.patch121.07 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#159 cache-tags-918538-159-interdiff.txt3.96 KBBerdir
#159 cache-tags-918538-159.patch119.2 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,200 pass(es). View
#156 cache-tags-918538-156-interdiff.txt1.62 KBBerdir
#156 cache-tags-918538-156.patch119.44 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,493 pass(es), 27 fail(s), and 1,553 exception(s). View
#154 cache-tags-918538-154-interdiff.txt67.18 KBBerdir
#154 cache-tags-918538-154.patch117.83 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#152 cache-tags-918538-151-interdiff.txt54.91 KBBerdir
#152 cache-tags-918538-151.patch112.15 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,019 pass(es). View
#148 cache-tags-918538-148-interdiff.txt17.93 KBBerdir
#148 cache-tags-918538-148.patch93.34 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,007 pass(es). View
#147 cache-tags-918538-146.patch84.15 KBBerdir
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,014 pass(es). View
#147 cache-tags-918538-146-interdiff.txt7.99 KBBerdir
#144 cache-tags-918538-144-interdiff.txt23.57 KBBerdir
#144 cache-tags-918538-144.patch76.15 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,994 pass(es), 14 fail(s), and 0 exception(s). View
#142 cache-tags-918538-142-interdiff.txt5.94 KBBerdir
#142 cache-tags-918538-140.patch55.26 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,179 pass(es), 61 fail(s), and 51 exception(s). View
#140 cache-tags-918538-140-interdiff.txt3.75 KBBerdir
#140 cache-tags-918538-140.patch52.92 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,610 pass(es), 476 fail(s), and 100 exception(s). View
#138 cache-tags-918538-138-interdiff.txt18.23 KBBerdir
#138 cache-tags-918538-138.patch54.76 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,583 pass(es), 533 fail(s), and 236 exception(s). View
#136 cache-tags-918538-136-interdiff.txt5 KBBerdir
#136 cache-tags-918538-136.patch37.29 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#134 cache-tags-918538-134-interdiff.txt6.93 KBBerdir
#134 cache-tags-918538-134.patch35.08 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#132 cache-tags-918538-132.patch38.3 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#106 918538_105-interdiff.txt12.4 KBBerdir
#106 918538_105.patch102.09 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,459 pass(es), 35 fail(s), and 65 exception(s). View

Comments

Status: Needs review » Needs work

The last submitted patch, drupal.cache-clear-hook.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
FAILED: [[SimpleTest]]: [MySQL] 14,813 pass(es), 355 fail(s), and 134,832 exception(es). View

The installer is really the last thing I care for here.

Status: Needs review » Needs work

The last submitted patch, drupal.cache-clear-hook.2.patch, failed testing.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
PASSED: [[SimpleTest]]: [MySQL] 24,826 pass(es). View
Damien Tournoud’s picture

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?

sun’s picture

First example: There are modules (like Mollom module) that invoke info hooks in other modules to collect available data of them, which may depend on information that is cached. Now, the collected and processed data may be cached by the module on its own, which quickly leads to a stale cache.

Taking #898360: Cache node types as a simple example, I'd like to be able to just do:

function mollom_cache_clear_alter($bin, $cid, $context) {
  if ($bin == 'cache' && $cid == 'node_types') {
    // Reset our cache of available forms.
    mollom_form_cache(TRUE);
  }
}

Of course, Mollom would probably rather expose that cache clear condition in its own info hook, so it does not hard-code some special caches, but anyway, doesn't matter for this issue.

Second example: Administration menu module implements client-side caching, but unless menu_rebuild() is invoked to entirely rebuild the menu, it's close to impossible to take all hooks and forms into account that should lead to an invalidation of the client-side cache. For instance, whenever 'cache_views' is cleared, a client should retrieve a new copy of the menu.

Damien Tournoud’s picture

Component: base system » cache system

Reassigning.

sun’s picture

Do the use-cases I outlined in #7 make sense? Or am I completely nuts? As explained in #924616: Make database cache leverage static cache by default, I'm really struggling very hard to make any sense of the caching nightmare we introduced for D7.

sun’s picture

Version: 7.x-dev » 8.x-dev

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

carlos8f’s picture

subscribing

beejeebus’s picture

subscribing

xjm’s picture

Tagging issues not yet using summary template.

catch’s picture

Priority: Major » Normal
Status: Needs review » Needs work

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.

sun’s picture

The only way to resolve this issue without a hook would be to ditch the info hook pattern entirely.

It is the info hook pattern that allows modules to "register" arbitrary new stuff and drop old stuff without any kind of notice.

There's no way for another module that implements its own caching to track these registration, configuration and data changes in a reliably way, except for attempting to listen to cache clears currently.

To clarify what that would mean in code for one of the examples in #7:

function node_type_save($type) {
  // ...
  entity_bundle_save('node', $bundle_info);
  // ...
  form_save('node_' . $type->type . '_form', $form_info);
}
function comment_entity_bundle_insert($entity_type, $bundle_info) {
  // ...
  entity_bundle_save('comment', $bundle_info);
  // ...
  form_save('comment_' . $entity_type . '_' . $bundle_info['bundle'] . '_form');
}

# ---------------------

function node_type_delete($type) {
  // ...
  entity_bundle_delete('node', $bundle_info);
  // ...
  form_delete('node_' . $type->type . '_form');
}
function comment_entity_bundle_delete($entity_type, $bundle_info) {
  // ...
  entity_bundle_delete('comment', $bundle_info);
  // ...
  form_delete('comment_' . $entity_type . '_' . $bundle_info['bundle'] . '_form');
}

The essential change: There is no hook_node_info() and also no hook_forms() anymore. There is also no hook_entity_info() anymore.

Somewhat related to this is #597280: Introduce form registry to fix various problems and increase security, in which I already proposed to introduce a proper form registry. But that still assumed an info hook based approach; whereas to also resolve this issue, it would have to be a CRUD based approach.

sun’s picture

Introducing a hook in the cache system would introduce another circular dependency.

Won't fix based on that argument...

Although I still don't have any idea how to resolve my use-cases without it.

A clarification on the second use-case might help to understand the scenario better:

  1. Administration menu caches its menu.
  2. The cached menu contains links (NOT menu links) to arbitrary configuration items/entities collected from all modules.
  3. These links to configuration items can (naturally) change or vanish.
  4. The only time when the cached menu is reliably flushed is when menu_rebuild() is invoked, but that only happens when the router is entirely rebuilt.
  5. It's also not possible to "listen" to menu tree rebuilds or saving of menu links, because the cached links to configuration items do not have any menu links in the first place.
  6. It's impossible to take all hooks and forms into account that should lead to an invalidation of the cache.

For example, whenever the Views cache is cleared, this may mean that a view was created, edited, or deleted. The view itself does not have a menu link. But admin_menu exposes a direct link to it in order to access it faster. Thus, its cache needs to be flushed whenever any of the dynamic items in the menu may have changed.

The immediate response to that is: Listen to CUD hooks instead of cache hooks.

However, admin_menu tries to provide a simple way for other modules to integrate with it; i.e., a single hook. Thus, asking for CRUD involvement immediately means a step from 1 hook to 4 hooks, as it either means:

function admin_menu_views_insert() {
  admin_menu_flush_caches();
}
function admin_menu_views_update() {
  admin_menu_flush_caches();
}
function admin_menu_views_delete() {
  admin_menu_flush_caches();
}

or respectively, it means to put module-specific code into other modules:

function views_save($view) {
  ...
  // Flush admin_menu cache, if installed.
  module_invoke('admin_menu', 'flush_caches');
}
function views_delete($view) {
  ...
  // Flush admin_menu cache, if installed.
  module_invoke('admin_menu', 'flush_caches');
}

Thus, the most simple solution, without enforcing additional functions, would be to enhance the already existing info-alike hook to additionally return cache IDs that are sort-of treated like "triggers"; e.g.:

function views_admin_menu_map() {
  $map = array(
    ...
  );
  $map['cache ids']['cache_views'] = array();
}
asteconn’s picture

sun,

would #636454: Cache tag support solve this issue in your use case?

sun’s picture

Cache tags would only really make a difference if a tag would be invalidated across all known cache bins.

However, that's not the case currently.

If that was the case, then it sounds like it might resolve the problem, but I'm not 100% sure. If I get it right, then admin_menu would have to add all the cumulative cache tags from all affected data structures to its own cache items. Thus, if any of the tags was invalidated, admin_menu's affected cache items would be invalidated, too.

But to make that possible, cache tags and their invalidation counts would have to be managed in a cache bin-agnostic storage, which would inherently mean that cache tags would have to be their own key/value store service.

cache_invalidate($tags);
         |
   CacheTagManager::invalidate($tags)
               |
               ----> UPDATE {cachetags} SET invalidations = invalidations + 1 WHERE tag IN (:tags)


======== ^^^ INVALIDATION ^^^ ===== vvv VALIDATION vvv ===============

CacheBackendInterface::getMultiple($cids)
         |
         ----------< SELECT * FROM {$bin} WHERE cid IN (:cids)
         |
   ::validTags($checksum, $tags)
         |
         x<--- CacheTagManager::checksumTags($items::$tags)
         |       |
         |       --< SELECT tag, invalidations FROM {cachetags} WHERE tag IN (:tags)
         |
      $items

...something like that.

sun’s picture

Title: Allow modules to react on cleared caches » Decouple cache tags from cache bins
Issue tags: +API clean-up

Trying to resurrect this. And clarifying the issue title.

By now, cache tags turned out to be the answer for invalidating caches in a multidimensional modular system. This allows modules to do two discrete things:

1) Cache data that depends on or is related to some other data, by adding/using the same tags that the original/originating module is using for its own cache entries.

2) Cache an aggregate of other data and auto-invalidate it when required, by cumulating all relevant cache tags for the aggregate cache item.

I'm very grateful for them to exist.

The only remaining problem right now is that cache tags are still bound to cache bins. (At minimum, storage-wise.)

I.e., as a module, I'm using cache bin X. But:

i) The data of the original/other module is potentially cached in another bin.

ii) The other bin may not use the same storage as mine.

iii) I only know about my own cache bin.

iv) Tagged cache items are only invalidated/deleted within a single cache bin.

In turn, what I'd like to see to happen for D8 is to decouple the cache_tags bin from the "actual" cache bins, so the cache tags are managed across all cache bins. The storage of cache_tags can still be swappable and thus be replaced with something else than SQL. The only part that matters is that there is a single + reliable list of tags that is central for all cache bins.

So, e.g., if I do cache()->invalidateTags(array('filter_formats' => TRUE)); then I should not have to worry about the actual cache bins that might be affected. Instead, the expectation is that invalidating or deleting a tag has an impact on all currently actively/registered cache bins/backends.

As a result, I think we should move the cache tags functionality from CacheBackendInterface to the CacheFactory.

What do you think?

carlos8f’s picture

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

catch’s picture

Status: Needs work » Postponed (maintainer needs more info)

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...

catch’s picture

Status: Postponed (maintainer needs more info) » Active

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.

catch’s picture

Priority: Normal » Major

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).

damiankloip’s picture

Assigned: sun » Unassigned
Status: Active » Needs review
FileSize
14.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,670 pass(es), 7 fail(s), and 2 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 918538-24.patch, failed testing.

catch’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
4.37 KB
15.91 KB
FAILED: [[SimpleTest]]: [MySQL] 59,010 pass(es), 1 fail(s), and 0 exception(s). View

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?

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -API clean-up

The last submitted patch, 918538-27.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 918538-27.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +API clean-up

The last submitted patch, 918538-27.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.28 KB
18.79 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, 918538-33.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
18.8 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -API clean-up

The last submitted patch, 918538_35.patch, failed testing.

slashrsm’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.38 KB
17.74 KB
FAILED: [[SimpleTest]]: [MySQL] 59,893 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 37: 918538_36.patch, failed testing.

damiankloip’s picture

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 :)

slashrsm’s picture

Status: Needs work » Needs review
FileSize
19.33 KB
FAILED: [[SimpleTest]]: [MySQL] 59,978 pass(es), 6 fail(s), and 0 exception(s). View
3.89 KB

@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.

Status: Needs review » Needs work

The last submitted patch, 40: 918538_40.patch, failed testing.

damiankloip’s picture

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.

catch’s picture

Priority: Major » Critical

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.

slashrsm’s picture

Assigned: Unassigned » slashrsm

Working on this...

damiankloip’s picture

@slashrsm, any joy with this?

slashrsm’s picture

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.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
37.64 KB
FAILED: [[SimpleTest]]: [MySQL] 7,821 pass(es), 832 fail(s), and 22,271 exception(s). View
20.7 KB

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

Status: Needs review » Needs work

The last submitted patch, 48: 918538_48.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
41.02 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
5.07 KB

Let's try again....

Status: Needs review » Needs work

The last submitted patch, 50: 918538_50.patch, failed testing.

The last submitted patch, 50: 918538_50.patch, failed testing.

slashrsm’s picture

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

The last submitted patch, 50: 918538_50.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
29.62 KB
69.42 KB
FAILED: [[SimpleTest]]: [MySQL] 62,610 pass(es), 55 fail(s), and 135 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 55: 918538_50.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
slashrsm’s picture

FileSize
71.08 KB
FAILED: [[SimpleTest]]: [MySQL] 54,537 pass(es), 246 fail(s), and 492 exception(s). View

Forgot to attach file.

Wim Leers’s picture

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

Status: Needs review » Needs work

The last submitted patch, 58: 918538_57.patch, failed testing.

The last submitted patch, 58: 918538_57.patch, failed testing.

slashrsm’s picture

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

slashrsm’s picture

FileSize
71.29 KB
FAILED: [[SimpleTest]]: [MySQL] 63,076 pass(es), 35 fail(s), and 116 exception(s). View
948 bytes
Berdir’s picture

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

slashrsm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 63: 918538_63.patch, failed testing.

The last submitted patch, 63: 918538_63.patch, failed testing.

slashrsm’s picture

@Berdir: DUBT stands for?

tim.plunkett’s picture

slashrsm’s picture

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?

slashrsm’s picture

Status: Needs work » Needs review
FileSize
72.76 KB
FAILED: [[SimpleTest]]: [MySQL] 64,275 pass(es), 44 fail(s), and 1,381 exception(s). View
45.93 KB
Damien Tournoud’s picture

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.

Damien Tournoud’s picture

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

Status: Needs review » Needs work

The last submitted patch, 71: 918538_71.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
74.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 918538_75.patch. Unable to apply patch. See the log in the details link for more information. View
9.01 KB

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.

Status: Needs review » Needs work

The last submitted patch, 75: 918538_75.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
73.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 918538_76.patch. Unable to apply patch. See the log in the details link for more information. View
slashrsm’s picture

Issue summary: View changes
Berdir’s picture

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

Status: Needs review » Needs work

The last submitted patch, 77: 918538_76.patch, failed testing.

Wim Leers’s picture

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.

damiankloip’s picture

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.

Berdir’s picture

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 :)

Wim Leers’s picture

Ok, was just trying to help :)

damiankloip’s picture

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

slashrsm’s picture

Status: Needs work » Needs review
FileSize
72.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 86: 918538_86.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
72.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,586 pass(es), 4 fail(s), and 2 exception(s). View
615 bytes

Missing use.

Status: Needs review » Needs work

The last submitted patch, 88: 918538_87.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
76.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,390 pass(es), 4 fail(s), and 0 exception(s). View
8.09 KB

Status: Needs review » Needs work

The last submitted patch, 90: 918538_90.patch, failed testing.

Wim Leers’s picture

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

martin107’s picture

Issue tags: +Needs reroll
Berdir’s picture

Assigned: slashrsm » Berdir

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

Berdir’s picture

Assigned: Berdir » Unassigned
Status: Needs work » Needs review
FileSize
92.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
21.22 KB

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

Status: Needs review » Needs work

The last submitted patch, 95: 918538_95.patch, failed testing.

Berdir’s picture

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

The last submitted patch, 95: 918538_95.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
93.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,424 pass(es), 37 fail(s), and 75 exception(s). View
1.76 KB

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

Status: Needs review » Needs work

The last submitted patch, 99: 918538_99.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
96.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 18,315 pass(es), 894 fail(s), and 873 exception(s). View
16.85 KB

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.

Status: Needs review » Needs work

The last submitted patch, 101: 918538_101.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
95.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 18,318 pass(es), 893 fail(s), and 870 exception(s). View
10.89 KB

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

Status: Needs review » Needs work

The last submitted patch, 103: 918538_103.patch, failed testing.

damiankloip’s picture

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...

Berdir’s picture

Status: Needs work » Needs review
FileSize
102.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,459 pass(es), 35 fail(s), and 65 exception(s). View
12.4 KB

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.

Berdir’s picture

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.

znerol’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 106: 918538_105.patch, failed testing.

Berdir’s picture

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 ;)

znerol’s picture

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)?

Berdir’s picture

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.

catch’s picture

#111 sounds great.

damiankloip’s picture

Assigned: Unassigned » damiankloip

Going to do some work on this one.

slashrsm’s picture

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.

andypost’s picture

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

slashrsm’s picture

It turns out summary already describes cache tag service approach.

Wim Leers’s picture

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.

slashrsm’s picture

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

Berdir’s picture

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.

catch’s picture

Issue tags: +D8 upgrade path

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.

chx’s picture

Issue summary: View changes
ksenzee’s picture

Assigned: damiankloip » ksenzee
Issue tags: -Needs issue summary update

Working on this for now.

catch’s picture

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.

damiankloip’s picture

ksenzee, is anything going on with this?

Fabianx’s picture

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

damiankloip’s picture

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

Fabianx’s picture

xjm’s picture

Issue tags: +Triaged D8 critical
catch’s picture

Issue summary: View changes
Berdir’s picture

Assigned: ksenzee » Berdir
Issue tags: +Ghent DA sprint

Working a bit on this.

Berdir’s picture

Status: Needs work » Needs review
FileSize
38.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

Status: Needs review » Needs work

The last submitted patch, 132: cache-tags-918538-132.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
35.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
6.93 KB

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

Status: Needs review » Needs work

The last submitted patch, 134: cache-tags-918538-134.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
37.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
5 KB

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

Status: Needs review » Needs work

The last submitted patch, 136: cache-tags-918538-136.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
54.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,583 pass(es), 533 fail(s), and 236 exception(s). View
18.23 KB

Fixing the unit test fatals to get more test feedback.

Still very unsure about a lot of stuff.

Status: Needs review » Needs work

The last submitted patch, 138: cache-tags-918538-138.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
52.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,610 pass(es), 476 fail(s), and 100 exception(s). View
3.75 KB

I might need to switch to a test issue soon.

Status: Needs review » Needs work

The last submitted patch, 140: cache-tags-918538-140.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
55.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,179 pass(es), 61 fail(s), and 51 exception(s). View
5.94 KB

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

Status: Needs review » Needs work

The last submitted patch, 142: cache-tags-918538-140.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
76.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,994 pass(es), 14 fail(s), and 0 exception(s). View
23.57 KB

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...

Status: Needs review » Needs work

The last submitted patch, 144: cache-tags-918538-144.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

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

Berdir’s picture

FileSize
7.99 KB
84.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,014 pass(es). View

Now with 100% more patches.

Berdir’s picture

FileSize
93.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,007 pass(es). View
17.93 KB

drupal_static()'s, begone!

Wim Leers’s picture

Issue summary: View changes

Clarification in the IS.

Wim Leers’s picture

  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.

catch’s picture

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.

Berdir’s picture

FileSize
112.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,019 pass(es). View
54.91 KB

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.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +DX (Developer Experience)

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 :)

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Needs profiling
FileSize
117.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
67.18 KB

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

Status: Needs review » Needs work

The last submitted patch, 154: cache-tags-918538-154.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
119.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,493 pass(es), 27 fail(s), and 1,553 exception(s). View
1.62 KB

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.

Wim Leers’s picture

Wow, huge progress! Will review.

Status: Needs review » Needs work

The last submitted patch, 156: cache-tags-918538-156.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
119.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,200 pass(es). View
3.96 KB

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.

Wim Leers’s picture

Status: Needs review » Needs work

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?

Berdir’s picture

Status: Needs work » Needs review
FileSize
121.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
19 KB

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.

Status: Needs review » Needs work

The last submitted patch, 161: cache-tags-918538-161.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
121.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
19 KB

I thought you reviewed this :p

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

Berdir’s picture

FileSize
121.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,326 pass(es). View
1.39 KB

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

The last submitted patch, 163: cache-tags-918538-161.patch, failed testing.

damiankloip’s picture

Status: Needs review » Needs work

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

Wim Leers’s picture

#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/

Fabianx’s picture

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.

Fabianx’s picture

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.

Fabianx’s picture

+++ 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.

znerol’s picture

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?

Berdir’s picture

Status: Needs work » Needs review
FileSize
118.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
13.66 KB

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.

Status: Needs review » Needs work

The last submitted patch, 172: cache-tags-918538-172.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
118.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,418 pass(es). View
579 bytes

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

Berdir’s picture

#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.

catch’s picture

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.

Berdir’s picture

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)?

Berdir’s picture

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.

pounard’s picture

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.

catch’s picture

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.

Berdir’s picture

FileSize
118.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
25.62 KB

Ok, renamed to CacheTagsChecksumInterface, DatabaseCacheTagsChecksum, @cache_tags.checksum and checksumProvider in the backends.

Status: Needs review » Needs work

The last submitted patch, 181: cache-tags-918538-181.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
118.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,640 pass(es), 6 fail(s), and 116 exception(s). View
2.29 KB

Would help if I wouldn't upload a completely broken implementation.

Status: Needs review » Needs work

The last submitted patch, 183: cache-tags-918538-183.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
119.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php. View
10.05 KB

Fixing those test fails and renamed the service to cache_tags.invalidator.

Status: Needs review » Needs work

The last submitted patch, 185: cache-tags-918538-185.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
119.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,694 pass(es). View
966 bytes

Grml.

Berdir’s picture

Assigned: Berdir » Unassigned

Green, needs reviews, unassigning.

dawehner’s picture

  1. +++ b/core/core.services.yml
    @@ -32,6 +32,17 @@ services:
    +    calls:
    +      - [setContainer, ['@service_container']]
    

    We could use the container.trait parent here ...

  2. +++ b/core/core.services.yml
    @@ -32,6 +32,17 @@ services:
    +  cache_tags.checksum:
    

    In case we would prefix the service name with 'validator' it would be a bit more clear, to what this belongs to, so what about using cache_tags.invalidator.checksum ?

  3. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
    @@ -10,7 +10,7 @@
    +class ApcuBackend implements CacheBackendInterface, CacheTagsInvalidatorInterface {
    
    @@ -45,20 +45,18 @@ class ApcuBackend implements CacheBackendInterface {
       protected $invalidationsTagsPrefix;
    

    While reading the code I was wondering why we have code in ApcuBackend::invalidateTags() but never access the stored counter?

  4. +++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
    @@ -189,16 +183,17 @@ protected function prepareItem($cache, $allow_invalid) {
    +    if ($tags) {
    +      $tags = array_unique($tags);
    +      Cache::validateTags($tags);
    +    }
    +
    

    Is there a particular reason we don't a) skip the check and b) do the unique call in validateTags itself? It is just a little bit easier to understand what is going on when you have 1 vs. 4 lines.

    This could be for sure done in a follow up as well.

    Did we also thought about including the sort() which is part of the db backend in to the service itself.

  5. +++ b/core/lib/Drupal/Core/Cache/CacheCollector.php
    @@ -280,7 +280,7 @@ public function reset() {
    -      Cache::deleteTags($this->tags);
    +      Cache::invalidateTags($this->tags);
    

    I try to be curious here ... do we recommend people to inject the cache_tags.invalidator service on the longrun? Maybe we can open up a new follow up to clear up those usecases?

  6. +++ b/core/lib/Drupal/Core/Cache/CacheTagsChecksumInterface.php
    @@ -0,0 +1,55 @@
    + * Cache backends can use this to check if any cache tag invalidations
    

    This sentence sounds like someone waiting on simpletest, went onto reddit on accident, and forgot about what was actually the main topic at the moment :) I'll assume that this will document how to use it. Did you considered adapting the name to something like CacheTagsChecksumProviderInterface maybe? The current name does not tell enough maybe, what this is doing.

  7. +++ b/core/lib/Drupal/Core/Cache/CacheTagsChecksumInterface.php
    @@ -0,0 +1,55 @@
    +  /**
    +   * Reset statically cached tags.
    +   *
    +   * This is only used by tests.
    +   */
    +  public function reset();
    

    Maybe a bit of a provocative question (and I know we used to do that already in many places): When we know its only used for tests purposes, does it have to be part of the interface? You can still add the method in case you need to write tests against your congrete implementation. This is not a blocker at all, just wanted to throw out some thoughts.

  8. +++ b/core/lib/Drupal/Core/Cache/CacheTagsInvalidatorInterface.php
    @@ -0,0 +1,25 @@
    +/**
    + * Defines required methods for classes wanting to handle cache tag changes.
    + *
    + * @ingroup cache
    + */
    +interface CacheTagsInvalidatorInterface {
    

    Do you think it would help if we explain here, that cache backends can implement the interface in case they want to deal with invalidation for themselves.

  9. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsChecksum.php
    @@ -0,0 +1,205 @@
    +    // Remove tags that were already invalidated during this request from the
    +    // static caches so that another deletion or invalidation can occur.
    

    You probably went through this already, but can we document why we have to allow multiple invalidations per request? Note: We can drop the notion of 'deletion' here.

  10. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsChecksum.php
    @@ -0,0 +1,205 @@
    +    return $checksum == $this->calculateChecksum($tags);
    

    should we use === here?

  11. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -621,7 +621,7 @@ protected function resetSystem() {
    diff --git a/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    
    diff --git a/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    index 9b0eb65..d18a8c7 100644
    
    index 9b0eb65..d18a8c7 100644
    --- a/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    
    --- a/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    
    +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    +++ b/core/lib/Drupal/Core/Field/FieldStorageDefinitionInterface.php
    @@ -8,6 +8,7 @@
    
    @@ -8,6 +8,7 @@
     namespace Drupal\Core\Field;
     
     use Drupal\Core\Entity\FieldableEntityInterface;
    +use Drupal\Core\TypedData\DataDefinitionInterface;
     
     /**
      * Defines an interface for entity field storage definitions.
    

    Unrelated change.

  12. +++ b/core/modules/system/src/Tests/Cache/ApcuBackendUnitTest.php
    @@ -34,7 +34,8 @@ protected function checkRequirements() {
    +    $backend = new ApcuBackend($bin, $this->databasePrefix, \Drupal::service('cache_tags.checksum'));
    +    return $backend;
    

    Feel free to get rid of the assignment.

  13. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheTagsInvalidatorTest.php
    @@ -0,0 +1,69 @@
    +
    +
    

    Nitpick: 2 empty lines.

  14. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheTagsInvalidatorTest.php
    @@ -0,0 +1,69 @@
    +    $cache_tags_invalidator->invalidateTags(['node' => [2, 3, 5, 8, 13]]);
    

    Nice numbers!

  15. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheTagsInvalidatorTest.php
    @@ -0,0 +1,69 @@
    +    $non_invalidator_cache_bin = $this->getMock('\Drupal\Core\Cache\CacheBackendI');
    

    ... is it really intented that we point to a non-existing class? the "nterface" of "Interface" is missing here.

  16. +++ b/core/tests/Drupal/Tests/Core/Cache/CacheTagsInvalidatorTest.php
    @@ -0,0 +1,69 @@
    +    $container = new Container();
    

    Oh interesting: I always thought you have to use the ContainerBuilder, but it fact you don't have to.

  17. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -193,14 +201,14 @@ protected function setUpEntityManager($definitions = array()) {
       public function testClearCachedDefinitions() {
         $this->setUpEntityManager();
    -    $this->cache->expects($this->at(0))
    -      ->method('deleteTags')
    +    $this->cacheTagsInvalidator->expects($this->at(0))
    +      ->method('invalidateTags')
           ->with(array('entity_types'));
    -    $this->cache->expects($this->at(1))
    -      ->method('deleteTags')
    +    $this->cacheTagsInvalidator->expects($this->at(1))
    +      ->method('invalidateTags')
           ->with(array('entity_bundles'));
    -    $this->cache->expects($this->at(2))
    -      ->method('deleteTags')
    +    $this->cacheTagsInvalidator->expects($this->at(2))
    +      ->method('invalidateTags')
           ->with(array('entity_field_info'));
     
    

    Next time you have to do something like this: Use ->withConsecutive([['entity_types'], ['entity_bundles'], ['entity_field_info']])

  18. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -859,30 +867,30 @@ public function testGetAllBundleInfo() {
    +    $this->cacheBackend->expects($this->at(4))
           ->method('get')
           ->with("entity_bundle_info:en", FALSE)
           ->will($this->returnValue((object) array('data' => 'cached data')));
    

    Feel free to create a follow up: For readability reasons it would be great to move the cacheBackend up to the other cacheBackend once.

Berdir’s picture

FileSize
119.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
17.57 KB

Thanks for the review.

1. We could, changed ;)
2. Works for me, renamed.
3. Yeah, that's left-over code, removed.
4. There used to be an additional call there. I reverted the changes that I could so they don't distract :) I think the suggested changes don't really belong here.
5. I'm not sure on that part. We could, but for example, injecting it into plugin manager would break all subclasses/contrib plugin managers again for no good reason.. 9.x I guess, or new/standalone services...
6. It probably was netflix but close ;) Updated the documentation. Yes, I considered including Provider, read from comment #168, @catch in #180 preferred to not include Provider.
7. I'm not sure, it is not something we use for testing the implementation, it is used for web tests, like a bunch of other reset methods too, see WebTestBase::refreshVariables(), I'm replacing drupal_static_reset()'s with a reset() method.
8. Like this.
9. I tried, is this any clearer?
10. I'm not sure. At least right now, e.g. the database backend is passing in strings because it is loaded from the database, we would have to explicitly cast to (int) in the caller (because doing a type safe check and cast it in the implementation would be pointless).
11. Grrrr. Yet another left-over from Ghent discussion :p
12. Removed, was needed in an older version.
13. Fixed.
15. No idea why that doesn't throw errors. Fixed, should be the interface.
16. Yes, using the builder is actually pointless :)

dawehner’s picture

4. There used to be an additional call there. I reverted the changes that I could so they don't distract :) I think the suggested changes don't really belong here.

Agreed

9. I tried, is this any clearer?

That is indeed better.

10. I'm not sure. At least right now, e.g. the database backend is passing in strings because it is loaded from the database, we would have to explicitly cast to (int) in the caller (because doing a type safe check and cast it in the implementation would be pointless).

Ah okay no big deal here.

In other words, +1

jhodgdon’s picture

Status: Needs review » Needs work

@Berdir asked me to take a look at this patch and its documentation. Overall I think it looks great, and I think the issue summary makes total sense. The code and documentation seem very clear, and I think the separation between the various classes/services is very clean.

Just to be clear, all I read on this issue is the summary and ... well at least most of the latest patch (I skipped reading the tests and may not have read the code too carefully in a few places), so apologies if I've missed discussions that weren't part of the summary.

I do have a few questions:

(a) In CacheTagsInvalidator::getInvalidatorCacheBins(), I don't think I understand why this is not done once and saved in a member variable the first call, then retrieved from the member variable after that for subsequent calls? It seems like this is being calculated every time there is an invalidateTags() call, which seems inefficient. I may not be understanding what this class is actually used for though?

(b) In the existing core.api.php in the cache tags section:

 *
 * @todo Update cache tag deletion in https://drupal.org/node/918538

This needs to be removed.

(c) Also in core.api.php in the cache tags section, there is text where it talks about deleting tags, and I think now all you can do is invalidate them, right? Here's the text:

...
 * Data that has been tagged can be deleted or invalidated as a group: no matter
 * the Cache ID (cid) of the cache item, no matter in which cache bin a cache
 * item lives; as long as it is tagged with a certain cache tag, it will be
 * deleted or invalidated.
...

I think the rest of core.api.php text is fine as it is.

The last submitted patch, 190: cache-tags-918538-190.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
120.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,679 pass(es). View
1.97 KB

Thanks for the review.

a) I'm not sure about storing that information. This shouldn't be called tha often (except from the installer), and theoretically, something could change e.g. during tests. It would probably be ok, but I don't think it would make much of a difference.

b) & c) should be fixed.

Also fixed the (again) broken installer.

Status: Needs review » Needs work

The last submitted patch, 194: cache-tags-918538-194.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

That is a random fail: #2398259: Random test fail in AccessRoleTest

Prepared a change record for this issue: https://www.drupal.org/node/2398255

jhodgdon’s picture

Regarding the cache bins calculation, it's called for each call to invalidateTags()... Does invalidateTags() typically only happen once during an entity save operation (or other requests?), or are there multiple calls that typically need to be invalidated, during different steps in a request?

It just seems like a fairly expensive calculation... and is the 'cache_bins' parameter of the Container generally changed during a request (or ever)? Because that's what it's iterating over:

+    foreach ($this->container->getParameter('cache_bins') as $service_id => $bin) {
+      $service = $this->container->get($service_id);

Docs changes look good!

Berdir’s picture

Yes, it usually only happens on write operations that change things that are cached, typically config or entities. The parameter usually doesn't change as that is based on services, but it is theoretically possible that difference backends are returned.

And no, it's not that expensive. If anything,then it is only expensive once, after that, the services are initialized and 10 calls to the container aren't going to make any difference at all.

catch’s picture

Overall this looks great.

Two nitpicks was all I found so far:

  1. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsChecksum.php
    @@ -0,0 +1,207 @@
    +   * A list of tags that have already been invalidated in this requested.
    

    s/requested/request.

  2. +++ b/core/lib/Drupal/Core/Cache/DatabaseCacheTagsChecksum.php
    @@ -0,0 +1,207 @@
    +          $this->catchException($e);
    

    catchException() contains the check that's already in ensureTableExists(), however we also currently can't rethrow this exception on ensureTableExists() in the case that the table already does exist (right now it just returns FALSE). Doesn't matter that much and not sure how to improve it, but it shows up as a bit odd.

Berdir’s picture

FileSize
120.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,251 pass(es). View
664 bytes

Thanks, rerolled, 1. fixed, not sure I understand the second point.

catch’s picture

So in ensureTableExists() we do this check:

 if (!$database_schema->tableExists('cachetags')) {
    // snip...
   return TRUE;
}
// snip...
return FALSE;

Then again in catchException:

+    if ($this->connection->schema()->tableExists('cachetags')) {

So if there's an exception, and the table already exists, then we have to check the existence of the table twice before rethrowing the exception. It'd be more efficient for ensureTableExists() to flag that yes, the table does actually exist already, so that catchException() becomes unnecessary. But I can't immediately see a clean way to do that, and wouldn't hold the patch up on it.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks great, has undergone a nice amount of scrutiny, CR looks great, docs have been reviewed, and I also can't see a clean way to do #202. Hence: RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Couple of minor things - looks great. Very happy to see the statics introduced to makes tests pass be replaced by a service.

+++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -45,20 +45,18 @@ class ApcuBackend implements CacheBackendInterface {
   protected $invalidationsTagsPrefix;

@@ -67,13 +65,15 @@ class ApcuBackend implements CacheBackendInterface {
     $this->invalidationsTagsPrefix = $this->sitePrefix . '::itags::';

Not needed anymore afaics

+++ b/core/lib/Drupal/Core/Config/CachedStorage.php
@@ -131,7 +131,7 @@ public function write($name, array $data) {
-      Cache::deleteTags(array($this::FIND_BY_PREFIX_CACHE_TAG));
+      Cache::invalidateTags(array($this::FIND_BY_PREFIX_CACHE_TAG));

@@ -146,7 +146,7 @@ public function delete($name) {
-      Cache::deleteTags(array($this::FIND_BY_PREFIX_CACHE_TAG));
+      Cache::invalidateTags(array($this::FIND_BY_PREFIX_CACHE_TAG));

@@ -162,7 +162,7 @@ public function rename($name, $new_name) {
-      Cache::deleteTags(array($this::FIND_BY_PREFIX_CACHE_TAG));
+      Cache::invalidateTags(array($this::FIND_BY_PREFIX_CACHE_TAG));

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -346,7 +347,7 @@ public function setInfo(array $tokens) {
+    Cache::invalidateTags(array(

+++ b/core/modules/book/src/BookManager.php
@@ -433,7 +433,7 @@ public function deleteFromBook($nid) {
+    Cache::invalidateTags(array('bid:' . $original['bid']));

@@ -763,7 +763,7 @@ public function saveBookLink(array $link, $new) {
+    Cache::invalidateTags($cache_tags);

+++ b/core/modules/views/src/ViewsData.php
@@ -322,6 +322,6 @@ public function clear() {
-    Cache::deleteTags(array('views_data'));
+    Cache::invalidateTags(array('views_data'));

Can we can a followup created to inject the service instead of calling out to the static method. No need to make this patch any bigger though.

Berdir’s picture

Status: Needs work » Reviewed & tested by the community
Related issues: +#2398085: Inject cache_tags.invalidator instead of using Cache::invalidateTags()
FileSize
120.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,361 pass(es). View
1.28 KB

Removed dead code from ApcuBackend.

The follow-up for injecting is #2398085: Inject cache_tags.invalidator instead of using Cache::invalidateTags(), we forgot to link that back to here.

alexpott’s picture

Assigned: Unassigned » catch

Assigning to catch since he's been very involved with this topic.

catch’s picture

+++ b/core/lib/Drupal/Core/Cache/CacheTagsChecksumInterface.php
@@ -0,0 +1,62 @@
+
+  /**
+   * Returns the sum total of validations for a given set of tags.
+   *
+   * Called by a backend when storing a cache item.
+   *
+   * @param string[] $tags
+   *   Array of cache tags.
+   *
+   * @return int
+   *   Cache tag invalidations checksum.
+   */
+  public function getCurrentChecksum(array $tags);
+

Only thing here is I'm not sure we should restrict this to an int - we probably want to allow it as a string, so that we can check hashes of invalidations rather than a numeric sum. This came up a couple of times on irc but don't think there's an issue, so opened #2401929: Cache tag checksums can be equal for different sets of checksums.

Don't have anything else though.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Berdir’s picture

Hm, interesting point, but we have to restrict it to *something*? This then needs to be stored, which e.g. for the database backend, is a separate column, so we can't just define it as mixed and then in 8.1 decide to use a string instead of an integer?

I think changing it to anything else than integer is not in the scope of this issue, so we should document it as that and change it if we decide to return string hashes or something else?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

+1 to #209. It has served us fine so far, no need to start changing that detail (relatively speaking) in this already big issue/discussion/patch. To continue that discussion, catch already opened #2401929: Cache tag checksums can be equal for different sets of checksums.

Moving this back to RTBC.

catch’s picture

Hmm so the reason I brought it up here was because previously what gets stored hasn't been part of the API at all, but was just an implementation detail.

This patch brings the fact it's a string or integer into the API (only as a return value but still). I was wondering if we just define it as a string, then it'd be a bug that the database uses an int as storage compared to varchar, but no API change later if we change it.

Berdir’s picture

We discussed to change the API and storage to string but not doing anything about the implementation. With the idea that we could change the implementation without having to change the API/storage.

But after having a look, I'm not sure that makes sense. The checksum provider stores 'invalidations', changing that to a string would not make sense, nor we would we actually store a string there, we would store a 'last_invalidated' timestamp...

Berdir’s picture

FileSize
121.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,390 pass(es). View
1.22 KB

Ok, changed just the API and the storage of database cache bins. Then we just have to change the cache_tags table if we want to change this...

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

Didn't want to keep this at RTBC.

catch’s picture

Assigned: catch » Unassigned
Status: Needs review » Reviewed & tested by the community

Thanks. Sorry that was so last minute, I didn't think about the API change until the very last look through.

Back to RTBC, will commit today.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 503e46b on 8.0.x
    Issue #918538 by Berdir, slashrsm, damiankloip, sun, tobiasb: Decouple...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

podarok’s picture

This patch brakes upgrade path from beta4
Unknown column 'checksum' in 'field list': INSERT INTO {cache_config}

Here is a manual upgrade path

ALTER TABLE `cache_config`
ADD `checksum` varchar(255) NULL COMMENT 'The tag invalidation checksum when this entry was saved.',
COMMENT='Storage for the cache API.'; 


ALTER TABLE `cache_bootstrap`
ADD `checksum` varchar(255) NULL COMMENT 'The tag invalidation checksum when this entry was saved.',
COMMENT='Storage for the cache API.'; 


ALTER TABLE `cache_discovery`
ADD `checksum` varchar(255) NULL COMMENT 'The tag invalidation checksum when this entry was saved.',
COMMENT='Storage for the cache API.';


ALTER TABLE `cache_default`
ADD `checksum` varchar(255) NULL COMMENT 'The tag invalidation checksum when this entry was saved.',
COMMENT='Storage for the cache API.'; 

ALTER TABLE `cache_data`
ADD `checksum` varchar(255) NULL COMMENT 'The tag invalidation checksum when this entry was saved.',
COMMENT='Storage for the cache API.';

ALTER TABLE `cache_entity`
ADD `checksum` varchar(255) NULL COMMENT 'The tag invalidation checksum when this entry was saved.',
COMMENT='Storage for the cache API.';

ALTER TABLE `cache_render`
ADD `checksum` varchar(255) NULL COMMENT 'The tag invalidation checksum when this entry was saved.',
COMMENT='Storage for the cache API.';


ALTER TABLE `cache_menu`
ADD `checksum` varchar(255) NULL COMMENT 'The tag invalidation checksum when this entry was saved.',
COMMENT='Storage for the cache API.';


ALTER TABLE `cache_toolbar`
ADD `checksum` varchar(255) NULL COMMENT 'The tag invalidation checksum when this entry was saved.',
COMMENT='Storage for the cache API.';


Berdir’s picture

Yes, there is no upgrade path yet.

You can just drop the cache tables, but there are many other changes as well, this is just the first one that you'll notice.