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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir’s picture

Status: Active » Needs review
FileSize
6.61 KB

Untested, but something like this should do it?

Fabianx’s picture

Looks great to me!

Do we need some tests for this?

Berdir’s picture

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

+++ 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

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

plach’s picture

Sounds good :)

Berdir’s picture

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

Fabianx’s picture

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

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

@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

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

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.