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
- 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.) - 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). - Have the
Entity
base class implement those methods, so that all entities in Drupal core get them for free. - Have the
Entity
base class implementpostSave()
andpostDelete()
, to ensure that the aforementioned cache tags are invalidated when appropriate. - Have the
ContentEntityBase
&ConfigEntityBase
classes also implement cache tag invalidation for other methods where that is necessary: for now onlyConfigEntityBase::disable()
. - 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. - Have the
EntityViewBuilder
andBlockViewBuilder
classes implement and use that method. - 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
- Get blocker committed (again): #2134857: PHPUnit test the entity base classes
- Get patch green
- Build consensus
- Get committed
User interface changes
None.
API changes
EntityInterface::getCacheTag()
EntityInterface::getListCacheTags()
Postponed until
Comment | File | Size | Author |
---|---|---|---|
#55 | interdiff.txt | 932 bytes | Wim Leers |
#55 | entity_base_cache_tags-2217749-55.patch | 39.25 KB | Wim Leers |
#28 | interdiff.txt | 7.28 KB | Wim Leers |
#28 | entity_base_cache_tags-2217749-28.patch | 33.86 KB | Wim Leers |
Comments
Comment #1
Wim LeersThis 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".
Comment #2
Wim LeersPatch!
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.
Comment #3
xjmIs #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.
Comment #4
andypostSuppose this could help ER fields properly clean their cache
Comment #5
Wim Leers#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 :)
Comment #6
BerdirI 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 :)
Comment #7
jessebeach CreditAttribution: jessebeach commentedComment #8
jessebeach CreditAttribution: jessebeach commentedComment #9
Wim Leers#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.
Comment #10
Wim LeersI found a silly piece of dead code that I should've removed in the "phase 2 interdiff" in #2.
Comment #11
Wim LeersIn 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:
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:
Comment #12
damiankloip CreditAttribution: damiankloip commentedThis 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.
Comment #13
Wim LeersAfter posting #11, I noticed a single failure when running the entire PHPUnit suite:
ViewUI
implementsConfigEntityInterface
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.Comment #14
Wim Leers#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.
Comment #16
BerdirBased on the above argument, couldn't this affect lists too?
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?
Why the switch to invalidation? Without actually dealing with invalidated tags, that's not really useful?
Comment #17
Wim Leers#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:
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 tagThe 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 what
EntityViewbuilder::resetCache()
does: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.
Comment #18
xjmMeant 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.
Comment #19
Wim LeersChasing HEAD.
Comment #20
msonnabaum CreditAttribution: msonnabaum commentedThis 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.
Comment #21
Wim LeersVery fair point. Done.
Comment #22
msonnabaum CreditAttribution: msonnabaum commentedGreat! RTBC assuming testbot likes it.
Comment #25
Wim LeersThis 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.
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.
Comment #26
Wim Leers21: entity_base_cache_tags-2217749-21.patch queued for re-testing.
Comment #28
Wim Leers#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.
Comment #29
catchFor 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.
Comment #30
Wim LeersAssigning to catch per #29.
Comment #31
BerdirYeah, 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?
Comment #32
Wim LeersEach entity class has control of its cache tags: all it has to do is override the parent methods (
getCacheTag()
andgetListCacheTags()
).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.
Comment #33
BerdirI 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.
Comment #34
Wim Leers#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()
andgetListCacheTags()
invoke some alter hooks :)Comment #35
damiankloip CreditAttribution: damiankloip commentedI'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 :/
Comment #36
BerdirOne 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.
Comment #37
alexpottentity_base_cache_tags-2217749-28.patch no longer applies.
Comment #38
visabhishek CreditAttribution: visabhishek commentedSimply reroll
Comment #40
Jalandhar CreditAttribution: Jalandhar commentedApplying patch with reroll and required changes for the cause of failure in #38.
Please review.
Comment #42
Jalandhar CreditAttribution: Jalandhar commentedSorry, used $entity->id() instead of $entity->get('theme') for getting theme.
Updating the patch with this change from my previous patch. Please review it.
Comment #43
Jalandhar CreditAttribution: Jalandhar commentedComment #44
BerdirSee #36, we should get rid of that @todo.
Comment #45
Wim Leers#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
andBlockViewBuilder
.@Berdir: updated the cache tags docs.
Comment #46
BerdirAs discussed, nodes' should be node's as it's a single node.
Looks great :)
Comment #47
Wim LeersComment #48
BerdirGreat, back to RTBC.
Comment #50
Wim Leers#2124535: Prevent secondary configuration creates and deletes from breaking the ConfigImporter broke this.
Straight reroll.
Comment #51
Wim LeersAnd back to RTBC; should come back green.
Comment #53
Jalandhar CreditAttribution: Jalandhar commented50: entity_base_cache_tags-2217749-50.patch queued for re-testing.
Comment #55
Wim LeersI made a mistake in the straight reroll that's triggering that one test failure. This will be green.
Comment #56
alexpottCommitted bf61ca1 and pushed to 8.x. Thanks!
Comment #58
Wim LeersHURRAY! At last :)
Thank you! This unblocks a lot of cache tag work!