AnnotatedClassDiscovery::getDefinitions is critically slow when viewing nodes with comments

This shows all kind of actual different kind of bugs.

The reason why this works is the following code in the plugin manager:

  public function getDefinition($plugin_id) {
    // Fetch definitions if they're not loaded yet.
    if (!isset($this->definitions)) {
      $this->getDefinitions();
    }
    // Avoid using a ternary that would create a copy of the array.
    if (isset($this->definitions[$plugin_id])) {
      return $this->definitions[$plugin_id];
    }
    return array();
  }

So instead of asking the discovery for every plugin definition, it builds them once and reuses it.

In the Factory classes, using $this->discovery is correct
And the test ones are fine as is.
+1 for RTBC

We're not adding any of that here. Just not bypassing it by calling into a deeper object.

Yes, this changed by using the default plugin manager that does caching itself and not as part of the discovery decorator chain.

Fix is correct and is something we need to check for upcoming conversions.

Nice find Fabianx!

Committed 8242143 and pushed to 8.x. Thanks!

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.phpundefined
@@ -40,7 +40,7 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
   public function createInstance($plugin_id, array $configuration = array()) {
-    $plugin_definition = $this->discovery->getDefinition($plugin_id);

It's not good that there's two ways to get exactly the same information, one of which should never be used. Could we discuss making it harder to make this mistake?

Also looking at the xhprof we still have menu local tasks taking 20ms, I followed up on #2046565: Cache the local action plugins that appear per route.

The reason why this was in core all over the place is that the plugin manager got mostly replaced by the DefaultPluginManager which handles the caching instead of the decorator approach of before. $this->discovery has been good before (as it was the cache decorator),
so we probably failed to review the change properly.

I guess the question is if we need a getDefinition($id) on the discovery, or if the discovery should only be responsible for returning an array of all definitions. Then it would be much more obvious to use $this->getDefinition() instead of $this->discovery->getDefinition(). That would obviously be an API change, so maybe just document it properly..

I'd be OK with an API change if it stops this.

Agreed with #12 that there shouldn't be a way to ask the discovery about a single definition, this should be the manager's task, discovery now is only about finding all.

That would mean removing getDefinition() from DiscoveryInterface (and from existing discovery classes / decorators ?), and adding it to PluginManagerInterface (that would be the only explicit method in there, PluginManagerInterface currently only extends other interfaces...)

Ok, opened #2052921: Document the complexities of the DefaultPluginManager

Skimming the patch, all the changes seem to be to managers that are hard coding logic they copied out of factories instead of using factories. I don't understand why that's being done as I don't see any specific logic in any of them to justify it.