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 DeriverBase but 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 like doGetDerivativeDefinitions() or findDerivativeDefinitions() and similiarly would have to be updated to no longer populate $this->definitions themselves.
  • 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:

  1. Make DerivativeDiscoveryDecorator implement CachedDiscoveryInterface and make it clear out its $derivers when the cache is cleared.
  2. Make DefaultPluginManager call clearCachedDefinitions() on its discovery if that implements CachedDiscoveryInterface. (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

CommentFileSizeAuthor
#13 core-3399559-13.patch5.11 KBakalam

Issue fork drupal-3399559

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:

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

Issue summary: View changes

tstoeckler’s picture

Status: Active » Needs review

Opened 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.

tstoeckler’s picture

Test only job fails as expected 👍

tstoeckler’s picture

Issue summary: View changes
tstoeckler’s picture

Linking 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

smustgrave made their first commit to this issue’s fork.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record
There was 1 failure:
1) Drupal\KernelTests\Core\TypedData\TypedDataDefinitionEntityBundleTest::testEntityBundleDefinitions
Failed asserting that an array does not contain 'entity:entity_test_with_bundle:test'.
/builds/issue/drupal-3399559/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-3399559/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-3399559/core/tests/Drupal/KernelTests/Core/TypedData/TypedDataDefinitionEntityBundleTest.php:72
/builds/issue/drupal-3399559/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 1, Assertions: 6, Failures: 1.

Rebased 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.

tstoeckler’s picture

Status: Needs work » Needs review

Fair enough, created a quick change notice.

tstoeckler’s picture

Issue tags: -Needs change record
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

akalam’s picture

StatusFileSize
new5.11 KB

The MR !5263 worked for me in Drupal 10.1. Uploaded a static patch to apply safety from composer.

  • catch committed 0bb6da96 on 11.x
    Issue #3399559 by tstoeckler, smustgrave: Statically cached derivative...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x (which will also become 10.3.x), thanks!

tstoeckler’s picture

Awesome thanks. Published the change notice now. (I put 10.3.0 as the version there, hope that was correct.)

catch’s picture

Yes 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.

Status: Fixed » Closed (fixed)

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