Problem/Motivation
Derivers in core cache their definitions statically in a $derivatives property. That property cannot be cleared in any way. Because the derivers themselves are also cached on the respective discovery which is cached on the respective plugin manager which is cached on the plugin manager the only way to actually clear the definitions from memory is to rebuild the respective plugin manager service (or the entire container).
In long running processes this can lead to various problems, in particular because derivatives that depend on some state or configuration will not disappear after their state or configuration has long been gone. I hit this when TypedDataManager would still happily tell me all about the entity:node:article data type in a test even though I had just deleted the article node type.
Steps to reproduce
Delete an entity bundle and fetch the entity:$entity_type_id:$bundle definition from TypedDataManager afterwards in the same process. (No amount of clearCachedDefinitions() calls will help either. What does "help" and proves this bug is \Drupal::getContainer()->set('typed_data_manager', NULL).)
Proposed resolution
It would be conceivable and arguably conceptually nicer to introduce a CachedDeriverInterface to have derivers handle the clearing of the static caches explicitly. This would be fairly tricky for a number of reasons:
- Ideally this cache handling would happen directly in
DeriverBasebut we can't do that, at least not directly, because of backwards-compatibility. - If the cache handling happens in a base class every (!) deriver would have to be changed to no longer implement
getDerivativeDefinitions()themselves but instead something likedoGetDerivativeDefinitions()orfindDerivativeDefinitions()and similiarly would have to be updated to no longer populate$this->definitionsthemselves. - It would be good to have the cache-handling be similar or analogous to that for the discovery, but that also involves a persistent cache so it will quickly get very confusing. In particular because the cache-handling is currently not handled by the discovery classes themselves but by the plugin manager.
So instead a more practical and less invasive approach is proposed:
- Make
DerivativeDiscoveryDecoratorimplementCachedDiscoveryInterfaceand make it clear out its$deriverswhen the cache is cleared. - Make
DefaultPluginManagercallclearCachedDefinitions()on its discovery if that implementsCachedDiscoveryInterface. (That this is not already the case is an oversight that should be fixed anyway, even though only with 1. does it have any functional implications.)
Remaining tasks
User interface changes
-
API changes
-
Data model changes
-
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3399559
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:
- 3399559-plugin-derivative-cache-clear
changes, plain diff MR !5263
Comments
Comment #2
tstoecklerComment #4
tstoecklerOpened a merge request with the proposed resolution. We don't really have a generic test infrastructure with dynamic derivers, so I opted for a kernel test that tests the scenario described in the issue summary: Deleting an entity bundle should remove the respective data type.
Comment #5
tstoecklerTest only job fails as expected 👍
Comment #6
tstoecklerComment #7
tstoecklerLinking test-only job here (instead of retriggering it for the latest pipeline) since there was no functional change in the test code: https://git.drupalcode.org/issue/drupal-3399559/-/jobs/284285
Comment #9
smustgrave commentedRebased to run test only feature.
Looking at the change the file isn't internal so believe a simple CR should be added for announcing new functions.
Comment #10
tstoecklerFair enough, created a quick change notice.
Comment #11
tstoecklerComment #12
smustgrave commentedThanks!
Comment #13
akalam commentedThe MR !5263 worked for me in Drupal 10.1. Uploaded a static patch to apply safety from composer.
Comment #15
catchCommitted/pushed to 11.x (which will also become 10.3.x), thanks!
Comment #16
tstoecklerAwesome thanks. Published the change notice now. (I put 10.3.0 as the version there, hope that was correct.)
Comment #17
catchYes 10.3.0 is good. Tempting to backport this one but given the highly theoretical API change easier to not think about it - we probably could if it's urgent/blocks something else.