Updated: Comment #0

Problem/Motivation

Most content entity types provide cache tags. Most config entity types don't. Overall, it's currently more like the wild west, where everybody does whatever one feels like. But we really need predictable, reliable cache tags for entities.

Proposed resolution

  1. Add EntityInterface::getCacheTag(), which is responsible for generating the cache tag for a unique entity. (Used for invalidating caches when updating or deleting an entity.)
  2. Add EntityInterface::getListCacheTags(), which is responsible for generating the cache tag(s) for a list of entities. (Used for invalidating caches when creating, updating or deleting an entity, so that the new entity may show up immediately, and the updated entity may now meet the listing's filtering requirements, e.g. for Views).
  3. Have the Entity base class implement those methods, so that all entities in Drupal core get them for free.
  4. Have the Entity base class implement postSave() and postDelete(), to ensure that the aforementioned cache tags are invalidated when appropriate.
  5. Have the ContentEntityBase & ConfigEntityBase classes also implement cache tag invalidation for other methods where that is necessary: for now only ConfigEntityBase::disable().
  6. Add EntityViewBuilderInterface::getCacheTag(), which is responsible for generating the cache tag for an entity view of the entity type that entity view builder is for.
  7. Have the EntityViewBuilder and BlockViewBuilder classes implement and use that method.
  8. PHPUnit test coverage for that default cache tag invalidation. (Gratefully building on top of #2134857: PHPUnit test the entity base classes.)

This patch would provide the assurance that for each entity type, a proper cache tag exists and that if you inherit from the recommended base classes, cache tag invalidation already works. Together with #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags (which provides integration test coverage for the most visible content entity types), we can have peace of mind that Drupal 8's entity cache tags are comprehensive.
The only potential addition that might be needed is to have >=1 simple integration test per entity type to ensure that the proper cache tags are also effectively being set in the render process. Most config entities are not rendered directly, but are used as part of another rendering process, e.g. the image_style config entity is used in the image field's formatter.

Remaining tasks

  1. Get blocker committed (again): #2134857: PHPUnit test the entity base classes
  2. Get patch green
  3. Build consensus
  4. Get committed

User interface changes

None.

API changes

  1. EntityInterface::getCacheTag()
  2. EntityInterface::getListCacheTags()

Postponed until

#2134857: PHPUnit test the entity base classes

CommentFileSizeAuthor
#55 interdiff.txt932 bytesWim Leers
#55 entity_base_cache_tags-2217749-55.patch39.25 KBWim Leers
#50 entity_base_cache_tags-2217749-50.patch39.17 KBWim Leers
#47 interdiff.txt828 bytesWim Leers
#47 entity_base_cache_tags-2217749-46.patch39.2 KBWim Leers
#45 interdiff-28-45.txt6.45 KBWim Leers
#45 entity_base_cache_tags-2217749-45.patch39.2 KBWim Leers
#42 interdiff-40-42.txt648 bytesJalandhar
#42 entity_base_cache_tags-2217749-42.patch32.62 KBJalandhar
#40 interdiff-38-40.txt2.12 KBJalandhar
#40 entity_base_cache_tags-2217749-40.patch32.61 KBJalandhar
#38 entity_base_cache_tags-2217749-38.patch32.64 KBvisabhishek
#28 interdiff.txt7.28 KBWim Leers
#28 entity_base_cache_tags-2217749-28.patch33.86 KBWim Leers
#21 interdiff.txt3.89 KBWim Leers
#21 entity_base_cache_tags-2217749-21.patch26.63 KBWim Leers
#19 entity_base_cache_tags-2217749-19.patch25.78 KBWim Leers
#17 interdiff.txt7.54 KBWim Leers
#17 entity_base_cache_tags-2217749-17.patch26.08 KBWim Leers
#14 interdiff.txt3.13 KBWim Leers
#14 entity_base_cache_tags-2217749-14.patch21.49 KBWim Leers
#13 interdiff.txt738 bytesWim Leers
#13 entity_base_cache_tags-2217749-13.patch21.46 KBWim Leers
#12 8597055-12-ViewUI.txt1.42 KBdamiankloip
#11 interdiff.txt3.25 KBWim Leers
#11 entity_base_cache_tags-2217749-11.patch20.79 KBWim Leers
#10 interdiff.txt1.6 KBWim Leers
#10 entity_base_cache_tags-2217749-10.patch20.39 KBWim Leers
#9 entity_base_cache_tags-2217749-9.patch21.61 KBWim Leers
#2 interdiff-phase-3-entityinterface_cache_tags_methods.txt3.21 KBWim Leers
#2 interdiff-phase-2-generalized_to_entity_base.txt8.19 KBWim Leers
#2 interdiff-phase-1-configentitybase_only.txt15.65 KBWim Leers
#2 entity_base_cache_tags-2217749-2.patch22.7 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Category: Bug report » Task

This should be a task, not a bug report. This must happen before beta, because it's likely that developers will start to implement entity types in beta. This is an API addition that will significantly help contrib module entity types deal with cacheability and therefore performance. Hence marked as "beta blocker".

Besides that aspect, this is a blocker for the critical #2124957: Replace 'content' cache tag with 'rendered' and use it sparingly. Hence marked as "critical".

Wim Leers’s picture

Patch!

Unfortunately postponed until #2134857: PHPUnit test the entity base classes is committed.

I've attached three interdiffs that show the different stages of how I got to this patch. At first it was just about solving #2204159: (Responsive) Image styles do not add correct cache tags, nor do they invalidate cache tags upon flushing generically (i.e. for all config entities at once), but then I realized I might as well do it for *all* entities.

xjm’s picture

Is #2134857: PHPUnit test the entity base classes really a hard blocker for this issue? We don't generally block criticals on normal refactorings, and that particular issue is probably going to take awhile because of the testbot problem.

andypost’s picture

Suppose this could help ER fields properly clean their cache

Wim Leers’s picture

#3: Yes, it's a hard blocker, because it builds on that test coverage. I was partially inspired to do things this way because that patch had landed.

#4: "ER"? Entity Reference? If so: yes, because an ER field will be able to ask the entities it references what their (uniquely identifying) cache tags are :)

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

jessebeach’s picture

Issue summary: View changes
jessebeach’s picture

Title: Entity base class should provide standardized cache tags and built-in invalidation » [PP-1] Entity base class should provide standardized cache tags and built-in invalidation
Wim Leers’s picture

#2134857: PHPUnit test the entity base classes will likely be green again today and will hopefully be committed very soon.

This is a reroll on top of current HEAD + #2134857.

Wim Leers’s picture

FileSize
20.39 KB
1.6 KB

I found a silly piece of dead code that I should've removed in the "phase 2 interdiff" in #2.

Wim Leers’s picture

Issue summary: View changes
FileSize
20.79 KB
3.25 KB

In working with damiankloip on bringing full cache tag support to Views (see #2222847: Simplify Views cache tags to just a list tag per entity type a.o.), it quickly became clear that our assumption so far in dealing with "list" cache tags was wrong.

So far, this is the purpose we assigned to list cache tags:

Used for invalidating caches when creating an entity, so that the new entity may show up immediately, e.g. for Views

But, that's incomplete/insufficient. List cache tags must serve another purpose as well: if an entity is updated, due to a listing's sorting and/or filtering, an entity may start to appear in the listing, stop to appear in the listing, or appear in a different location in the listing. The "stopping to appear" and "appear in a different location" parts would be covered by the "own" cache tag. But the "start to appear in the listing" part wouldn't be. Therefore, the "list" cache tag must not only be invalidated upon entity creation, but also upon updating an entity!
So, updated the purpose:

Used for invalidating caches when creating or updating an entity, so that the new entity may show up immediately, and the updated entity may now meet the listing's filtering requirements, e.g. for Views

damiankloip’s picture

FileSize
1.42 KB

This is what needs to happen to ViewUI.

EDIT: Yes, forget the second hunk. That needs to go into the reroll for #2134857: PHPUnit test the entity base classes.

Wim Leers’s picture

FileSize
21.46 KB
738 bytes

After posting #11, I noticed a single failure when running the entire PHPUnit suite: ViewUI implements ConfigEntityInterface but does not inherit from any entity base class. Therefore it didn't conform to the interface yet. #12 is the solution. The second of hunk in #12 is unrelated to this issue.

Wim Leers’s picture

Title: [PP-1] Entity base class should provide standardized cache tags and built-in invalidation » Entity base class should provide standardized cache tags and built-in invalidation
Status: Postponed » Needs review
FileSize
21.49 KB
3.13 KB

#2134857: PHPUnit test the entity base classes landed, which unblocks this patch!

Rerrolled #13, which was also was broken due to recent changes in #2134857.

Status: Needs review » Needs work

The last submitted patch, 14: entity_base_cache_tags-2217749-14.patch, failed testing.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -377,6 +388,11 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
    +
    +    // An entity was deleted, invalidate its own cache tag.
    +    $ids = array_keys($entities);
    +    $entity_type_id = $entities[$ids[0]]->getEntityTypeId();
    +    Cache::invalidateTags(array($entity_type_id => $ids));
    

    Based on the above argument, couldn't this affect lists too?

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -393,6 +409,21 @@ public function referencedEntities() {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getListCacheTags() {
    +    // @todo Add bundle-specific listing cache tag? https://drupal.org/node/2145751
    +    return array($this->entityTypeId . 's' => TRUE);
    +  }
    

    That said, this seems like a very limited approach to that? What kind of lists will this affect? Will views for example automatically add this tag for all views? How should an entity type even know for which lists it's used once it provides something like views integration?

    That would prevent more specific cache clears, for example, let's say I have a frontpage with 5 different views based on a bundle, term reference, ... The forced cache tag clear would prevent me from doing something more intelligent on my specific site, like looking at a specific term reference and only invalidate a specific view?

    So that kind of would need a hook or something if you want to have a generic solution, not sure if that's overkill?

  3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -276,14 +277,12 @@ public function resetCache(array $entities = NULL) {
           }
    -      Cache::deleteTags($tags);
    +      Cache::invalidateTags($tags);
    

    Why the switch to invalidation? Without actually dealing with invalidated tags, that's not really useful?

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
26.08 KB
7.54 KB

#16.1: Deletion affects lists in general, but only in the sense that something that did appear in the list may now disappear. But in that case, the rendered list should be tagged with this entity's own cache tag, which does get invalidated. The "list" cache tags only serve the purpose of ensuring that when an entity is modified in a way that it may cause a list to either 1) have a new item or 2) affect other list pages (in a paged list) that a modified entity itself is not on.
damiankloip and I couldn't think of any example where deleting has that problem… except that now I've written this, I do see it: deleting an entity that is on page 3 of the list, will affect pages 4–N as well, even though the entity doesn't exist on those pages. So you're right! Thanks :)

#16.2: Yes, Views would automatically add this tag for all Views. This is being done in close collaboration with damiankloip from the Views team. The current cache tag support in Views is too incomplete/unreliable/broken, and we're moving back to basics. That would indeed mean that all node-based Views would be invalidated as soon as any node is modified. The next iterations will add more granularity, to use the cache more efficiently (the first next step will be to add bundle-specific list cache tags). Even though that sounds bad, it's still better than what we have today. Baby steps towards the ideal unicorn place :)

#16.3: For two reasons:

  1. we're working towards having only one anyway (so no longer both "delete" *and* "invalidate")
  2. invalidation has the same effect, but has the advantage of *allowing* the code doing the cache get to choose to use a stale cache entry (so: freedom to opt-in to stale cache entries)

If you oppose this, I'll change it back, but I can't think of any reason to keep it as is.

Removing the <entity type>_view_<entity bundle>:1 cache tag

The test failures in #14 (in the tests I added at #2188565: Test coverage for Comment, Custom Block, Node, Taxonomy Term and User entity cache tags) were very instructive. This is part of whatEntityViewbuilder::resetCache() does:

     if (isset($entities)) {
       $tags = array();
       foreach ($entities as $entity) {
         $tags[$this->entityTypeId . '_view_' . $entity->bundle()] = TRUE;
       }

The problem is the cache tag being invalidated there: "_view_:1". See, every time any entity will invalidate the render caches of all other entities in the same bundle. That makes zero sense. So I'm removing that cache tag in this reroll.

xjm’s picture

Issue tags: -beta blocker +beta target

Meant to update this yesterday; we discussed the issue with @catch and it is okay to go in after beta1 as well. So setting to beta target.

Wim Leers’s picture

FileSize
25.78 KB

Chasing HEAD.

msonnabaum’s picture

This looks great overall, but I'd like to see the contents added to postSave and postDelete in their own methods to keep them from getting out of hand in the future. Something like invalidateTagsOnSave/Delete.

Wim Leers’s picture

FileSize
26.63 KB
3.89 KB

Very fair point. Done.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

Great! RTBC assuming testbot likes it.

The last submitted patch, 19: entity_base_cache_tags-2217749-19.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: entity_base_cache_tags-2217749-21.patch, failed testing.

Wim Leers’s picture

Drupal\Tests\Core\Cache\NullBackendTest                        1 passes
PHP Notice:  Undefined property: Drupal\Tests\Core\Cache\CacheContextsTest::$results in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 701

Notice: Undefined property: Drupal\Tests\Core\Cache\CacheContextsTest::$results in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 701
Drupal\Tests\Core\Cache\CacheContextsTest                        passes

This does not cause the fatal, this is the well-known problem being fixed at #2225999: Drupal PHPUnit tests should extend UnitTestCase, not PHPUnit_Framework_TestCase.

Drupal\Tests\Core\Config\ConfigEntityDependencyTest            2 passes
PHP Fatal error:  Call to a member function getParameter() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Cache/Cache.php on line 71
PHP Warning:  Invalid argument supplied for foreach() in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 658

Warning: Invalid argument supplied for foreach() in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 658
Drupal\Tests\Core\Config\Entity\ConfigEntityTypeTest           4 passes

I ran both tests, and I didn't get any failures. In fact, I ran the entire unit test suite (both are unit tests) and didn't see anything but green.

Therefore, re-testing.

Wim Leers’s picture

The last submitted patch, 21: entity_base_cache_tags-2217749-21.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
33.86 KB
7.28 KB

#2226027: ConfigEntityBase::preSave() tries to load the original entity but instead loads itself broke this: it added a new ConfigEntityStorageTest which now needed to be updated to verify the cache tags as well. This delights me, because it ensures that our config entities are now even more precisely covered with test coverage! :) tim.plunkett++

That being said, this additional test coverage does not introduce anything new, it's just a test expansion. Therefore msonnabaum's RTBC still applies.

Hopefully the patch will now be green again.

catch’s picture

For the listing cache tags, we always knew that was going to be difficult/impossible to get right.

I think it's fine to add the entity type, then split that to bundle, then try more granular after that - anything is better than the 'content' cache tag or manually clearing the {cache_render} bin.

Additionally it ought to be possible to override the entity base classes to prevent that cache tag ever being set via contrib/custom code - that should allow sites to do something more granular themselves.

Haven't reviewed the whole patch recently, will try to get back here soon now it's RTBC.

Wim Leers’s picture

Assigned: Wim Leers » catch

Assigning to catch per #29.

Berdir’s picture

Yeah, I think the list stuff is the only questionable thing here I guess.

As @catch mentioned, a way to control/alter the cache tags of an entity would be very useful on one side and possibly also the opposite, controlling which tags a views uses, but that should probably be possible by altering the render array it returns?

Wim Leers’s picture

Each entity class has control of its cache tags: all it has to do is override the parent methods (getCacheTag() and getListCacheTags()).

Views is not yet using any of this, that's one of the next steps. I've discussed this issue with damiankloip, this is one of those baby steps that Views needs in order to use cache tags correctly/consistently. And yes, it would be possible to control which tags a View uses, and Views will come with multiple caching strategies: you can choose to not rely on cache tags at all if you so desire.

This is the baby step that brings us standardized (and therefore: reliable, predictable, easily determinable) cache tags. Once we have that, and once we use those cache tags correctly and consistently, we can look into optimizations.

Berdir’s picture

Each entity class has control of its cache tags: all it has to do is override the parent methods (getCacheTag() and getListCacheTags()).

I know, by altering, I mean someone else than the entity class. Every site uses nodes differently and has different views and lists and so on, Node can not possibly know how my site works.

Wim Leers’s picture

#33: Sure, but "ability to alter the set of cache tags that an entity invalidates" is a pre-existing problem.

And this patch will make it simpler to do just that, because we could make getCacheTag() and getListCacheTags() invoke some alter hooks :)

damiankloip’s picture

I'm sorry. I am all for this issue , it's a great thing for cache tags. However, views IMO will not be using this. As mentioned before - we pretty much cannot rely on anything that generates information on a per row basis in views :/

Berdir’s picture

One more thing, as commented above, the cache documentation now has a @todo that points to this issue: https://api.drupal.org/api/drupal/core%21modules%21system%21core.api.php... (end of that chapter). would be nice to describe this a bit there.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

entity_base_cache_tags-2217749-28.patch no longer applies.

error: patch failed: core/modules/block/lib/Drupal/block/BlockViewBuilder.php:68
error: core/modules/block/lib/Drupal/block/BlockViewBuilder.php: patch does not apply

visabhishek’s picture

Status: Needs work » Needs review
FileSize
32.64 KB

Simply reroll

Status: Needs review » Needs work

The last submitted patch, 38: entity_base_cache_tags-2217749-38.patch, failed testing.

Jalandhar’s picture

Assigned: catch » Jalandhar
Status: Needs work » Needs review
FileSize
32.61 KB
2.12 KB

Applying patch with reroll and required changes for the cause of failure in #38.

Please review.

Status: Needs review » Needs work

The last submitted patch, 40: entity_base_cache_tags-2217749-40.patch, failed testing.

Jalandhar’s picture

FileSize
32.62 KB
648 bytes

Sorry, used $entity->id() instead of $entity->get('theme') for getting theme.

Updating the patch with this change from my previous patch. Please review it.

Jalandhar’s picture

Status: Needs work » Needs review
Berdir’s picture

See #36, we should get rid of that @todo.

Wim Leers’s picture

Assigned: Jalandhar » Wim Leers
Issue tags: -Needs reroll
FileSize
39.2 KB
6.45 KB

#38 was not simply a reroll, it introduced a change. And the rerolls after it (#39–#42) are very tricky to follow. Plus, the applied solution was wrong. Therefore, here's a fresh reroll, against #28.


#28 didn't apply anymore because #2185617: Each page cache entry should have a theme cache tag landed. So in this interdiff, you'll see an updated diff for \Drupal\block\Entity\Block and BlockViewBuilder.

@Berdir: updated the cache tags docs.

Berdir’s picture

+++ b/core/modules/system/core.api.php
@@ -277,13 +277,29 @@
  * - An array of values. For example, the "node" tag indicates that particular
- *   nodes' data is present in the cache, so its value is an array of node IDs.
- * Data that has been tagged can be deleted or invalidated as a group.
+ *   nodes' data is present in the cache item, so its value is an array of node
+ *   IDs.

As discussed, nodes' should be node's as it's a single node.

Looks great :)

Wim Leers’s picture

FileSize
39.2 KB
828 bytes
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Great, back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: entity_base_cache_tags-2217749-46.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
39.17 KB
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

And back to RTBC; should come back green.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 50: entity_base_cache_tags-2217749-50.patch, failed testing.

Jalandhar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 50: entity_base_cache_tags-2217749-50.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
39.25 KB
932 bytes

I made a mistake in the straight reroll that's triggering that one test failure. This will be green.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed bf61ca1 and pushed to 8.x. Thanks!

  • Commit bf61ca1 on 8.x by alexpott:
    Issue #2217749 by Wim Leers, Jalandhar, visabhishek, damiankloip: Entity...
Wim Leers’s picture

Issue tags: -sprint

HURRAY! At last :)

Thank you! This unblocks a lot of cache tag work!

Status: Fixed » Closed (fixed)

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