Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Over at #2241229-12: Aggregator Feed & Item content entity types, and AggregatorFeedBlock should use cache tags, we noticed that we haven't yet added test coverage for EntityInterface::getListCacheTags()
. We should add test coverage for that. This issue began doing that and only that, but in the process it encountered several prohibiting problems:
EntityInterface::getListCacheTags()
is an instance method, but it also needs to be called when e.g. a view listing nodes is empty, i.e. when there is no instance- inconsistent and often plain wrong usage of list cache tags in Drupal core (precisely because we don't yet have test coverage)
protected function Entity::onSaveOrDelete()
in HEAD invalidates cache tags of all referenced entities, just in case they have a back reference, which was the original/simplistic approach to ensure e.g. the "nodes for term" listing page was invalidated when a term was invalidated when necessary. That's now unnecessary, because either A) it is a backreference to this specific entity which today has a cache tag (and back then it did not yet), and this specific entity's cache tag is invalidated upon saving/deleting anyway, so then it's unnecessary, or B) it is a "backrefererence list" (again the "nodes for this term" listing), which is a listing, in which case the list cache tag should be used (EntityInterface::getListCacheTag()
). As a side-effect, this means that far too many cache items were being invalidated! (See #12b for a clear explanation of that.)
Proposed resolution
Test coverage for EntityInterface::getListCacheTags()
, but also:
- Make
EntityInterface::getListCacheTags()
a static method - Corrected usage of list cache tags.
- Removed
protected function Entity::onSaveOrDelete()
and used the entity's list cache tag when necessary.
Remaining tasks
Review.
User interface changes
None.
API changes
- Change:
public function EntityInterface::getListCacheTags()
->public function <strong>EntityTypeInterface</strong>::getListCacheTags()
(to allow listings to associate the "list" cache tag of an entity even if there are zero matches; currently you can only get the entity type's list cache tag from an instance). We can keep BC if necessary by having a deprecated getListCacheTags() on the base Entity class (but not on the interface) - Addition: All listings (Views,
CommentDefaultFormatter
and book navigation) now set the needed list cache tags.
Comments
Comment #1
Wim LeersThis patch is a requirement (and hence a blocker) to have reliable Views caching, with cache tag-based invalidation. Without this patch and the guarantees it provides, Drupal 8's cache tags will only be guaranteed to be reliable for individual entities.
The test coverage that this patch adds will allow us to have peace of mind for Views caching. It thoroughly tests all permutations, and because it adds this test coverage in
EntityCacheTagsTestBase
, all existing cache tag test coverage for entities now has list cache tag test coverage.I only know of three non-administrative listings of entities in Drupal core that are not generated by Views (we don't need to render cache administrative listings):
CommentDefaultFormatter
— now has the comment list tagComment::getListCacheTag()
In this patch:
public function EntityInterface::getListCacheTags()
topublic static function EntityInterface::getListCacheTag()
: to allow listings to associate the "list" cache tag of an entity even if there are zero matches; currently you can only get the entity type's list cache tag from an instance.EntityCacheTagsTestBase
.protected function Entity::onSaveOrDelete()
, because it only does things that used to be necessary, but not anymore: it reset view builder cache entries for all referenced entities (in case they have a back reference, e.g. for the "nodes tagged with this term" listing). That's unnecessary, because either A) it is a backreference to this specific entity, and this specific entity's cache tag is invalidated upon saving/deleting anyway, so then it's unnecessary, or B) it is a "backrefererence list" (again the "nodes for this term" listing), which is a listing, in which case the list cache tag should be used (EntityInterface::getListCacheTag()
). The list cache tag was not used correctly yet, but now it is.UserPictureTest
, which enabled/disabled the user pictures on nodes and comments, but due to missing cache invalidation actually always tested one and the same thing…) have been fixed, all uncovered by working on this, and all necessary.As you can tell, this patch is very necessary, without it, Drupal 8's cache tag handling still leaves a lot to be desired. Now that cache tag handling is solid for most things, we can now finally deal with "list" cache tags, and have comprehensive test coverage :)
Comment #2
Wim LeersComment #3
BerdirLooks great, bunch of questions and hints...
Wondering if we should just move this to EntityTypeInterface, that would be easier to access when it is static, and this could stay non-static and forward to that? Harder to override I guess, not sure how common that is. (Not sure how a solution for that would look like atm)
=> This shorthand static methods and the loading based on the class name is most a shorthand for developers, it is not guaranteed to work (for example, when you have dynamically defined entity types or something like that).
Beautiful.
Nitpick: missing trailing comma.
I guess this is one of those where the dynamic lookup doesn't work?
Why is this necessary?
What are we testing here exactly?
I guess this is related to the non-unique tags issue that we found?
Are you sure this is correct? should this be the {$id}s tag?
Ah, the previous call didn't work anymore because we're then testing on nodes and comments.
Looks like we enable the features through the API? Would it work if we would use the UI? And if not, should we make the UI work with this tag for now, so that not only the test works, but actually doing it as a user does too?
Yeah, code like this when knowing that this will then call back to the entity manager to figure out the entity type is.. crazy :)
But nice find.
I'm still planning to look into manually optimizing the cache tag handling once it works in a general/global way. What I have in mind is basically a custom cache plugin that allows me to manually set a cache tag + code that clears that tag on certain conditions. Like, for a view of promoted nodes of a specific type, I don't want to add the global list tag, because that will do way more cache clears than necessary). But that's a different topic...
Comment #4
Wim LeersConfigEntityStaticCacheTest
, amongst others.core/modules/hal/src/Tests/EntityTest
and run that test (it's a fast test, so no pain there). Now you'll get bizarre failures, along the lines ofValue array(array()) is not equal to value array(array('value' => ''))
. I started debugging this, and for some reason, an entity that is created in memory and then saved has different field values compared to the denormalized normalized entity… but only for fields that don't have any value set and that also don't have a default value specified in the base field definition. Just specifying a value made the test pass. No caching at all is going on here, so I don't understand why this test would suddenly fail, but it sure seems like something is broken somewhere deep… :($tags
contains the expected cache tags, in string form (['node:5', 'user:3']
), and I hit a case where I might add the same twice. So: unrelated.onSaveOrDelete()
was wrong! And you're correct, using the UI would work. All "system"/"config" forms invalidate all caches. IMO we shouldn't use the UI in this test, we should just fix #2040135: Caches dependent on simple config are only invalidated on form submissions.We're also planning to work on this! I'm very, very, very happy that you say you want to work on this, which means you're at the very least interested :) damiankloip and I have been discussing that. The thing is: before we can work on that, we must make sure that less optimized cache tag invalidation for Views actually works, and it unfortunately currently doesn't. See #2183017: Views doesn't bubble up rendered entities' cache tags and all related issues, and related issues on the related issues.
Comment #5
BerdirRe-roll.
EntityTest: Had a look. What is/was happening is that onSaveOrDelete() -> getReferencedEntities() goes through all fields, field items and their properties to find EntityReference properties. inlcuding computed
This force-initializes $this->properties for all properties, which only happens when you access it as a property (->get('value') or getProperties() and not ->value).
That affects Map::getValue(), which starts of with $this->value and adds initialized properties, so that any changed value is correctly updated and returned. On HEAD, properties are force-initialized, so they are explicitly added to $values. Now, this no longer happens. That's where the difference is.
I think fixing it by explicitly providing values is good enough for this, the serializiation of empty fields is already weird in HEAD and it has no practical difference, we could actually consider to filter out fields that are empty from the test, not sure and not here.
I think this leaves the behavior and location of getListCacheTag() as the last problem here.
Comment #6
dawehnerI am a bit confused and curious, as this basically means that the cache tags can't depend on actual information any longer. Won't this make a lot of the really complex usecases
of views even impossible right from the beginning?
Comment #7
Wim LeersOh, somehow I missed your comment, Berdir! And thanks for the reroll :)
So basically you're saying:
?And I'm confused by this statement :) What is made impossible by which exact change? You still have the exact same possibilities in terms of getting a given entity's cache tags, in any case.
Comment #8
damiankloip CreditAttribution: damiankloip commentedJust appending and 's' to the end is still strange, I didn't like that before either. Some entity types this is weird with:
- block_content > block_contents
- editor > editors
- menu_link_content > menu_link_contents
- taxonomy_vocabulary > taxonomy_vocabularys
Also, we need to consider the possibility of getting bundle aware list tags? How do we manage that generally? Let alone with a static method. I'm not sure :)
Or do we just make them ourselves?
Comment #9
Wim LeersOh, sure, let's change that from suffixing "s" to suffixing "_list". I also dislike it.
I want to defer the "bundle list cache tag" discussion to a follow-up issue, because that's a can of worms. This is big progress. Let's get this in first! We even already have an issue for bundle list cache tags: #2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing.
Comment #10
xjmThe title and summary don't describe a critical issue, and the patch seems to be doing more than adding test coverage. Can we update it with the actual scope of this issue, including explaining how it actually blocks reliable views caching (see Wim's initial comment in #1).
Comment #11
xjmNote that if it's a soft blocker for the related issues Wim links (would make them easier/cleaner/safer to patch, but not a hard blocker nor any actual critical bug), then it would make sense to mark this major. Critical tasks are usually designated at maintainer discretion, and this issue doesn't really fit the description. Thanks!
Comment #12
Berdir@xjm: This is critical for two reasons I think:
a) We currently have 3 different notations for "list of entities": nodes, node_view and node_list and we use it differently in different places. That makes it impossible/very hard to use the correct cache tag for clearing/tagging "listings of nodes".
b) In HEAD, we clear cache tags of *referenced* entities. For example, comments display and author and reference that author. When you currently update a comment, we clear the cache tag of that user. That means that we delete every render cached node/comment/entity authored by that user. That is very inefficient and unecessary, because cache tags are supposed to work the other way round. Your cache should be included everywhere where it is relevant, so that is enough to just clear your own cache tag. Changing this shows a number of places that currently don't use cache tags properly (Or incorrectly/unknowingly rely on this behavior to work), so this patch has to fix them.
Those are API/behavior changes that need to happen asap, so that contrib can use cache tags properly.
Comment #13
xjmSo a better title/scope would be something like: "Do not propagate cache tag clears referenced entities and standardize tag list names"? And sounds like a bug instead?
Comment #14
damiankloip CreditAttribution: damiankloip commented#2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing was an issue opened when we had the initial views cache tags issue in. Which we then simplified. I would say list tags is a different thing? Maybe that could be re purposed, I don't mind.
Comment #15
Wim Leers#13: Yes, basically the original intent of this issue is fully captured in the current title. But, in doing so, I was forced to fix problems in order to make the issue title actually reality. Most notably, the problem in #12b needed to be fixed.
Title & issue summary updated accordingly.
Good call, xjm!
Comment #16
Wim LeersReroll now that #2340123: Setting cache tags can be tricky: use strings instead of nested arrays to improve DX has landed. Interdiff only shows the changes that weren't part of the conflict.
If I missed anything, thanks to #2340123 throwing an exception, we'll find out with certainty :)
Comment #17
damiankloip CreditAttribution: damiankloip commentedWhy (getCacheTag() changed somewhere else too) are we also changing to getListCacheTag(). We should stick with tagS, no? It is feasible (and already done) that more than one tag is returned. As well as the return being documented as being an array. Both of these methods should be plural.
Also, when are we going to change the
$entity_type . 's'
usage? I'm sure we talked about this a few times. Can we make an issue for it?Comment #18
damiankloip CreditAttribution: damiankloip commentedThis is something we still need to resolve afaik?
ugh, howcome?
We can call static methods in the context of an instance too, but meh. Does not really matter.
Nice, it's happening here anyway :)
It is stuff like this I was talking about in my last comment, about being plural.
I guess technically we should get the entity type from the class in this case? To future proof the class being switched out?
And generally the test coverage improvements look great.
Comment #19
damiankloip CreditAttribution: damiankloip commentedWim, saw your tell on IRC:
1. Follow up is fine, created that: #2345139: Rename list cache tag suffixes from 's' to '_list'
2. The fact that this method currently just returns one tag seems like a bad reason to rename it to singular, plus it just looks wrong IMO. I also mentioned before, it's returning an array too. These methods can be overridden etc.. and it is likely more would get added. The singular naming implies that just one tag would always get returned, if that was the case it should return a string.
Comment #20
Wim Leerspublic static Entity::load()
and other static methods use.$node->getListCacheTags()
. But we don't always have an instance in the third, so it doesn't work there. I'm fine with improving that further, but let's not block progress on that.getListCacheTags()
again.These changes are listed in
interdiff-feedback.txt
.Since it seems like we won't be bikeshedding about #2345139: Rename list cache tag suffixes from 's' to '_list', and it's a very small amount of work, I've done that here already. See
interdiff-rename_list_cache_tag.txt
.Comment #22
Berdir+1 to the changes about renaming the method and s => _list.
Comment #23
Wim LeersAnd that
s
->_list
rename caused the failures in #20 :(Fixing those failures, this should be green again.
Comment #25
Wim LeersComment #27
BerdirFixed those test fails I hope. Working on what we discussed.
Comment #28
BerdirComment #29
BerdirOk, moving list cache tags to EntityType, let's see what I missed. Unit tests are passing.
Comment #31
BerdirGrr :)
Comment #34
BerdirOnly the test that failed on HEAD failed, this should be green, so needs review.
Comment #35
BerdirComment #36
Wim LeersThe entity type definition object.
Nit: Could be
string[]
.Can be deleted.
Comment #37
BerdirFixed those things.
Comment #38
Wim LeersWonderful :)
Comment #41
BerdirA conflict in the Shortcut annotation and ViewsUI had one method at the end removed by us and another added, keeping at RTBC.
Comment #42
damiankloip CreditAttribution: damiankloip commentedCool, this looks good. +1
Comment #43
catchLooks good, nothing to complain about.
Comment #44
webchickI had reviewed this too the other night and only didn't commit it on a technicality, soooo...
Committed and pushed to 8.x. Thanks!