Opening this as a follow-up to #636454: Cache tag support and #730060: Replace CACHE_TEMPORARY, cache_clear_all() and minimum cache lifetime with cache tags support, especially the first issue has a lot of background discussion that is worth going through on how this might be applied.
We need to do the following:
- apply cache tags to rendered entities.
- collect those cache tags on rendering (will likely be similar to the #attached support in drupal_render())
- clear those tags on entity updates.
- additional review all uses of cache_invalidate() that are currently in core to determine whether they're appropriate/necessary.
- probably as a follow-up to this issue, determine how to apply cache tags to lists of content - i.e. where changes to content will affect what shows up in the list. This is a lot more complicated to figure out, and doesn't affect the initial implementation of tagging individual content pieces and aggregating them.
Putting this against base system since this is going to be part drupal_render(), part entity system most likely, and it probably won't affect cache backends at all.
Comment | File | Size | Author |
---|---|---|---|
#202 | 1605290-202.patch | 1.89 KB | Anonymous (not verified) |
#200 | 1605290-200.patch | 1.82 KB | Anonymous (not verified) |
#194 | 1605290-194.patch | 56.48 KB | amateescu |
#194 | interdiff.txt | 582 bytes | amateescu |
#192 | 1605290-192.patch | 56.37 KB | amateescu |
Comments
Comment #1
catchFirst, untested, draft for drupal_render() support.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedah, nice. perhaps we should use a tag to keep track of follow up cache tags issues? i've added the tag 'D8 cache tags', feel free to change it.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedI'll try to move this along since Views is working on adding cache tags to its cache plugins. #1712456: How to leverage cache tags in Views
Comment #4
catchMoshe - any update on this?
Comment #5
amateescu CreditAttribution: amateescu commentedHere's a start on the entity side of things :) Not sure if I'll be able to get back to it before next week so I'm not assigning the issue to me just yet.
Comment #6
catchWe only need the entity ID tag here afaik. Not sure there's a use case for entity type.
Can't find much else to complain about though.
Comment #7
catchTalked to amateescu, there's a decent case for having the entity type as a tag - for example updating display settings you'd be able to clear the cache just for that entity type. Also just one tag per entity type is a relatively small number to be fetching each request.
amateescu also mentioned that we need to account for child elemends such as field items adding cache tags for parents, without being cached themselves - say a reference field etc. It looks like #cache['tags'] should support this fine - drupal_render() already allows you to have #cache set but no cid or cid parts.
Comment #8
amateescu CreditAttribution: amateescu commentedMy friday just cleared up, so I'll work on merging #1 and #5 + what was discussed above sooner than expected.
Comment #10
catchComment #11
amateescu CreditAttribution: amateescu commentedHere's some more progress on this.
The most important change is that I figured out we also need to clear the render cache when creating new entities, not only on update. I can easily find at least two scenarios where we need this:
- when a new comment is posted, we want the node page associated with it to be updated
- when we create a new node and tag it with a taxonomy term, we want the taxonomy/term/ page to be updated
The patch attached should be pretty much feature-complete, but probably will need to be postponed on all the entity ng conversions..
Comment #13
amateescu CreditAttribution: amateescu commentedI hope that's not a legitimate install failure, so let's try a re-roll on latest HEAD first.
Comment #15
catchThis should invalidate the cache tags, then it'll invalidate any cached item that's been tagged correctly (i.e. views as well).
I can't find the comment invalidation in the patch, but it'd make sense to me for comment_save() to invalidate the cache tag for the node as well as for the comment.
Comment #16
amateescu CreditAttribution: amateescu commentedThis is the part that invalidates all references.
Now that I look at it again, I guess it should only do this for new entities, because when we update an entity, the relevant caches should already be tagged with its id.
I'll do that in the next re-roll.
EDIT: Oh, and we also need a global switch for this. The current way of cache clearing is fine if you insert/update one entity, but it's really bad for bulk operations (e.g. Migrate, VBO).
Comment #17
amateescu CreditAttribution: amateescu commentedSilly me :/ This should get us past the installer again.
Comment #18
catchI'd forgotten comments reference nodes now :)
For clearing - in the implementation we might want to keep all the cleared tags during the request then clear them all at once.
Another option would be to do all the tag clearing only in the form submission callbacks (as we do for cache_clear_all()) - if you call $entity->save() from somewhere else you'd be responsible for clearing the tags too, or not.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commentedYes please. We can't have cache clears firing all over the place for data migrations and so on.
Comment #20
amateescu CreditAttribution: amateescu commentedThat's exactly what I said a few lines above :)
Now that we got into the fun territory of cache clearing, I have to say that I would prefer option 1) from #18 because, let's be honest, bulk operations and data migrations are the minority use case here, there are far more individual $entity->save() calls and I'd prefer to offer a better DX for this case.
How about something along the lines of
_menu_clear_page_cache()
?In the meantime, let's see how many fails we have left after I fixed some broken code in resetCache().
Comment #21
amateescu CreditAttribution: amateescu commentedActually, forget the pseudo-code from #20, this is what I mean.
Comment #22
catchFor end of request clearing we should do that in the storage controller (or if we split cache tag handling out of the direct storage controllers maybe in a base class for that), for me that makes sense for all tags not just entity ones.
Even if we do this though I'd still go for doing the tag clearing in the submit handler - keeping the tags around in memory is going to mean a massive query at the end of migration, more memory etc.
Comment #24
xjmComment #25
amateescu CreditAttribution: amateescu commentedRemoved auto cache clearing from the storage controllers and fixed a couple of other stuff. Let's see how we stand with tests..
Comment #26
amateescu CreditAttribution: amateescu commentedOne thing that worries me about not doing any automatic cache clearing is that we have to touch every single test that deals with entity view output. How about introducing a system.performance config variable for enabling entity render caching?
Comment #27
amateescu CreditAttribution: amateescu commentedDouble post.
Comment #29
amateescu CreditAttribution: amateescu commentedMaybe this won't be as bad as I thought.
Comment #31
tstoecklerI think throwing (and catching) an exception whenever saving a non-renderable entity is a bit costly. I think we should go the extra mile here of \Drupal::service('plugin.manager.entity')->getDefinition($entity->entityType()); and check whether that has a 'render controller' key. (Alternatively we could add a $nothrow option or something getRenderController() (or, actually, getControllerClass()))
Other than that, looks pretty sweet!
Comment #32
amateescu CreditAttribution: amateescu commentedI'll be away for a week so I'm posting my progress so far. One major pain point remaining is.. of course, comments :) More specifically: comments are cached individually, as they should, but they're also cached in the node's render array ($node->comments).
A solution for that could be to allow #attached callbacks to alter their $elements array. Then we could simply change
comment_node_view()
to do something like this instead:Re #31: From those two options, I would prefer the latter; introduce an extra argument to
get*Controller()
andgetControllerClass()
similar(/identical) to ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, ContainerInterface::NULL_ON_INVALID_REFERENCE and ContainerInterface::IGNORE_ON_INVALID_REFERENCE.Comment #34
amateescu CreditAttribution: amateescu commentedMerged HEAD and fixed a few more tests..
Comment #36
catchThis is at least major.
Comment #37
Wim LeersAgreed. Considering that ESI is a crucial part of getting D8 to perform well, I'd say it's even critical (assuming render cache and high render cache hit ratio, to achieve low amortized CPU time per served page).
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commented"Considering that ESI is a crucial part of getting D8 to perform well"
if ESI is a crucial part of getting D8 to perform well, then are we saying that the 99%* of sites that won't use ESI are just toast?
please, no, can we stop this right here? IMO, we can't ship D8 if the only way to make it perform well is to use ESI. or we can ship it, but wow, that is an admission of some very serious design fail with Scotch and Whiskey etc.
* i may have made that figure up, it's probably higher than 99%
also, Wim, i'm not picking on you :-) i've seen variations of this from lots of devs in the D8 cycle, this just pushed me over the edge.
Comment #39
effulgentsia CreditAttribution: effulgentsia commentedI think some of the confusion is from casual lumping of "enable Drupal render caching by default for blocks and entities" and "allow for caching blocks in a caching layer outside of Drupal" into "ESI". Technically, ESI is only a subset of the 2nd, and while I would still really really love to have core support it out of the box, I agree might not hold up a release if not done in time, and should not be required for decent performance, only for extreme scalability. The 1st, however, may end up being the only way to make D8 have not worse performance than D7, but fortunately, is available to all sites, not just the 1% using out-of-Drupal caching layers.
Comment #40
Wim Leers[offtopic]
#38: Hehe, no worries :) I feel more or less the same way — I'm just echoing what I've read elsewhere. As #39 indicates, it's more subtle than that though: the things that enable ESI also enable better caching *within Drupal itself*. I.e. ESI is about making blocks/entities etc. (as #39 says) individually retrievable and cacheable, so they can be integrated with super high-end, super expensive ESI solutions. But it also means you can leverage that same granular cacheability (and corresponding cache clearing) for doing "ESI" with your Varnish or nginx. And it also means that even without any of that, Drupal has to do less work per page load if we have this sort of granular caching working well.
So: it's about caching individual parts/components/pieces/blocks/… of a page. It'll have a big effect, no matter if you're using just Drupal, a reverse proxy in front of Drupal, or a very high-end CDN in front of Drupal.
I hope that made sense :)
[/offtopic]
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedso i read the patch, and wow, i'm really, really off-topic with my comments.
nothing to do with ESI in there, at all. it just collects tags from render arrays. +1 from me, and sorry for the noise.
Comment #42
BerdirHere's another re-roll.
Haven't really read how you want to solve the... "comment/nested-entity"-problem but that's going to be interesting :) The node contains a list of comments that might or might not be visible. But even if we add the cache tags for those to the node build array, there is still no way to detect if a comment is actually in there or not, we can just track explicit cache clears and then again for all users.
Maybe we need to build a cache key based on the contained entities, so that a different group of entities will be cached differently?
Comment #44
catchI'd probably have any comment addition/update clear the cache tag for the node ID. That'll force things like comment count in node teasers to be updated as well as listings like most commented. If we individually cache the generated HTML for each comment as well then rebuilding the full list of comments for the node is still going to be cheap-ish.
Comment #45
amateescu CreditAttribution: amateescu commentedThis should get us back to just a few failing tests: comment 'new' indicator and comment pager.
For the former, @catch opened #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user and the latter is the problem that @Berdir was referring to in #42. The problem with comment pager is that when we initially do nested caching on the first page, subsequent pages have no way of telling the render controller that we are displaying a different set of comments.
I also opened and patched #1965208: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem and #1996714: Convert FileItem and ImageItem to extend EntityReferenceItem so we can have a cleaner way of gathering entity ids from entity / taxonomy / field / image reference fields in
EntityRenderController::resetCache()
.Comment #46
amateescu CreditAttribution: amateescu commentedForgot the interdiff.
Comment #48
BerdirGiven that there are various problems with comments currently anyway (new mark, links, ..) could we simply disable the render cache on the entity-level if there are comments. There probably needs to be an easy way to disable caching anyway, not sure if we have that already?
Comment #49
amateescu CreditAttribution: amateescu commentedAdding cache clearing in
user_role_grant/revoke_permissions()
was a very bad idea, don't know what I was thinking. This one should really get us to only comment 'new' and pager fails.Re #48: I'm not sure what that buys us, the hard work is already done, so waiting for / fixing those two remaining issues doesn't sound too bad to me..
Comment #51
amateescu CreditAttribution: amateescu commentedStraight up reroll. Still waiting for those comment issues to be fixed...
Comment #52
Wim LeersThis function is of the *utmost* importance. It's short, it's simple, but it is also the basic building block by which render caching stands or falls.
I think it's essential to have unit test coverage for this.
Playing the devil's advocate: are we absolutely certain we can assume this in all cases?
Or, which I think is more likely, we are defaulting to this, but modules/entity types can override this default.
IOW: I should interpret this as a default?
{@inheritdocs}
I understand this is necessary, but… it's not very nice to see a specific field type check in here. Is there no other way?
A comment here would be nice.
"Add 'new' anchor if needed (and if so, disable render cache)."
?
These are changes to keep existing tests working; ideally we'd also have tests to prove that the entity render cache is working.
This is again for entity reference fields?
Again.
But why is it this time not necessary to check the type or some other property? The code reads like it's being applied to all fields, yet the comment says differently.
Why? Why not set a short expiration time to prevent filling the cache endlessly?
Comment #53
amateescu CreditAttribution: amateescu commentedAbout testing: yeah, even though the patch is thoroughly tested already by existing tests, I agree that we need some that are dedicated to this functionality.
Every entity type can override
getBuildDefaults()
in its own render controller, and the render array built here can be altered in a lot of places.Yeah, this patch was started long before that policy and needs to be updated. And it's {@inheritdoc}, without S.
As I already said in #45, I opened and provided patches for all reference-like fields to have a target_id property (instead of 'tid' or 'fid'). After that, we will just need to check if a specific field is an instance of
\Drupal\Core\Entity\Field\Type\EntityReferenceItem
(not the configurable one provided by the Entity reference module).This has nothing to do with entity reference fields? $targetEntityType is a property of the EntityDisplayBase object.
And again, this has nothing to do with entity reference! All reference-like fields (entity ref, taxo ref, file, image) have the 'entity' property for their items already in HEAD.
And that short lived cache entry would help.. who exactly? Not to say that we'd have to take it into account in the cache key, which will special case entity types that are revisionable.
Comment #54
Wim LeersI'm a relative outsider to Field API and figured it'd be useful to have a pair of eyes look at this that doesn't know what else has happened. Apologies if my review was frustrating to you — just trying to help.
I missed #45. Hence most of my reference field remarks are indeed incorrect, with the exception of the one on this piece of code I think:
That one is *not* generic, right?
I'm just questioning the assumptions in this patch, that's all! This is probably the sanest default. But everywhere where assumptions are embedded, especially WRT caching, IMO the comment should convey *why* this is the sanest default: not only the *what* but also the *why*.
Comment #55
amateescu CreditAttribution: amateescu commentedYep, that's the one that is not generic and that the issues/patches mentioned in the last paragraph of #45 will address.
Fair point that we need an extra comment there to explain why we're only caching the current (active) revision and not all others. Will do that along with all your other remarks in the next iteration.
Thanks for trying to help and sorry for the tone.. :)
Comment #56
effulgentsia CreditAttribution: effulgentsia commentedRaising to critical, because while we can still optimize some of the OOP overhead we've added to D8 in general, and to Entities in particular, I don't think we can optimize uncached entity rendering to be faster than it was on D7. Happy to be proven wrong, and if we do manage to optimize sufficiently before getting this in, I'd be ok with this then being downgraded back to major, but I suspect we'll get this in first.
Comment #57
catchAgreed on critical.
Comment #58
Wim Leers+1
Comment #59
Caseledde CreditAttribution: Caseledde commentedThere are a lot of per-role, per-user and per-request aware fields (like Fivestar, Flag, etc). There already is an issue for the 'new'-mark on comments: #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user.
If we cache an entity, we have to be aware of those fields / extra_fields.
In the context of #1875974: Abstract 'component type' specific code out of EntityDisplay we are able to handle all fields in a different way.
So here is a resolution propose:
In this way, we are able to provide context aware link sections, 'new'-marks for comments etc. without touching the cache granularity of the parent entity.
Comment #60
catchFivestar, flag etc. should move to the same mechanism that edit/contextual module are using in core - which doesn't require changing markup based on the current user role (those checks are done via AJAX requests and replacement). This is about the same as what you're suggesting, except that the replacement is done in JavaScript rather than PHP. It's not possible to rely on PHP replacement due to reverse proxies.
Comment #61
moshe weitzman CreditAttribution: moshe weitzman commentedThat AJAX technique is ideal. If not possible for some reason, those modules just need to add cache_tags for each role/group/whatever. The cache system handles retrieving the right cached HTML after that.
Comment #62
catchCache tags don't affect cache IDs at all, only invalidation.
Since #cache is set on the render array, anyone altering the render array ought to also be able to alter the cache ID/cid parts though so as long as that's exposed properly it's an option for modules that can't do js-replacement for some reason.
Comment #63
moshe weitzman CreditAttribution: moshe weitzman commentedOops, yes thats what I should have said. Thanks.
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedYes, but providing a field type doesn't involve altering the entity render array directly. Rather, field.module sits as an intermediary for that. Creating some API for field types to inform field.module of their cache granularity and then letting field.module alter the #cache of the entity render array sounds like a nice idea, but I think can be a noncritical, post-API-freeze follow up, since it would be a small API addition, not a BC break. If not done in time for D8 core, then field type modules can fall back to implementing the corresponding alter hooks themselves.
Comment #65
Caseledde CreditAttribution: Caseledde commentedThere is no need to change the cache granularity of the entity because of a single field. If we do, we still have issues like #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user or #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links. If we use tokens to be replaced in the rendered entity, we do not need granularity for the entity. For example, the granularity for entites comes in handy, if we have to be aware of field access. The we can use roles as granularity for the entity. But we are still albe to cache the content of a single field separatly and just decide to display or not. No rendering needed anymore.
The second point is, to return the cached html before the renderable array will be build. We cache the html, because it is not necessary to rebuild it again and again. The renderable array should'nt be rebuild as well. In this way, we can't alter the render array after the entity was cached, because the alteration will not be called anymore.
So we need an API to enable field developers to return markup during the rendering of an entity, which will be cached, and during the display of this entity, if the field displays dynamic content.
If we can provide an generic API to tag a field component with attributes like
The field API has to call this methods during the rendering of the entity and during the display via ajax to replace the generic tokens build by the field module by react of the 'Field Granularity'-tag.
Comment #66
Caseledde CreditAttribution: Caseledde commentedUpdated patch from #51. It was out of date and not longer applyable.
Comment #67
Caseledde CreditAttribution: Caseledde commentedInterdiff between #51 and #66.
Comment #68
catchWhat we should likely do is collect the cid parts recursively in the same way that #attached and #cache_tags are - then adding cid parts to a field formatter will affect the entity rendering as well.
Comment #69
catchExcept that's a completely stupid idea from me in #68 - it's OK for cache tags and #attached 'cos you can combine the result of whatever was cached, but cid parts you need to know before you even request from cache, so yeah that does need to be calculated up front, or otherwise circumvented.
Comment #70
Caseledde CreditAttribution: Caseledde commentedNeeds review to run testbot.
What should our next steps be? I can imagine, that my proposal is al little bit big, but i think we have to do something anyway, so I think we can do it in a generic way right away.
Comment #72
tstoecklerRe #70/@Caseledde: I'm not sure I've understood your #65 100% correctly, but either way I think we all agree that getting this patch in as is would be a great step forward.
I agree with you and @effulgentsia in #64 that - even if we enforce modules to strictly only return markup that is dependent on the set of available roles - we should provide facilities that help with doing per-user-markup-but-only-via-ajax without having to re-invent the heavy lifting each time. That should definitely be discussed in a follow-up issue, though.
I don't know in fow far #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash is related, also.
Caching the render arrays themselves, instead of building them each time, is very interesting idea, that I haven't seen discussed in the core queue so far. The granularity question is the same one there, but if we want to enforce role-based markup that needs to happen on a render array level anyway. I've seen promising benchmarks based on https://drupal.org/sandbox/Caseledde/1970904 that this is in fact worth investigating, but again I would suggest you open a follow-up once this is in.
Comment #73
Wim Leers#2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash is essential for client-side caching of these AJAX-retrieved, user- or role-specific pieces of content. Otherwise, these have to be retrieved every single time, as is the case right now in Drupal core (i.e. for rendering contextual links and in-place editing metadata).
Comment #74
tstoecklerAhh, that makes perfect sense. Thanks for explaining that!
Comment #75
msonnabaum CreditAttribution: msonnabaum commentedShould fix comment and filter fails.
Comment #77
msonnabaum CreditAttribution: msonnabaum commentedHere's a new one that should fix more comment tests.
It takes a different approach than previous patches in that it adds the logic directly into the entity rather than the form/render controllers. The logic of when an entity changes and what entities are related to that entity are firmly within the responsibilities of the entity itself, so it makes sense to handle it there.
I added two methods to entity, changed() and relationships(). The names aren't great, but they express the basic intention of declaring that something changed and getting a list of related entities that need to change as well.
Comment #78
mikeytown2 CreditAttribution: mikeytown2 commentedComment #79
msonnabaum CreditAttribution: msonnabaum commentedIgnore last patch.
Comment #80
amateescu CreditAttribution: amateescu commentedNo no no, no :P This was also my original approach until #18, where @catch and @moshe weitzman convinced me that clearing caches every time an entity is saved is the wrong thing to do.
I think relationships() is a good concept to have in general (maybe getRelatedEntities()/Data() would be a better name?), but changed() needs to go :)
P.S. An interdiff would be very helpful to see exactly what was changed..
Comment #81
effulgentsia CreditAttribution: effulgentsia commentedRelated: #2040135: Caches dependent on simple config are only invalidated on form submissions
Comment #82
msonnabaum CreditAttribution: msonnabaum commented@amateescu - I spoke with Moshe yesterday and he no longer thinks this is necessary. Perhaps catch still feels this way, but I just dont see how it's sustainable at all. If we're leaving invalidation up to the controllers, we're violating the boundaries of the entity. Only it can truly know when it has changed, because it's the owner of it's save/delete methods.
The remaining failures are still quite unique. We haven't solved the basic problems of this patch, so I think we should focus on doing whatever will get these cases passing.
Comment #84
damiankloip CreditAttribution: damiankloip commentedHere a patch with some earlier work in. I have moved the new methods into Entity instead, as we will need to use this cache invalidation with config entities too; such as when a field instance is updated, all entities that it is attached to need to have their cache invalidated.
Comment #85
amateescu CreditAttribution: amateescu commented@damiankloip, what do you mean by "earlier work"? Earlier from a previous patch from here? Which one?
Doesn't anyone use interdiffs these days? :(
@msonnabaum, if we go with that line of thinking, then we also need to move the static cache invalidation out of the storage controller? And we'll have to get back to the global switch idea as well: "Hey, I'm trying to do a migration here, stop messing with all those caches." ... (hard work on the db) ... "Ok, I'm done. Carry on and clear everything."
Comment #86
msonnabaum CreditAttribution: msonnabaum commentedSorry, I wasnt posting interdiffs because it kept telling me they were impossible and producing nonsense.
And sure, it might be nice to have the global switch thing, but IMO it's a total distraction from the very difficult problems in the current fails. Lets fix those first.
Comment #87
msonnabaum CreditAttribution: msonnabaum commentedComment #88
amateescu CreditAttribution: amateescu commentedThat's what I've been saying for a couple of months already (since #45) :)
I suspect that no one actually read the previous comments, so I'll say it again. There are three problems:
- comment 'new' marker -> easy to fix as we already have and established pattern of how it should be done
- comment links -> @catch and @Wim Leers discussed this in another issue, can't remember which one atm
- comment pager -> see the comment in #45, nfi how to fix it..
Can we please postpone this one and stop chasing our tails? :)
Comment #90
msonnabaum CreditAttribution: msonnabaum commentedThere are a lot more than 3 remaining problems in this patch…
Wim had a workable solution for the "comment new" patch today. Hopefully he'll post something soon in the other issue.
Here's an inelegant solution to the pager issue. Hopefully we can do better, but this should work for now.
Comment #92
damiankloip CreditAttribution: damiankloip commentedI think the ViewsUI class is ruining the party at the moment, with INTERDIFF...
Comment #93
catchThis shouldn't be necessary.
If an entity (say a node) gets rendered with a reference field (say taxonomy terms), the entity reference formatter should add cache tags for those entities to the render array. Since tags are collected recursively, those term IDs then end up in the cache tags for the node render, and if any term ID gets updated, the rendered node cache gets invalidated.
This looks like it's doing it the other way 'round - when a term references a node, delete the cache tags for the node, but that's not necessary and a lot more complex.
Comment #94
amateescu CreditAttribution: amateescu commented@catch is correct in #93, the latest iterations introduced a lot of unneeded complexity in this patch. I'm working to get it back to a sane state.
Comment #95
msonnabaum CreditAttribution: msonnabaum commented@catch I added that to fix the issue where a node_view cache wasn't updated when a new comment was added. Since the comment is new, the node was never tagged with it. Not sure how else to handle that.
Comment #96
amateescu CreditAttribution: amateescu commentedThis gets us back to more or less what we had in #17 (yes, five months ago), before the madness with cache clearing in form controllers started.
The first interdiff is the 'get back to a sane state' part, and the second one is starting the 'clear caches on save and delete' path.
Comment #98
catch@msonnabaum that makes sense now.
'm not sure if it makes sense for anything other than comments though - they're the only entity that references an entity, then gets rendered by the entity they reference, other entity references work backwards from that - or don't get nested within the referenced entity (i.e. node listings on taxonomy terms aren't within the entity render).
Comment #99
amateescu CreditAttribution: amateescu commentedAs discussed in IRC, that case should already be handled by the logic in resetCache() from the render controller, which should pick up the referenced node by looking at the 'nid' property of the new comment. Mark is working on merging the patches now.
Comment #100
msonnabaum CreditAttribution: msonnabaum commentedOk, here's a merging of the two patches. I kept the methods on the entity because think it's a bit cleaner, but we can move the actual cache tags clearing out eventually. I think an event listener might work best.
Comment #102
msonnabaum CreditAttribution: msonnabaum commentedComment #103
msonnabaum CreditAttribution: msonnabaum commented#100: 1605290-100.patch queued for re-testing.
Comment #105
amateescu CreditAttribution: amateescu commentedAfter our discussion on IRC about this, I still don't see why clearing these tags is the responsability of the entity object, since the render controller is the one that defines the cache bin and only it knows how to handle its own cache...
Comment #106
msonnabaum CreditAttribution: msonnabaum commented#100: 1605290-100.patch queued for re-testing.
Comment #108
msonnabaum CreditAttribution: msonnabaum commentedRe-roll.
Comment #109
msonnabaum CreditAttribution: msonnabaum commentedComment #111
msonnabaum CreditAttribution: msonnabaum commentedThought the bot was failing, was a dumb mistake.
Comment #112
msonnabaum CreditAttribution: msonnabaum commentedAlso, I don't entirely disagree with #105, I'd just rather leave any more refactoring until after we're closer to passing.
Comment #114
amateescu CreditAttribution: amateescu commentedThe purpose of the original code from EntityRenderController (where the comment is actually correct about what the code is doing) was to get entity ids only, not full entity objects. Since $property->entity is a computed property, this means that we're now loading a bunch of entities for no reason, since we only need their ids for cache clearing.
This is not how cache tags work, the second dimension of the array needs to be keyed by entity ids.Edit: Bah, I'm not sure that's true, I might have keyed the array for different purposes.Comment #115
tstoecklerI haven't dug into EntityRenderController::getBuildDefaults() so sorry if this is obvious from the context, but: Why would $this->viewModesInfo[$view_mode] not be set?
It seems this is a merge conflict. The file.full view mode was recently removed in the view mode UI issue.
Same here.
Comment #116
amateescu CreditAttribution: amateescu commentedThe thing is.. that's not refactoring, it's just coming back to the "default" state of this patch. Here's how it should work if we want to keep relationship gathering in the entity class. The only big change is that resetCache() does not need full entity objects anymore, just ids.
The rest of the interdiff is just cleaning up bad rerolls that accumulated mistakes over time.
Beside the usual comment failures, at least Drupal\serialization\Tests\EntitySerializationTest (probably Drupal\taxonomy\Tests\TermTest too) fails because of #114.1.
@tstoeckler:
Because some tests (mostly in Views IIRC) like to request non-existent view modes and I didn't feel like fixing everything in here, so I chose the easier way out :) We can try to remove that condition in a patch testing issue if you want to see exactly what was wrong.
Edit: I intentionally left that debug() in there to prove that a test is failing because of needless entity loading.
Comment #118
tstoeckler@amateescu: Ahh, that's interesting, thanks. I guess a simple comment/@todo would be enough to satisfy me, for now.
Comment #119
amateescu CreditAttribution: amateescu commentedThis should fix all DUTB tests, let's see what's left..
Comment #120
amateescu CreditAttribution: amateescu commentedOops, that was only the interdiff.
Comment #122
amateescu CreditAttribution: amateescu commentedKeeping up with HEAD.
Comment #124
amateescu CreditAttribution: amateescu commentedComment #125
dlu CreditAttribution: dlu commentedMoved to entity system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).
Comment #126
amateescu CreditAttribution: amateescu commentedRerolled and disabled the render cache for nodes and comments. Let's see what happens.
Comment #128
Anonymous (not verified) CreditAttribution: Anonymous commentedhopefully this fixes the two fails.
Comment #129
dixon_I hate to come in late in this issue, but it feels wrong to have
relatedEntities()
inEntityInterface
. To me it feels like we should create a separate service for this, or maybe tuck it intoEntityManager
(but I'd prefer a separate service I think). This code shouldn't need to be extended per entity type really, it should be fairly generic, no?It would also be quite useful to have the related/dependent entity logic in a service so that contrib easily can extend it independently of each entity class. We need something like this for content staging in Deploy module. In D7 I created Entity Dependency module for this.
Comment #130
amateescu CreditAttribution: amateescu commented@dixon_, that is indeed a good point. I would prefer to see it in EntityManager rather than a separate service, and I'm sure others would like it to stay in EntityInterface. Given that, are you ok with discussing this in a followup? This patch has been through too many bikesheds already..
Documented the new methods from EntityInterface, so I think we're finally ready for a rtbc here :)
Comment #131
Wim Leerss/ ,/, /
And more importantly: making the entire query string part of the key is problematic, because it makes it easy for a malicious user to trigger the generation of new cache entries, thus always hitting the server *and* filling the cache with identical cache values with different keys … leaving no room for "proper" cache entries.
I feel this was discussed before, but maybe that was a separate issue.
Comment #132
amateescu CreditAttribution: amateescu commentedThat was added by Mark in #90 as a 'dirty fix' for comment paging, but since we disabled the cache for nodes and comments, we don't need to worry about it in this patch.
Comment #133
catchThis is no different with the page cache - we cache 404s in the page cache as well.
Sites that are concerned about this should use a cache that handles LRU or similar.
Comment #134
Wim LeersWow. I see it in the patch now. Is this just temporary, until #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user is fixed? The test results of #122 — the last patch before you disabled this, seem to confirm that that *is* the main reason?
If node/comment render caching is disabled, then why are lines like these necessary?
Can't this be done in
statistics_node_view()
?Finally, and most importantly: I don't see any additional test coverage. Wouldn't it make sense to do a full render cache life cycle on an entity? I.e. create it, view it, view it again and ensure that it's served from the render cache, modify it, ensure that it's re-rendered.
Comment #135
amateescu CreditAttribution: amateescu commentedYep, we want to get the initial patch in and then work out everything needed for nodes and comments.
Because I'm dumb :/
We don't have access to the $build array there.
And yes, I guess we need some dedicated tests using the entity_test entity type.
Comment #136
amateescu CreditAttribution: amateescu commentedRerolled after #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity.
Comment #137
Wim LeersHaving discussed this with amateescu in IRC, this is currently the only thing remaining.
Comment #138
Wim LeersFor #1991684-40: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, I've reviewed this patch in depth.
Overall review
drupal_render_collect_cache_tags()
, because the render cache's reliability depends on it.drupal_render()
is indeed using the render cache on entities, and that saving an entity clears the corresponding cache entries.cache_render
. It's reasonable to say it should be a follow-up, but at the same time it is this patch that will cause the render cache to be used much more widely. It's a small change. If you choose not to do it here, then please create a follow-up.Code review
Link to an issue?
What about other offsets? If we don't care about other offsets, then there should be a comment explaining why.
$related_entity_ids
is first initialized to contain the entity whosechanged()
method is being invoked.Then, the method iterates over all related entities and adds those to
$related_entity_ids
.Finally, we iterate over
$related_entity_ids
by entity type, clearing all render cache entries for entity IDs of a given entity type.So, clearly,
$related_entity_ids
is the set of all affected entities whose render cache entries should be cleared.Hence I think
$related_entity_ids
should be renamed to$affected_entity_ids
.The
$tags
parameter here should go away, I think this is a leftover from earlier patches?Like catch in #93, I'm very confused why this is necessary. If an entity is changed, then all cache entries containing that entity should be cleared, and nothing else. E.g. when creating a node, why should the author's profile page be cleared?
But then I read this in #11:
In the first case, I would say it is the comment entity's responsibility to know on which entity it was posted and thus the comment entity should just clear the render cache entry for the entity it is for.
This is also what #44 says.
But in the second case, it's a lot harder: displaying all nodes that reference a taxonomy term is displaying the inverse of the relationship "ENTITY references TERM", which is the one that we really are manipulating.
So, without having stored "backreferenced", we may still have those (and it's easy create those using Views).
We want to be certain that we don't render any stale content/state, so rather than demanding optimal (minimum) cache clearing, we cast a wide net to make sure everything that might be affected, is cleared.
I think that is fine for this initial version — we don't want to make this too complex.
So, while I think this can be improved, I also think that this is the simplest possible solution that can work. Even though it may be suboptimal, a more optimal solution is likely to bring overhead with it as well. KISS.
One thing that I'd like to see changed though is the method name. "related" is a very broad term. "referenced" is much more narrow, precisely describes what happens, and is actually the term used in the comment.
So let's rename
relatedEntities()
toreferencedEntities()
?Note that
\Drupal\Core\Entity\Entity
implementsComplexDataInterface
, which already has aonChange()
method. At the very least, it is confusing to see bothonChange()
andchanged()
on\Drupal\Core\Entity\Entity
.This function is deprecated.
\Drupal\Core\Entity\EntityManager::getDefinitions()
should be used directly.It'll be possible to manipulate
#cache
to suit your site's specific characteristics (e.g. to change granularity) by usinghook_entity_view_alter()
.Good.
Can use a comment.
Can use a comment.
Comment #139
amateescu CreditAttribution: amateescu commentedtl;dr version: add tests, a few comments and rename two methods :P
changed()
andrelatedEntities()
are not mine in this patch, so let's try this first.Comment #140
amateescu CreditAttribution: amateescu commentedIt seems I offended Moshe with the issue assignment.nevermind me :)Comment #141
sdboyer CreditAttribution: sdboyer commentedso while i'm gonna ultimately concede, i find this horrifying, so i'm gonna raise the issue anyway.
the fact that we are tying contained/referenced entities in to choices about the render cache for the referencing entity is nuts. if this were a system for caching the raw data of an entity, that would be one thing: one could make an argument for caching the entity + its references together as an aggressive denormalization technique, justified by the fact that the consumers of that cached data actually MAY have a need to access anything in that composite datastructure.
however, in the case of caching rendered output...ugh. no. this is a really unfortunate intermixing of data with presentation; the presentation layer should know precisely what data its referencing, rather than having to fall back on this global, wildly oversensitive strategy. this is what you get when you mix data with presentation.
Wim comments that he's fine with this for an initial version. i guess i am, too. i can't disagree that this seems like the simplest possible solution that will work. but i think a more complex, granular solution that is also really reliable won't be possible until we run the hell away from this tight coupling of rendering logic directly with the entities themselves. that's what presentational elements are for - like say, blocks!
Comment #142
amateescu CreditAttribution: amateescu commentedEither you haven't actually read the patch or I don't understand this comment at all :)
Of course the presentation layer knows what data is referencing, that's how it gathers the cache tags. The snippet you pasted is only used for cache invalidation.
Comment #143
Wim Leers#141:
So it seems almost everybody is confused by the
changed()
andrelatedEntities()
. Note that they have been changed/fixed, so earlier comments in the issue are not relevant anymore — I got confused by this too.In an attempt to bring everybody on the same page, I stepped through a whole bunch of scenarios that will hopefully make cache tag clearing the situation more clear.
The render cache entries for the listed entities will be reset. In each case, we want to make sure that simply all referenced entities are listed — that is the goal of this issue.
Value of
$related_entity_ids
inEntity::changed()
when creating or editing a …1. Term
2. "article" Node, no tags, no comments yet
3. "article" Node, one tag, no comments yet
4. "article" Node, two tags, no comments yet
5. Comment on Node that references two Terms
(Note that the the Terms referenced by the Node are not listed, which is correct.)
6. Comment on Comment (i.e. comment reply) on Node that references two Terms
(Note that the the Terms referenced by the Node are not listed, which is correct.)
7. "article" Node, two tags, has one comment
(Note that the Comment on the Node is not listed.)
Conclusion
Everything is working as intended AFAICT.
BUT!
Case 1 makes sense.
But in case 2: why the User? Because any "nodes by User X" listing will only have cache tags for the specific nodes (e.g. node:x, node:y) in the listing as well as the user's cache tag (user:a).
Concern: this will clear any render cache entry by that user, the majority of which probably has not changed. I.e. many false positives. To reduce false positives, we could introduce " references " cache tags, so: "user:a references node:article". If we'd then clear that cache tag, we'd reduce the number of false hits significantly.
In other words: more granularity could reduce false positives. We can do that by not only having cache tags for a single entity, but also having cache tags for relationships. Because only those things that depend on that relationship will be affected.
Similarly in case 3: Not clearing the "user:a" tag but clearing "user:a references node:article" would be better.
And not clearing the "term:b" tag but clearing "term:b references node:article".
Similarly in cases 4–7.
Thus: we can reduce false positives, but that should be a follow-up issue, because we have to establish clear rules for that. Preferably it'd be auto-generated somehow (maybe as part of
EntityQuery
?), to prevent human errors.I did fix one bug that I reported: #138.2 — otherwise the results wouldn't have been correct.
Comment #144
amateescu CreditAttribution: amateescu commentedHow about KISS rather than overcomplicating everything with that "references" cache tag? Anyway, as long as it's discussed in a followup, I won't complain.
Comment #145
sdboyer CreditAttribution: sdboyer commentedchanging status so Wim's patch gets reviewed.
re: #142 - yeah, i chose a weird snippet to focus on for that comment. it was at least two layers removed from my actual objection, sorry. i blame the several beers i had just imbibed :P
@Wim Leers -
yeah, my objection here is really outside the scope of this issue - i dislike view modes being tightly bound with the entity object itself, and would prefer that they exist in a separate (presentational) layer...where we'd have essentially the same logic as what's here. i should have bitten my tongue.
your explanation in #143 is good, though - i had been under the impression that the clearages were more pervasive than they actually are. seems like they're actually much more targeted, which is great.
so, carry on, +1.
Comment #146
Wim Leers#144:
Indeed, KISS here. I merely explained how it could be more efficient, but it should only be done in a follow-up, not here.
#145: Yay — glad to read #143 was not in vain.
SO: the remarks in #138 should be addressed, then this should be RTBC.
Comment #147
catchWith updating a user clearing the node, yes that's tricky. Often the only thing about a user that appears on the node is the username and maybe the image, but we have no way to link changes to specific fields with invalidating the tags (unless you removed the tag in custom code then added it back conditionally).
On the other hand, if a user is deleted and their nodes get reassigned to anonymous, or they change their username and/or the path alias to their profile page is updated, then you definitely want all the nodes they ever posted invalidated - and this is still considerably better than the Drupal 7 equivalent.
While I'm here a reminder to me that we want to add cache tags to the page cache (based on all the cache tags in all the cached items for that request) so we can clear that the same way - but that's a follow-up enhancement once this is in.
Comment #148
BerdirAs discussed, it's not safe to override save(). See #2078517: Document that entity business logic belongs in preSave(), postSave(), preDelete() and postDelete(), not save()/delete(). Let's call this in postSave() and postDelete(). We might need to ensure that existing implementations of those methods call the parent implementation.
Comment #149
msonnabaum CreditAttribution: msonnabaum commentedNew patch that resolves some merge conflicts and addresses some of the concerns above.
I strongly dislike the idea that we can't guarantee that save() and delete() get called on the entity, but I went ahead and made the changes in #148 since anything else will require entity api refactoring.
I changed relatedEntities to referencedEntities. The original was named without realizing that all related entities are now entity references, so I agree this makes more sense.
I didn't change entity_get_info to EntityManager::getDefinitions(). Really, that data should be passed in to the render controller, we don't need to add a reference to the manager here.
The name conflict with changed() and onChange() is troublesome, but I don't quite know what to do about it. Typed data is just taking a useful method name from a subclass yet again. Any name I think of is worse and less descriptive. I honestly don't care though at this point, so whatever someone else wants to come up with I'll probably be ok with.
Comment #150
msonnabaum CreditAttribution: msonnabaum commentedComment #152
Wim Leers#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user got committed, which means the hacks/work-arounds/limitations in this patch for comments can be removed; full entity render caching should now be possible! :)
Comment #153
catchI don't think that's true yet, there's still edit/delete links on the comments themselves.
Comment #154
Wim Leersamateescu just pointed that out on Twitter too. Stupid that I'd forgotten about that :( I hadn't heard/read about that in months, that's probably why. I actually can't find an issue for it — can you point me to it?
Comment #155
catchI can't find it either. If I remember correctly there was an issue to add back contextual links to comments which was discussing all the links and how viable it'd be to use contextual links for them, but it's not coming up in search.
Comment #156
amateescu CreditAttribution: amateescu commented@Wim Leers, I don't think there's an issue for that.. yet. I googled a bit and I found where this topic was discussed between you and @catch: #1882482-105: [meta] Unify editing approaches in Drupal.
Anyway, I'll spend some quality time on rerolling and fixing this patch.
Comment #157
catchThat's the one!
Comment #158
amateescu CreditAttribution: amateescu commentedThe first interdiff has fixes for the test failures in #149 and the second one is for test coverage.
I added separate tests for
drupal_render_collect_cache_tags()
and for the whole entity render cache flow. Also found a bug in the process where new (unsaved) entities were generating a cache entry, and they shouldn't since they might not have an ID yet.I think we have everything now and we're ready for final reviews. One thing that still bugs me is the
changed()
method, which doesn't quite fit the scope of this issue.Comment #160
amateescu CreditAttribution: amateescu commentedAnd last round of fixes. All this wouldn't be necessary if we wouldn't load entities when we just need their ids.
Edit: I also opened a spin-off issue because this patch fixes it only for taxonomy terms: #2087995: All entity post*() and pre*() methods should call their parent implementation
Comment #161
Anonymous (not verified) CreditAttribution: Anonymous commentedusing drupal_static() and return flags in drupal_render_collect_cache_tags() looks a bit suspect. perhaps something like this:
in drupal_render_collect_cache_tags(), why do we care about 'first non-array value for a tag namespace wins'? reading the code, it seems we care, but i have NFI why.
also, why do we munge arrays for a given tag namespace into arrays with identical keys and values? reading the code, it seems we care, but i have NFI why. we test for that type of array, but we pass in
'tags' => array('render_cache_tag_child' => array(1 => 1, 2 => 2))
. if the munging is important, perhaps pass'tags' => array('render_cache_tag_child' => array(1, 2))
and test forarray((1 => 1, 2 => 2))
in the output.why do we need $reset here? if calling code wants to reset the cache, they should call resetCache() on the render controller themselves IMO.
put $view_mode_is_cacheable first in the condition, seeing as we went to all the trouble to calculate it...
Comment #162
dixon_Re: #153, #154, #155 and #156, I created #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user.
Comment #163
amateescu CreditAttribution: amateescu commentedRe #161:
drupal_render_collect_attached()
, but I don't know the historic reasons for that.Because
entity_load()
has it :)Patch size is smaller because the Entity BC decorator was removed in the meantime.
Comment #164
amateescu CreditAttribution: amateescu commented@catch said in IRC that the reason for drupal_static() is probably bogus (and related to some crazy stuff going on with the collection at the time) so I replaced it with the code from #161 which is nice indeed.
Comment #165
Anonymous (not verified) CreditAttribution: Anonymous commentedcoolio, i think this is RTBC.
Comment #166
Anonymous (not verified) CreditAttribution: Anonymous commentedwoops. we need some comments explaining why we mess with the tags array.
after that, rtbc.
Comment #167
amateescu CreditAttribution: amateescu commentedDone!
Comment #168
Wim LeersI thought in #152 that having fixed #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, this issue could finally enable render caching on nodes & comments. But sadly, comment ops links prevent that. For stopping comment links from breaking render cache, see #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user.
So this patch only brings render caching for other entities, like Taxonomy Terms. That's still a big step forward though.
+1 for RTBC on overall status of this patch.
I do have some remarks though: one potential problem (which could be fixed in a follow-up), but mostly @todo nitpicks. So I'll leave this as RTBC to let a committer decide — I don't want to hold up this patch.
It'd be better if this linked to an issue.
Shouldn't the bundle be included here? The per-view mode entity display config is stored in files like
entity.display.node.article.teaser.yml
. Right now, cache entries with a different view mode than the one affected (changed) would be deleted/invalidated as well.(Discovered while working on a talk about D8 performance — see http://wimleers.com/talk-really-fast-drupal-8/.)
I'm not sure if this should be solved here or elsewhere, but it feels strange to add a question as a todo.
Should get a @todo pointing out that fixing https://drupal.org/node/2090783 should enable comment & node render caching?
Comment #169
alexpottPatch no longer applies.
Comment #170
amateescu CreditAttribution: amateescu commentedRerolled and adressed #168:
Comment #172
Wim LeersThanks for the clarifications. I think the two exceptions are unrelated to this patch, so re-testing.
Comment #173
Wim Leers#170: 1605290-170.patch queued for re-testing.
Comment #175
Wim Leers#170: 1605290-170.patch queued for re-testing.
Comment #177
amateescu CreditAttribution: amateescu commentedRerolled and fixed those new exceptions.
Comment #178
amateescu CreditAttribution: amateescu commentedAnd here's the correct interdiff :/
Comment #179
catchThought it was strange that the comment change fixed the exception ;)
I'll try to commit more or less as soon as it's back green from the bot. Has been RTBC for five days excepting re-rolls.
Comment #181
Wim Leers#177: 1605290-177.patch queued for re-testing.
Comment #183
Fabianx CreditAttribution: Fabianx commented#177: 1605290-177.patch queued for re-testing.
Comment #184
catchOpened #2094241: Cache tag the page cache.
Comment #186
Anonymous (not verified) CreditAttribution: Anonymous commentedin case this helps someone who has a clue about Entity/Fields API, the fails happen here:
leads to this:
PHP Fatal error: Unsupported operand types in /var/www/8-drupal/webroot/core/modules/system/tests/modules/entity_test/entity_test.module on line 510
i guess we had some API change and we need a new incantation now?
Comment #187
Anonymous (not verified) CreditAttribution: Anonymous commentedderp derp. i missed the obvious bug here:
if ($entity_type = 'entity_test_render') {
is missing a '=', and was introduced over in #1778122-120: Enable modules to inject attributes into field formatters, so that RDF attributes get output. i've posted a patch there, though unfortunately it doesn't fix the fail here.
Comment #188
Anonymous (not verified) CreditAttribution: Anonymous commentedhere's an updated patch that fails in a way that someone better able to shave entity system yacks than i may be able to fix.
Comment #190
Fabianx CreditAttribution: Fabianx commented#188: 1605290-188.patch queued for re-testing.
Comment #192
amateescu CreditAttribution: amateescu commentedFigured out the problem with @yched.
Edit: deleted the bad rebase comment, ignore me :)
Comment #194
amateescu CreditAttribution: amateescu commentedIt was a bad merge after all, there's one thing that I needed to add back in the test.
Comment #195
BerdirLatest interdiff was lost in a re-roll and was already there before. The actual fix for EntityViewControllerTest looks good.
Comment #196
catchVery happily committed/pushed to 8.x.
Could use a change notice - how this gets enabled/disabled for entity types and to introduce the change in general. We should also cross-reference [#2086767] for how to not break this.
Comment #197
amateescu CreditAttribution: amateescu commentedYAY!
Here's the change notice: https://drupal.org/node/2095167
Comment #198
jibranYay!! Thank you everybody for working on this. Thanks @amateescu for change notice it makes a lot of sense.
Comment #199
Wim LeersGreat change notice, amateescu — thanks for your hard work on this!
Comment #200
Anonymous (not verified) CreditAttribution: Anonymous commentedlooks like we accidentally landed the $reset param to entity_view_multiple and entity_view ?
attached patch fixes that.
Comment #201
jibranCan we please do it in followup issue? We are already at #201.
I think we have to remove this as well.
Comment #202
Anonymous (not verified) CreditAttribution: Anonymous commentedwoops, good catch.
Comment #203
amateescu CreditAttribution: amateescu commentedNope, that was not by accident. See #163 and it's also in the change notice.
Comment #204
Anonymous (not verified) CreditAttribution: Anonymous commentedok, i'll open another issue. "because entity_load() has it" is complete rubbish IMO.
a load call shouldn't reset a cache, period.
Comment #205
yched CreditAttribution: yched commentedI just noticed the Entity::referencedEntities() method that got added here. It currently relies on loading entities in separate single loads, which sounds pretty bad ?
#2073661: Add a EntityReferenceField::referencedEntities() method is adding a method on ER FieldItemList class to (multi)load the entities referenced in the field.
It looks like we should:
- unify method names : referencedEntities() ? targetEntities() ?
- make the method in Entity rely on the one in ERFieldItemList ?
Comment #206
andypostFiled critical follow-up #2099105: Clean-up render cache when permission changes
Bacase render cache should care about permissions
Comment #207
catch@beejeebus and @yched follow-ups for both of those issues definitely.
I also opened #2099131: Use #pre_render pattern for entity render caching and #2099133: Comment form on same page as comments forces node render caching to be per user
Comment #209
Wim LeersEnabling node render caching is being done at #2151459: Enable node render caching. The many blockers to be able to do that, are listed in that issue.
Comment #210
YesCT CreditAttribution: YesCT commented#1743590: Isolated Block Rendering was postponed on this. since this is done, opening that one.
Comment #211
yched CreditAttribution: yched commentedNote : #2384163: Entity render cache is needlessly cleared when an Entity*Fom*Display is modified
Comment #212
yched CreditAttribution: yched commentedPing : #2471801: ContentEntityBase::referencedEntities() is inefficient, and not used anymore ?