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.
Updated: Comment 0
Problem/Motivation
Proposed resolution
Note: We don't leverage cache tags on the actual executing level, so views still executes the view.
Remaining tasks
User interface changes
API changes
Original report by @username
I suspect a bunch of cache handling in views is broken. There have been a few changes in core that have not yet been included in views. Especially cache tag stuff.
Attached is a patch that at least fixes a hook call :)
Comment | File | Size | Author |
---|---|---|---|
#55 | interdiff-1712456-55.txt | 4.51 KB | damiankloip |
#55 | 1712456-55.patch | 32.11 KB | damiankloip |
#39 | 1712456-38.patch | 24.52 KB | damiankloip |
#39 | interdiff-1712456-38.txt | 9.33 KB | damiankloip |
#37 | 1712456-37.patch | 16.82 KB | damiankloip |
Comments
Comment #1
dawehnerCommitted this part, because it is simply fine. Thanks!
Regarding the cache: views already uses cache($bin)->get(), i guess this is fine.
Regarding tags, it would maybe make sense to port the dynamic creation of cache keys to replace with tags.
As far as i understand tags it does simply the same kind of stuff but it would be easier.
Comment #2
aspilicious CreditAttribution: aspilicious commentedadding tag
Comment #3
aspilicious CreditAttribution: aspilicious commentedAfter grepping views I found one problematic file:
views\handlers\views_handler_relationship_groupwise_max.inc
That file uses cache_clear_all, cache_get and cache_set. These should get converted with cache_tags (so we can replace cche_clear_all with cache_invalidate(...).
Not 100% sure how to do this so leaving to others.
Comment #4
aspilicious CreditAttribution: aspilicious commentedThis should do it I think
Comment #5
aspilicious CreditAttribution: aspilicious commentedwrong patch
Comment #6
dawehnerThis part is fine, thanks!
lets figure out whether/how to use cache tags
Comment #7
damiankloip CreditAttribution: damiankloip commentedAre cache tags going to be that useful here, with a relationship plugin that can be used for lots of different things?
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedInteresting issue! In general, the cache tags that should be written relate to the ids that are returned by the query. So we want to return something like
array('node' => array(8,12,16,...)
as cache tags. Maybe we return a maximum of 500 ids? I'll ping catch to chime in here.Some related nice-to-have issues:
Comment #9
catchFor cache tags and views, I think there's two bits:
1. Cache entity IDs that appear in the cached, rendered HTML (so if there's a pager showing ten items per page, the ten entity IDs on that page). This should more or less replace views content cache - at least in the sense that if one of those entities is updated or deleted, the cache will be invalidated.
The only thing required then is getting the entity IDs out of the view and into the cache tags. If we have a new nested rendering model with blocks/layouts, then ideally the individual entity view mode cache tags would filter up to the rendered output of the whole view, and Views wouldn't actually need to do any of this (assuming it's using standard rendering). Until that's in place though, Views could do a simple implementation.
2. (much harder) tag the view itself with cache tags that affect the query results. So if a view lists nodes of type article, have a cache tag that is "$entity_type-$bundle" and tag the view with it, invalidating that tag when a matching entity to the tag definition is saved. This requires conventions about how tags are named/invalidated which we don't have in core yet. Figuring out which possible cache tags might affect a view is definitely Views' job, defining which tags get invalidated when entities are saved I'm not sure exactly where the responsibilities lie for that yet.
Comment #10
dawehnerRegarding the first point, this kind of solved already, because we have methods which return the used entities on the view.
The second one is indeed much harder, for example if you have a bundle filter, you should only add the cache tag if the bundle filter is not exposed for users, as there might be to granular caching. I kind of feel that this is not possible in a satisfying way,
so in the end people have to write custom code to get the full caching power.
Comment #11
xjmComment #12
DamienMcKennaI'd recommend also tracking the bundle so that the cache for all objects of that type can be flushed when the bundle is modified.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commented#1748164: Use #attached to add all CSS and JS is in so this is not blocked, AFAIK. Views still does a drupal_render() internally so it isn't perfect but I think we should proceed with adding tags even if the tags don't bubble all the way up the page.
Comment #14
dawehnerIt seems to be that we really have to make this bit swappable, because even we try it hard, there seems to be now way to do it right for all usecases?
Comment #15
ordermind CreditAttribution: ordermind commentedI remember back in the D6 days that using the WHERE part of the views query as a basis for determining which entities are involved in the view worked pretty well. For example a view would list nodes of a certain type, and then when a node of that type was created, updated or deleted, the view cache would be invalidated.
Comment #16
damiankloip CreditAttribution: damiankloip commentedThis still needs work, but I think this might be the right direction (**think).
Comment #18
damiankloip CreditAttribution: damiankloip commentedComment #19
amateescu CreditAttribution: amateescu commentedDiscussed in Prague that we could also add a tag with the entity bundle so we don't clear every view by entity type when an entity is changed, and here's an interdiff that adds support for invalidating the bundle tag in EntityRenderController.
Comment #20
damiankloip CreditAttribution: damiankloip commentedMerged in amateescu's patch, added a quick test for the getEntityTypes() method.
Also added a Tag cache plugin, so cache items will only be invalidated based on tags, and persist until that happens.
Still needs some more work, and tests.
Comment #21
dawehnerI am wondering how this would work with a listing which has different results depending on new content ... can we somehow support the usecase?
We could simplify the code a lot by using Query->getEntityTables() ... maybe we should try to reorganize some of the code.
Comment #22
damiankloip CreditAttribution: damiankloip commentedSo, if we have an ENTITY_TYPE_list_BUNDLE tag that gets added to views cache entries, this can get invalidated by the entity render controller resetCache() method, which will get invoked during postSave().
After our discussion, I've changed getEntityTables() to getEntityInformation() and refactored it a bit.
So I guess some questions:
- Do we make it so that the tags are only generated when a view uses fields? or certain tags anyway? A in theory, rendered entities should generate their own tags that would bubble up, so we wouldn't need to generate these twice.
- Cache tags like this only really work with entities so how do we deal with this in the UI? Just some clear descriptions that this will only invalidate stuff based on entities? Conditionally showing this plugin is probably a no go too, as you could have a base table with no entity support that creates relationships to entities etc.. this is pretty dynamic.
Comment #24
Wim LeersThis is only true when using Views that use the Entity/Field API render pipeline, isn't it? I.e. when using table views, Views would have to generate cache tags.
AFAICT Views in core only offers two caching options: "none" and "time-based". I think/fear you're right, which means that we'd need to present entity-based cache invalidation not as a third option (a third radio button in the UI), but something entirely separate?
OTOH, do we even need to provide a UI option for this? Why wouldn't we always want to enable this? Which means the UI could maybe have some text along the lines of "Note that Views' caching will always automatically react to entity changes."
Also, does Views have enough metadata to know whether *all* its contents are entity-based? If so, we can make smart decisions and enable entity-based caching by default.
Finally: note that individual entities can disable render caching, so Views probably needs to deal with that too.
Comment #25
damiankloip CreditAttribution: damiankloip commentedYes, see previous comment: "Do we make it so that the tags are only generated when a view uses fields? " - So we can tell easily when a view is using field based rows or not. Our results will also contain the loaded entities too, so it would only use these anyway.
Correct, this patch currently adds a third for tag based caching only (as well as for time based as it would still need to invalidate in that case) but as you point out, this could lead to issues with particular views that do not use any entities atm.
Yes, the current patch already takes care of that, we can use views' data to determine all the various entities used in the view, from the results (entities and relationship_entities).
This currently is not taken care of in the patch, but neither is any checking for field based views, so tags would be generated all the time currently. We could check this in the entity info aswell though, that shouldn't be a problem. I guess we could get into a few issues with some entities having render caching and some not..
Comment #26
dawehnerSo does that mean that we basically can't support query caching at all? This makes it kind of pointless for many many usecases. Should we maybe allow to combine time for query and tags for actual rendering?
Comment #27
DamienMcKennaPlease excuse me for not knowing all of the APIs so I may be unaware of something, but won't getCacheTags() lead to the same item being added multiple times to the $tags list? Shouldn't $tags[$type] be filtered for duplicates?
Comment #28
damiankloip CreditAttribution: damiankloip commentedDo you mean from the relationship entities?
Comment #29
DamienMcKenna@damienkloip: Yes. Never mind occasions those crazy views where resultant objects reference each other.
Comment #30
chrisjlee CreditAttribution: chrisjlee commented#25 - @damiankloip, I think entity is misspelled unless i'm missing something here (forgive me i'm just lurking; trying to get up to speed.)
Comment #31
damiankloip CreditAttribution: damiankloip commentedMade those changes.
Daniel, not sure what to do about the query caching atm, as we don't support that at all really. So that should be tackled if/when we add that?
Comment #33
xjmComment #34
dawehnerI think that a cache plugin without a query cache is basically dump.
Comment #35
damiankloip CreditAttribution: damiankloip commentedThis should get tests passing again, now the entities are loaded again...
I spoke to Daniel on IRC, he was getting query cache and results cache mixed up, this patch still supports results cache like before. Adding query caching is another issue altogether :)
Comment #37
damiankloip CreditAttribution: damiankloip commentedWe don't have the ViewExecutable::getEntityTypes() method anymore...
Comment #38
dawehnerWith this we have an infitive time until the caches of the result change. What about writing the class in a way that it either extends the time one, or accepts an existing entry and just wraps it with the cache expire logic needed here?
Comment #39
damiankloip CreditAttribution: damiankloip commentedHere is a new patch with some quick web tests, and a couple of other changes/fixes that were needed.
Comment #41
Wim Leers39: 1712456-38.patch queued for re-testing.
Comment #43
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #45
damiankloip CreditAttribution: damiankloip commentedThis needs a bit of fixing. Leave it with me.
Comment #46
damiankloip CreditAttribution: damiankloip commentedLet's try this.
Comment #48
damiankloip CreditAttribution: damiankloip commentedReroll and a couple of fixes.
Comment #49
amateescu CreditAttribution: amateescu commentedThe *_view_$bundle tag is not set anywhere and I commented on *_list_$bundle below as I don't think it brings any benefit.
Also, it's weird for the generic view builder to clear tags that it doesn't know about (since they are set by Views).
It's not clear from the patch why this change is necessary..
A 'view' tag seems to be too generic, how about 'views_view' instead?
I'm not sure this tag brings any benefits, we don't have code that clears render cache by entity bundle..
Comment #50
damiankloip CreditAttribution: damiankloip commentedYeah, this isn;t implemented yet, it seems we should implement bundles though? Say you save settings for one node type, it would invalidate caches for every node on the system?! So I guess nowhere does this now but I think we should start to.
We need a way like this to invalidate 'lists' containing this entity type. This and 'view' could be similar though I guess.
Erm, see #19 for that ;)
Don't all other tags just go with entity_type => array(id => id) too?
Comment #51
damiankloip CreditAttribution: damiankloip commentedOk, thanks amateescu. I think I've implemented your points; getEntityInformation is now called getEntityTableInfo (much better imo!), and I've removed the '_list_' tags.
Also, added a couple more test cases, as cacheFlush() was actually broken, as well as the filter on the view (the new test assertions cover this too, by using an article type).
Comment #52
amateescu CreditAttribution: amateescu commentedAwesome! I'll wait for the testbot before rtbc-ing.
Comment #53
dawehnerI am confused. So we do name the variable entity_ids, but we store full entities (according to referencedEntities().
What about using \Drupal\Core\Entity\EntityInterface[] instead?
I wonder whether we should call it on postDelete as well. Additional I ask myself whether the entity system should clear tags for entity types automatically.
Do we typehint on concrete entity classes or just on the interfaces?
I don't get why this change is needed, sorry.
Comment #55
damiankloip CreditAttribution: damiankloip commentedThanks! I've made those changes. You're right, the postDelete needs to be added too.
I also added a todo, that we could remove that code if views ever implements view builders.
Comment #56
amateescu CreditAttribution: amateescu commentedLet's do this :)
Comment #57
dawehnerComment #58
webchickThis is most likely best for catch to review.
Comment #59
Dries CreditAttribution: Dries commentedCommitted to 8.x. This could make a big difference on performance. Hard to measure right now though.
Comment #60
dawehnerPlease tell people that this is horrible for any view which has changing entries (feels like every view), as they will just not expire at all.
Comment #61
dawehner.
Comment #62
damiankloip CreditAttribution: damiankloip commentedNot sure why you're bringing this up at this point.
Let's catch up about this at some point, either way I think this should be a followup. Not re opening this one With the front of a string change... :)
Comment #63
dawehnerThe actual problem is that the committer was probably not aware how limited this feature actually is.
Comment #64
damiankloip CreditAttribution: damiankloip commentedI would like to think we can iterate on this now in some ways and improve this feature as we start using it more. If we are clever about our invalidation strategies, this will be very useful.
Comment #65
catchI was aware of this limitation, not sure if Dries was. Just to make it clear what the limitation is:
Solved:
Edits or deletes to items rendered as part of a view listing will invalidate the cache entry for the view. This is a cache tags version of views_content_cache
Not solved:
Newly listed items in the view (whether new entities, or entities edited where the edit would result in them appearing in the view).
There's two ways to fix this problem that I know of:
1. Add a TTL to the cache entry - this ensures no stale items/content are left in the view, then it's up to the person configuring the view to decide what the maximum acceptable time is for new items to appear in it.
2. Try to add extra cache tags to the view (and EFQ listings) that will be invalidated when items are created or updated with certain properties.
So for example a newly create article that's promoted to front page and published should invalidate the front page view, but a newly created article that's unpublished shouldn't. Adding a taxonomy term to an existing article could invalidate the taxonomy/term/n cache for that term, but not for all the other terms.
Comment #66
amateescu CreditAttribution: amateescu commentedThis is not really true. The patch introduced a "bundle" tag that is set by the view and cleared by the entity view builder, so any new or edited entity which is of a bundle that is already part of a view will clear the render cache for that view.
What's not covered is a new entity (not edit, since we assume we can't change bundles) of a bundle which *not* already part of the view.
Edit: Given the above, I've no idea why Daniel is so concerned over this change, the use case which is not covered is quite limited, as I don't think there are many views with no bundle restriction.
Comment #67
effulgentsia CreditAttribution: effulgentsia commentedI opened #2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing for resolving the remaining problems.
Comment #68
olli CreditAttribution: olli commentedIs the tag based plugin same as time based with expire times set to 0? If so, do we need this separate plugin? It is not obvious in the UI that time based uses tags too.
Comment #70
Wim LeersMore cache tags + Views issues, one of which is about crucial aspects that were forgotten in this patch: