Reset plugin discovery when a module/theme is installed

Nice fix!

+++ b/core/lib/Drupal/Core/Entity/EntityTypeManager.php
@@ -79,7 +80,6 @@ public function __construct(\Traversable $namespaces, ModuleHandlerInterface $mo
 
-    $this->discovery = new AnnotatedClassDiscovery('Entity', $namespaces, 'Drupal\Core\Entity\Annotation\EntityType');
     $this->stringTranslation = $string_translation;
     $this->classResolver = $class_resolver;
   }
@@ -87,6 +87,16 @@ public function __construct(\Traversable $namespaces, ModuleHandlerInterface $mo

@@ -87,6 +87,16 @@ public function __construct(\Traversable $namespaces, ModuleHandlerInterface $mo
   /**
    * {@inheritdoc}
    */
+  protected function getDiscovery() {
+    if (!$this->discovery) {
+      $this->discovery = new AnnotatedClassDiscovery('Entity', $this->namespaces, EntityType::class);
+    }
+    return $this->discovery;
+  }
+
+  /**

Does this maybe fixes https://twitter.com/weitzman/status/839605704243691521 ?

1. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -191,6 +191,7 @@ public function clearCachedDefinitions() {
   +    $this->discovery = NULL;
   

   Does this change mean that we might break contrib/custom plugin managers if they, like EntityTypeManager, do some custom discovery and don't implement getDiscovery()? And if so how are we going to prevent them breaking?

2. +++ b/core/modules/breakpoint/tests/src/Kernel/BreakpointDiscoveryTest.php
   @@ -20,6 +20,9 @@ class BreakpointDiscoveryTest extends KernelTestBase {
   +    // Fetch the definitions before installing the theme to prime the cache.
   

   I think this comment could do with being fleshed out a bit to explain that this is to test that theme installation clears the breakpoint definitions - right?

I don't think we should open up entity types to derivatives. Feels a bit out-of-scope here and would need further consideration.

I don't get the entire discussion about derivatives of entity types. We have them effectively already with hook_entity_type_build()

I don't see a reason to opt out.

This looks great for me

Back to RTBC per #15

I don't see a reason to opt out.

The only reason I see is that is opted out in HEAD and this patch is opting it in. Yes we get plugin discovery resetting but we also get more functionality. In my opinion the entity API does not need more ways to achieve the same thing. This is why I think https://www.drupal.org/files/issues/2860346-discovery-8.patch is the safer patch to go for.

As somebody who knows the plugin system implementation details not very well: why not rely on the config:core.extension cache tag?

Basically:

diff --git a/core/lib/Drupal/Core/Entity/EntityTypeManager.php b/core/lib/Drupal/Core/Entity/EntityTypeManager.php
index abe96a5..9a95cd7 100644
--- a/core/lib/Drupal/Core/Entity/EntityTypeManager.php
+++ b/core/lib/Drupal/Core/Entity/EntityTypeManager.php
@@ -76,7 +76,7 @@ class EntityTypeManager extends DefaultPluginManager implements EntityTypeManage
   public function __construct(\Traversable $namespaces, ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, TranslationInterface $string_translation, ClassResolverInterface $class_resolver) {
     parent::__construct('Entity', $namespaces, $module_handler, 'Drupal\Core\Entity\EntityInterface');
 
-    $this->setCacheBackend($cache, 'entity_type', ['entity_types']);
+    $this->setCacheBackend($cache, 'entity_type', ['entity_types', 'config:core.extension']);
     $this->alterInfo('entity_type');
 
     $this->discovery = new AnnotatedClassDiscovery('Entity', $namespaces, 'Drupal\Core\Entity\Annotation\EntityType');

Setting back to needs work for #19 - that sounds like a really good solution imo - easily the most robust.

@tim.plunkett
Just to understand you correctly, this is some static caching, right? I'd have expected that every service is reseetted properly on module install / theme install ...

This came up as daily bug smash target.

This appears like it could still be relevant in D9.5 (but someone correct me if I'm wrong)

Sounds like the issue brought up in #19 was addressed in #21

Rerolled for 9.5. Tried generating interdiff but had an issue since the original patch was from a while ago. But used 2860346-discovery-8.patch to start.

Had merge conflicts in

core/lib/Drupal/Core/Entity/EntityTypeManager.php
core/lib/Drupal/Core/Extension/ThemeInstaller.php
core/modules/breakpoint/tests/src/Kernel/BreakpointDiscoveryTest

That I had to manually resolve.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or 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.

Attached patch against Drupal 10.1.x

Patch #34 is not applied for Drupal 10.1.x so Inter-diff file is not added.

Checking patch core/lib/Drupal/Core/Entity/EntityTypeManager.php...
Checking patch core/lib/Drupal/Core/Extension/ThemeInstaller.php...
Hunk #1 succeeded at 300 (offset -4 lines).
Checking patch core/lib/Drupal/Core/Plugin/DefaultPluginManager.php...
Hunk #1 succeeded at 198 (offset 6 lines).
Checking patch core/modules/breakpoint/tests/src/Kernel/BreakpointDiscoveryTest.php...
error: while searching for:

  protected function setUp(): void {
    parent::setUp();
    \Drupal::service('theme_installer')->install(['breakpoint_theme_test']);
  }


error: patch failed: core/modules/breakpoint/tests/src/Kernel/BreakpointDiscoveryTest.php:24
error: core/modules/breakpoint/tests/src/Kernel/BreakpointDiscoveryTest.php: patch does not apply

1. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
   @@ -300,6 +300,13 @@ public function uninstall(array $theme_list) {
   +    // Clear plugin manager caches.
   +    \Drupal::getContainer()
   +      ->get('plugin.cache_clearer')
   +      ->clearCachedDefinitions();
   

   This dependency should be injected - all the others are in class already.

2. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
   @@ -300,6 +300,13 @@ public function uninstall(array $theme_list) {
   +
   +    $this->themeHandler->refreshInfo();
   

   Why is this necessary? Both install and uninstall are already calling $this->themeHandler->reset();

#34 states that #19 was addressed in #21, but it is not actually clear whether #21 meant that in a context of initially provided patch or in general:

That will clear the cache of definitions, which is fine. But that already happens due to a call to \Drupal::service('plugin.cache_clearer')->clearCachedDefinitions();.
This issue is about the actual discovery object itself.

So #19 is not actually addressed if the patch is not applied and it can be an easier solution.

https://www.drupal.org/node/3204271 has a similar issue that is possible caused by already cached definitions.

For example, for LayoutPluginManager, adding of the cache tag based on the installed extensions resolves the issue

$this->setCacheBackend($cache_backend, $type, ['config:core.extension']);