Problem/Motivation

See #2241377: [meta] Profile/rationalise cache tags. Visiting the requirements page invalidates 3 entity cache tags, that do not only invalidate the entity type and field definitions caches, but also the complete entity storage cache.

That's not nice.

Proposed resolution

Add a way to entity manager to ignore persistent caches. Call that and then get the change list, instead of invalidating all caches.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#11 entity-manager-use-caches-flag-2443409-11-interdiff.txt1.48 KBberdir
#11 entity-manager-use-caches-flag-2443409-11.patch20.9 KBberdir
#8 entity-manager-use-caches-flag-2443409-8-interdiff.txt12.05 KBberdir
#8 entity-manager-use-caches-flag-2443409-8.patch20.9 KBberdir
#4 entity-manager-use-caches-flag-2443409-4-interdiff.txt8.08 KBberdir
#4 entity-manager-use-caches-flag-2443409-4.patch14.05 KBberdir
#2 entity-manager-use-caches-flag-2443409-2.patch6.61 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Related issues: +#2241377: [meta] Profile/rationalise cache tags
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new entity-manager-use-caches-flag-2443409-2.patch6.61 KB

Untested, but something like this should do it?

fabianx’s picture

fabianx commented

Looks great to me!

Do we need some tests for this?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new entity-manager-use-caches-flag-2443409-4.patch14.05 KB
new entity-manager-use-caches-flag-2443409-4-interdiff.txt8.08 KB

Really, green, not even some unit test fails? How boring ;)

There are unfortunately no unit tests yet for EntityDefinitionUpdateManager, if so, then it would have been relatively easy to assert that no cache tags are invalidated, didn't find a good way to do something similar in the kernel tests. The only thing I can think of is that we'd have to register a cache tag invalidator that stores invalidations so that we can check which tags were invalidated.

What I did change is remove the explicit cache clears in the tests, I think those are wrong because then we're not testing that the entity definitions manager properly handles caches.

Also enabling caches again and explicitly invalidating them in applyChanges(), so if we actually change something, then I think it is correct to invalidate any changes.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented 
+++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
@@ -513,4 +513,17 @@ public function loadEntityByConfigTarget($entity_type_id, $target);
+  public function useCaches($use_caches = FALSE);

Nice solution, would it make sense to move it into the default plugin manager?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Maybe, could be in CachedDiscoveryInterface I guess? Then we wouldn't have to override those two methods in the entity manager.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Sounds good :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new entity-manager-use-caches-flag-2443409-8.patch20.9 KB
new entity-manager-use-caches-flag-2443409-8-interdiff.txt12.05 KB

Did that, extended unit test for default plugin manager, also added the missing cacheSet()

fabianx’s picture

fabianx commented

Does that mean we can now globally in the installer / ModuleHandler::install instead of:

clearCachedDefinitions()

use the

->useCaches(FALSE)

And just invalidate all config caches maybe once at the end?

/me excited - Patch looks good.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Needs work

Looks great aside from these dummy things below ;)

  1. +++ b/core/tests/Drupal/Tests/Core/Plugin/DefaultPluginManagerTest.php
@@ -186,6 +186,30 @@ public function testDefaultPluginManagerWithFilledCache() {
+   * Tests the plugin manager with no disabled cache.

    The PHP doc text is confusing :) 

  2. +++ b/core/lib/Drupal/Component/Plugin/Discovery/CachedDiscoveryInterface.php
@@ -21,4 +21,17 @@
+   * Can be used to ensure that uncached plugin definitions are
+   * returned, without invalidating all cached information.

+++ b/core/lib/Drupal/Core/Entity/EntityDefinitionUpdateManager.php
@@ -95,7 +95,13 @@ public function getChangeSummary() {
+      // getChangeList() only disables the cache and does not invalidate.
+      // In case there are changes, explictly invalidate caches.

    80 chars

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new entity-manager-use-caches-flag-2443409-11.patch20.9 KB
new entity-manager-use-caches-flag-2443409-11-interdiff.txt1.48 KB

@Fabianx: No, I don't think that is possible/correct, in that case, we have to clear caches, we can expect that they changed. Also, the flag would get lost during the container rebuild.

Fixed the comments.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Needs review » Reviewed & tested by the community

Looks ready to me

  • catch committed 2bf5dc1 on 8.0.x 
    Issue #2443409 by Berdir: Add a way to entity manager to get fresh...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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