- Issue created by @berdir
- 🇨🇭Switzerland berdir Switzerland
This would break manual invalidations of that tag. We could could consider a BC layer that translates these cache tags to entity_values somewhere.
I found 0 matches of 'node_values' in code search and exactly one for user_values, which is use case b) above (manually inserting stuff into a user field table).
But such a system could also be useful for the block cache tag issue.
- 🇬🇧United Kingdom catch
As far as I can tell, this is only used to avoid clearing the entire
cache.entity
bin when ::resetCache() is called on content entities without IDs.If we consolidated to a single cache tag, then it would be equivalent to clearing the entire bin anyway , so can we just remove the cache tag entirely and clear the whole bin?
I personally don't think we need to worry about bc for the cache tags as such, just that ::resetCache() should still work.
- 🇨🇭Switzerland berdir Switzerland
I was wondering about that, I guess that's an option too.
I assumed people might use that bin for some other stuff, but finding almost zero direct usages to it: http://codcontrib.hank.vps-private.net/search?text=%5CDrupal%3A%3Acache%..., http://codcontrib.hank.vps-private.net/search?text=%40cache.entity&filen....
Worst case is we invalidate a tiny bit more, but again, it's an extremely rare operation to do that.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
I basically said as much as #3 in your MR here: https://git.drupalcode.org/project/drupal/-/merge_requests/10962#note_45...
One thing I'd like to quote from my comment on the other MR:
Eh, double-edged sword really. On some sites it would greatly improve things to flush the entire entity cache in one go, but on other sites I'd be pretty annoyed if my large node cache got flushed because I created a new user.
But come to think of it, that doesn't happen on entity save so the tag is really just adding resource consumption for almost no gain outside of some tests and even there we could argue there's no point to be this granular. It will happen, though, on sites that manually called ::resetCache(). Those people might be a bit annoyed if their resetting of one entity type's cache now clears the entire entity bin.
For BC reasons theoretically we should keep the tag for one more major,, but that would still incur the cache tag lookup for an entire major and, as pointed out by @berdir, there is very little code that calls it.
+1 for removing the tag altogether, but I do think we need a CR for it, even if only as a warning that calling ::resetCache() now clears the entire bin.
- 🇬🇧United Kingdom catch
Cache tags themselves aren't really an API we need to provide bc for. Obviously we wouldn't just rename the entity list cache tags for fun but otherwise you're not really supposed to deal with individual cache tags directly that you don't control, or at least be prepared to update. Agreed with a change record though.
- Merge request !11145Remove _values cache tag from entity storage base. → (Closed) created by kristiaanvandeneynde
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
My Selenium is acting up locally so created a MR to see what testbot thinks. Will see if I can fix my local test setup. I chose to keep the persistent cache check because it seems counterintuitive to flush the persistent cache when we call ::resetCache() on an entity type that isn't even in said cache.
- 🇨🇭Switzerland berdir Switzerland
would probably wait with this until the preload issue is in this will obviously conflict heavily and we'll want to adjust that anyway.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Yeah sure, just wanted to get the ball rolling already and see what tests have to say. Made it go green locally so should go green here too.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Looks good to me. Left a note that I think a committer might raise, but good enough to RTBC for all I care as it can be changed upon commit if they so prefer.
- 🇬🇧United Kingdom catch
Went ahead and did the inline before commit.
Committed/pushed to 11.x, thanks!
- 🇨🇭Switzerland berdir Switzerland
Phew, things are moving fast at the moment, created a change record just in case and also updated https://www.drupal.org/node/3505248 → accordingly.
Automatically closed - issue fixed for 2 weeks with no activity.