Problem/Motivation

Sometimes, you want to be able to invalidate a list of entities without a save occurring. One such example is the Group module which, when a new access rule is put in place, wants to make sure that all entities subject to said rule have their lists invalidated.

Right now, this is really annoying to do, because you either:

  1. Have to rely on EntityBase::postSave() calling the protected invalidateTagsOnSave() for you and then you also need to deal with the fallout of entity classes extending said methods and running extra postSave logic that you did not necessarily want to trigger.
  2. Have to copy-paste whatever is inside EntityBase::getListCacheTagsToInvalidate() and potentially miss out on whatever else modules may have put in their version of that method, risking security issues because you did not invalidate every list properly.

One example of custom list cache tags in the wild is, surprise, also Group: https://git.drupalcode.org/project/group/-/blob/4.0.x/src/Entity/GroupRe...

You'll notice I made my override public, because php allows you to do so. But the base class method is still bery much protected.

Steps to reproduce

N/A

Proposed resolution

Make the method public so we can gather the list cache tags the way the module providing the entity type intended and invalidate them ourselves without having to trigger a save that really isn't a save.

Remaining tasks

Nothing, I'll make an MR with this change.

User interface changes

N/A

Introduced terminology

N/A

API changes

Protected becomes public, so needs a major version because any protected extension in contrib will otherwise throw a hissy fit. Or we do a little deprecation dance where we introduce a new public method and have the old one call the new.

Data model changes

N/A

Release notes snippet

N/A

Issue fork drupal-3608207

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde opened merge request !16206

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Active » Needs review

Highlighting from IS:

Protected becomes public, so needs a major version because any protected extension in contrib will otherwise throw a hissy fit. Or we do a little deprecation dance where we introduce a new public method and have the old one call the new.

The current MR just changes the visibility to public.

catch’s picture

catch
he/him
Primary language English
commented

I think we could do this in main with a change record.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

This is the Group issue that needs it and it sheds some more light on why not being able to call this directly makes you have to choose between:

  1. A surefire security issue (do nothing)
  2. A data corruption or annoying developers issue (call postSave() when no save actually happened)
  3. A milder potential security issue (copy-paste core code and pray no-one else extended getListCacheTagsToInvalidate() )
  4. A performance and annoying DX issue by calling save when you don't really need it (what older version of Group do)

#3608210: We also need to invalidate list cache tags now that we no longer save entities upon (un)grouping

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

This method went through many iterations in #2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing.

We ended up making it protected and not public due to interface/BC concerns. It being public doesn't really do much if it's not also on the interface, adding it there causes allowed but annoying fatal errors in modules like views_ui and search_api.

Also, earlier concerns by me where about people using it in the wrong way, when adding cache tags to the response, it is not correct to loop over the results and aggregate the list cache tags of your entities. This was before adding the toInvalidate() suffix, so that's only a minor concern now, getCacheTagsToInvalidate() is also public.

In regards to the use case, I doubt that list cache tags alone might be sufficient for all possible cases. You also need per-entity cache tags, which is doable, but more importantly, you might still miss out on onsave method logic, for example the 4xx-response tag in \Drupal\Core\Entity\EntityBase::invalidateTagsOnSave(). If that's a case we want to support then we might need to refactor \Drupal\Core\Entity\EntityBase::invalidateTagsOnSave() instead and introduce a new method that returns the cache tags. But even that will miss out on additional tags, such as those in presave/insert hooks, as recommended by https://www.drupal.org/project/views_custom_cache_tag. In short, I don't think there is ever going to be a 100% guarantee to fully and properly invalidate an entity when not actually saving it.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

We ended up making it protected and not public due to interface/BC concerns. It being public doesn't really do much if it's not also on the interface, adding it there causes allowed but annoying fatal errors in modules like views_ui and search_api.

True, but perhaps a necessary evil until we land #3516034: Add cacheable metadata to SelectInterface and entity QueryInterface objects?

The idea is that I can already add my own list cache tags to whatever query is altered by Group and invalidate those tags. But if that cacheable metadata is never consumed, then what good are those cache tags?

If that's a case we want to support then we might need to refactor \Drupal\Core\Entity\EntityBase::invalidateTagsOnSave() instead and introduce a new method that returns the cache tags

I could also live with that, but then we also need to look at ::invalidateTagsOnDelete()

In short, I don't think there is ever going to be a 100% guarantee to fully and properly invalidate an entity when not actually saving it.

Maybe so, but right now I am only able to invalidate one out of two major pieces and there seems to no longer be any good reason to keep the second piece gated behind a protected method. I'd rather cover 90%, knowing some edge cases may not work than not even cover 50%, knowing we'll have security issues.

Finally, when #3516034-43: Add cacheable metadata to SelectInterface and entity QueryInterface objects is properly resolved, this could also become a lot easier because then I could rely on my custom list cache tags more. See my comment in #3608210-2: We also need to invalidate list cache tags now that we no longer save entities upon (un)grouping

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Here's what I cooked up in the meantime, so I don't have to wait for either core issue to land. Not a fan of using ReflectionMethod, but it might get the job done until [#16573582] lands and then we might not even need this issue.

https://git.drupalcode.org/project/group/-/merge_requests/314/diffs

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Whatever I needed this for in Group is basically a workaround until we can land both #3516034: Add cacheable metadata to SelectInterface and entity QueryInterface objects and #3028976: Enable an entity query's return value to carry cacheability. I have since reconsidered the approach in Group and am wondering whether we still need to make the method in question public. It wouldn't hurt, necessarily, but I'm now more open to closing this issue if others don't see an immediate benefit either.

See my reasoning in the Group issue here: #3608210-9: We also need to invalidate list cache tags now that we no longer save entities upon (un)grouping

Just let me know :)

yakoub’s picture

yakoub commented

In short, I don't think there is ever going to be a 100% guarantee to fully and properly invalidate an entity when not actually saving it.

Have to rely on EntityBase::postSave() calling the protected invalidateTagsOnSave() for you and then you also need to deal with the fallout of entity classes extending said methods and running extra postSave logic that you did not necessarily want to trigger.

whoever overrides postSave, must keep their logic confined by postSave operation and not do shenanigans .
this remains the least risk and smallest window for producing errors .

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

whoever overrides postSave, must keep their logic confined by postSave operation and not do shenanigans .

Well, yes. But there could also be innocuous things happening there that we might disturb by calling postSave() when we aren't really saving.

E.g.: Someone adds a logger to postSave() or a statistics tracker, now if we call postSave() when we didn't actually save anything, we ruin their statistics.

yakoub’s picture

yakoub commented

they must have unit testing which verifies single save equals single statistical increment, and when they detect something wrong they should implement custom logic in case groups module is installed to decrement the statistics counter as required by hooking into some groups module event subscriber .
yes, there would be a bug .
but it can be mitigated easily .