Manually clear cache keys from plugin managers with finite variations instead of using cache tags

there are some cache tag invalidations for library_info and theme_registry, for example \Drupal\system\EventSubscriber\ConfigCacheTag::onSave. That's why I was asking whether we considering that an API and therefore BC break.

How it will clear theme on its uninstall?

there are some cache tag invalidations for library_info and theme_registry, for example \Drupal\system\EventSubscriber\ConfigCacheTag::onSave. That's why I was asking whether we considering that an API and therefore BC break.

hmm I think that's actually wrong, and that it should be doing that via ::clearCachedDefinitions() on the plugin manager, if that's what it wants to clear. However we might want to open a spin-off issue for that to make that change first.

> How it will clear theme on its uninstall?

It won't be requested anymore when it's uninstalled so that doesn't really matter. That said, module uninstall empties all cache bins and we could do the same for themes if we'd want to.

Some work, we'll see how well it works. Didn't touch the language plugin managers yet. library_info usage is too dynamic, we can't replace that one.

fix to see test result

23 failed tests

I missed the module theme build cache.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed CS

The last submitted patch, 12: 3335768-12.patch, failed testing. View results →

Thanks for fixing my cs issues ;)

Down to two failures now, great, and that seems to be just a unit test that we'll need to update.

And the per-language plugin caches need to be updated too.

Fixed the unit test and updated some other plugin managers that use cache tags.

* help topics has an extension cache tag, but all plugins are invalidated when modules are installed or uninstalled, so that shouldn't be needed at all.
* contextual links, removed for now, but this also has group caches that do _not_ use the tag, so I think those are already broken. The thing is that I doubt that these are needed at all. it also doesn't do a reset of the data on the property. The only calculation is a single loop and checking them against the group, that should be quite fast? And I'm pretty sure we load all when a definition is then fetched for them.
* local actions

Kept it for local task manager, as that has per-route caches.

Help topics having twig extensions for routes/links and search module integration required hack for reindexing

All plugin types depend on which modules are enabled, that's why w clear them all through the plugin.cache_clearer service. However, we're not yet doing that for themes, which the cache tag supported. I added the cache clear call for that case instead, this doesn't cover uninstall yet, we probably need that too.

Wonder if a simple change record could be created to announce the new function clearCachedDefinitions

> clearCachedDefinitions

This is not a new function, it's just implementing an existing API of plugin managers.

But yes, this does need a change record, to announce the cache tags that are no longer available for invalidation and that instead those (existing) methods must be used. This should be very rare, there's a single case in core.

Somehow the previous changes from the patches didn't make it into the merge request, added that back now. Setting back to needs review to run the tests, this also needs reviews.

Seems to have a CI failure in the MR.

Ignored error pattern #cache tag might be unclear and does not contain
the cache key in it.# was not matched in reported errors.
-- --------------------------------------------------------------

Fixed that I think.

+1. Happy with the changes and tests.

Still a bit of work left, and we need to create the change record.

Rerolled and updated constructors. Created https://www.drupal.org/node/3355227 → .

  Line   core/lib/Drupal/Core/Theme/Registry.php                              
 ------ --------------------------------------------------------------------- 
  770    Call to static method invalidateTags() on an unknown class           
         Drupal\Core\Theme\Cache.                                             
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  

Fixed that in the gitlab editor. FWIW, I'm not convinced that cache tag invalidation is now in a better place. I think both module and theme install and uninstall should consistently clear caches, but that's not in scope of this issue.

There's still CS errors and it needs follow-up for #29

Meh. ;)

Left few review comments, it looks mostly ready

Thank for the review, cleaned up ElementInfoManager and also added the trait for the removed property.

Seems there are 2 open threads on the MR still.

Think this should be updated for 10.2 now also?

As commented on the two open and related threads, I haven't seen mixed constructor declarations in core before, are you sure that is a thing we do? Setting to needs review to get more feedback.

There's in core https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/lib/Drupal/...

Moreover there's more

$ git grep ' __construct('|grep -E '( \w*\|\w* )'
core/lib/Drupal/Component/DependencyInjection/ReverseContainer.php:35:  public function __construct(private readonly Container|SymfonyContainer $serviceContainer) {
core/modules/block/src/Plugin/migrate/process/BlockTheme.php:46:  public function __construct(array $configuration, $plugin_id, $plugin_definition, Config|MigrationInterface $theme_config, array|Config $themes) {
core/modules/node/src/Form/NodeRevisionDeleteForm.php:76:  public function __construct(EntityStorageInterface $node_storage, EntityStorageInterface $node_type_storage, AccessManagerInterface|Connection $access_manager, DateFormatterInterface $date_formatter) {

So I added commit with this change

@Berdir thanks for review, fixed wrong condition

All the threads seem to be resolved.

I was looking at the change record and think it would help to mention that ConfigCacheTag now requires \Drupal\Core\Theme\Registry and ElementInfoManager no longer takes \Drupal\Core\Cache\CacheTagsInvalidatorInterface

Rebased the MR on 11.x. 🐛 ChainedFastBackend invalidates all items when cache tags are invalidated Fixed is in, so would be nice to land this as well to fully benefit from it.

#38: Personally, I find change records for trivial constructor changes for classes that are rarely if ever subclassed not useful. It doesn't belong in the change record that's about the actual change as it's related to that, and a separate one just for that is IMHO just noise and makes it harder to find relevant changes. The only afffected contrib module seems to be bootstrap for Registry, there are some subclasses of ElementInfoManager but they don't change the constructor.

But if that's blocking getting this to RTBC I'll create one.

Agreed we don't need change records for constructor changes. Not really here but will try to review this again soon.

Did a little bit of profiling, pretty much as expected. Not a big change, but with render caching disabled on standard install on frontpage as admin, we're saving 3 calls to DatabaseCacheTagsChecksum::getTagInvalidationCounts(), each of them did a lookup for a unique and otherwise not used cache tag, so we save 3 queries. There are more plugin managers being changed, but they're not all being used in this cenario (link relations, help?, migration, ..).

Putting in committer queue.
Also removing CR updates tag per #41

Updated the deprecation messages to 10.2 on commit. Committed/pushed to 10.2.x, thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

FYI this caused a regression in migrate_plus with no deprecation error: #3413533-5: Fix failing tests on HEAD → .