Allow annotations to inherit across namespaces

And there should be a parameter in services.yml,

parameters:
  modulename.plugin_annotations:
    'some\annotation\class':
      -  'my\annotation\namespace'

timplunkett suggested a service id instead of 'some\annotation\class' but I do not think that's a good idea -- the acting plugin manager knows the annotation class but not its own service id (or it shouldn't ).

Depends on if the plugin manager looks for this data, or if it's passed to the manager. In the former case, a classname makes more sense. In the latter, a service ID would be better.

Doesn't matter to me!

Erm nope, we agreed on 1) adding a method call to the default plugin manager class to gather namespaces b) a MigrateDrupalServiceProvider implements ServiceModifierInterface which will do a nice $container->getDefinition('plugin.manager.migrate.source')->addMethodCalls('addAnnotationNamespace', 'Drupal\migrate_drupal\whatever').

Yes, that's how I imagined it, exactly!!

I'm curious whether it could be enough to explicitly list additional annotation classes, not namespaces ...

The annotation reader takes namespaces...?

Adding a related issue: #2557433: Add internal, event, and property to the list of ignored annotations in the plugin annotation system

To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary.

I'm working on it

Here's the reroll

This is useful! Thanks a lot for creating this :)

1. +++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
   @@ -47,6 +47,13 @@ class AnnotatedClassDiscovery implements DiscoveryInterface {
   +  protected $annotationNamespaces = array();
   

   Use the short array syntax: [].

2. +++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
   @@ -74,6 +84,8 @@ protected function getAnnotationReader() {
   +      array_walk($this->annotationNamespaces, [$this->annotationReader, 'addNamespace']);
   

   I suggest using a loop and calling ::addNamespace using executable code. It improves static analysis of the code.

3. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -98,6 +98,14 @@ class DefaultPluginManager extends PluginManagerBase implements PluginManagerInt
   +  protected $annotationNamespaces = array();
   

   Use the short array syntax: [].

4. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -112,13 +120,16 @@ class DefaultPluginManager extends PluginManagerBase implements PluginManagerInt
   +  public function __construct($subdir, \Traversable $namespaces, ModuleHandlerInterface $module_handler, $plugin_interface = NULL, $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin', array $annotation_namespaces = array()) {
   

   Use the short array syntax: [].

5. +++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
   @@ -225,6 +248,29 @@ public function useCaches($use_caches = FALSE) {
   +   * Fetches from the cache backend, respecting the use caches flag.
   +   *
   +   * @see \Drupal\Core\Cache\CacheBackendInterface::get()
   +   */
   +  protected function cacheGet($cid) {
   +    if ($this->useCaches && $this->cacheBackend) {
   +      return $this->cacheBackend->get($cid);
   +    }
   +    return FALSE;
   +  }
   +
   +  /**
   +   * Stores data in the persistent cache, respecting the use caches flag.
   +   *
   +   * @see \Drupal\Core\Cache\CacheBackendInterface::set()
   +   */
   +  protected function cacheSet($cid, $data, $expire = Cache::PERMANENT, array $tags = array()) {
   +    if ($this->cacheBackend && $this->useCaches) {
   +      $this->cacheBackend->set($cid, $data, $expire, $tags);
   +    }
   +  }
   +
   +  /**
   

   This was accidentally added in the last re-roll and must be removed.

6. +++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php
   @@ -52,8 +52,10 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
   +  function __construct($subdir, \Traversable $root_namespaces, $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin', array $annotation_namespaces = array()) {
   

   Use the short array syntax: [].

Can this still go into 8.0.x or does it need to be 8.1? It currently blocks #2560795: Source plugins have a hidden dependency on migrate_drupal

Just a straight re-roll with the feedback from #18.

1. Fixed
2. Fixed
3. Fixed and updated two more right above in the same file.
4. Fixed
5. Fixed
6. Fixed

It would be nice to see some test coverage just so I can confirm you are trying to solve what I think you are. If it is this seems a useful API addition.

Here we go. I might rename a couple of things in the test like PluginExampleExtended to PluginExtended and plugin_test_extra to plugin_test_extended but you get the idea.

OK, done the renames, this is ready for a final review. Removing the framework manager tag as I think #22 counts as that.

Seems ready to me. Just need a change notice.

Change record created, just needs publishing. https://www.drupal.org/node/2686097

I really like the feature itself. Nice! ON the other hand I wonder whether it really makes sense to add this additional parameter instead of leveraging the existing $plugin_namespace and allow to pass in either a string or an array of values. IN the future this could then be even replaced by variable function arguments.

#28 looks like a CNR.

@dawehner, are you referring to the DefaultPluginManager or the AnnotatedClassDiscovery or both? I'm not sure how we'd reuse $plugin_namespace, surely that can be different to the annotation namespaces?

@benjy
You are absolutely right, I mixed up stuff.

I think we need to document how a module would be expected to provide these additional annotation namespaces. It's not immediately obvious. I think the answer is to use service decoration to replace the existing plugin manager and add the addition annotation namespaces... but how will this work for multiple decorations - ie where two different modules are adding adding annotation namespaces?

Blocks major issue #2560795: Source plugins have a hidden dependency on migrate_drupal.

@alexpott: Here's how #2560795: Source plugins have a hidden dependency on migrate_drupal is using it:

namespace Drupal\migrate_drupal\Plugin\Annotation;
use Drupal\migrate\Annotation\MigrateSource;

/**
 * Defines the annotation for Migrate source plugins which deal with Drupal
 * source data.
 *
 * @Annotation
 */
class MigrateDrupalSource extends MigrateSource {

}

Then migration plugins doing Drupal-to-Drupal migration simply reference MigrateDrupalSource rather than MigrateSource.

I think we need to document how a module would be expected to provide these additional annotation namespaces. It's not immediately obvious

Can't we do this in a follow-up? We already have a test which clearly shows how its used and a change record. The additional docs of how a module hooks into the right place to do such a task would be nice but not a blocker?

cross posted with @mikeryan, what @alexpott is asking for is an example that shows how you'd decorate an existing plugin manager to add your additional annotation class to the discovery.

Ah yes, realized that context once I looked closely at the patch itself...

+1 for #36.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -112,13 +120,16 @@ class DefaultPluginManager extends PluginManagerBase implements PluginManagerInt
+   * @param string[] $annotation_namespaces
+   *   (optional) Additional namespaces to scan for annotation definitions.
...
+  public function __construct($subdir, \Traversable $namespaces, ModuleHandlerInterface $module_handler, $plugin_interface = NULL, $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin', array $annotation_namespaces = []) {

+++ b/core/modules/system/src/Tests/Plugin/Discovery/AnnotatedClassDiscoveryTest.php
@@ -57,12 +57,21 @@ protected function setUp() {
+    $annotation_namespaces = ['Drupal\plugin_test\Plugin\Annotation', 'Drupal\plugin_test_extended\Plugin\Annotation'];

I think the parameter should be $additional_annotation_namespaces since in the test actually there is no requirement to add 'Drupal\plugin_test\Plugin\Annotation' to the array.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -188,6 +199,18 @@ public function clearCachedDefinitions() {
+  public function addAnnotationNamespace($namespace) {
+    if (!in_array($namespace, $this->annotationNamespaces)) {
+      array_push($this->annotationNamespaces, $namespace);
+    }
+  }

Let's return $this for fluency. And it seems strange that this is not going on an interface anywhere...

Renaming and fluencizing...

And it seems strange that this is not going on an interface anywhere...

Doesn't look like there's an appropriate interface out there, what would be a good name for a new one? PluginAnnotationNamespaceInterface?

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -188,6 +199,22 @@ public function clearCachedDefinitions() {
   /**
+   * Adds a namespace to be scanned for annotation definitions.
+   *
+   * @param string $namespace

We should either document that this should be called before using anything like createInstance()/getDefinitions() OR we should invalidate the discovery automatically.

Fixing for #43. I did both for a good measure. Just documenting that seemed like not a very great idea, but I am not sure if I caught all the properties that need to be reset.

Speaking of which, I am not sure if the method addAnnotationNamespace is even useful to this issue. I don't think this is tested either.

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -210,6 +214,8 @@ public function clearCachedDefinitions() {
+      $this->setCachedDefinitions([]);

This does a $this->cacheSet() call, so do we really want to do that?

@dawehner: I think we have to. If we don't and instead just set $this->definitions to NULL, a call to getDefinitions() will call getCachedDefinitions() which will load the definitions from cache (using $this->cacheGet()).

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -188,6 +199,28 @@ public function clearCachedDefinitions() {
   /**
+   * Adds a namespace to be scanned for annotation definitions.
+   *
+   * Use this with care as this resets the discovery and any definitions that
+   * may have been cached before this call. Generally, this should be called
+   * before any createInstance() or getDefinitions() calls.
+   *
+   * @param string $namespace
+   *   The namespace to add.
+   *
+   * @return \Drupal\Component\Plugin\PluginManagerInterface
+   *   A plugin manager for chaining.
+   */
+  public function addAnnotationNamespace($namespace) {
+    if (!in_array($namespace, $this->additionalAnnotationNamespaces)) {
+      array_push($this->additionalAnnotationNamespaces, $namespace);
+      $this->discovery = NULL;
+      $this->setCachedDefinitions([]);
+    }
+    return $this;
+  }

Imo as this is not used in this issue it should be removed. If this is what we plan to use in the migrate issue then it should be tested here.

Here it is without the unused method - I've verified this works with the latest (about-to-be-uploaded) patch on #2560795: Source plugins have a hidden dependency on migrate_drupal, and with the migrate_plus/migrate_tools modules.

I've tested this patch, thanks mike! This applies against current 8.1.x. I've tested this with the patch from#2560795.

@alexpott: Anything further we can do to get this (and its companion #2560795: Source plugins have a hidden dependency on migrate_drupal) in for 8.1?

Thanks.

Sorry @mikeryan this had fallen off my radar - it looks great!

Committed ff85e2e and pushed to 8.1.x and 8.2.x. Thanks!

Thanks @alexpott!

patch 2600926-48.patch no longer applies against 8.1-RC1. Is it no longer needed, or does it need to be re-rolled?

Nevermind, I see that it's in there (my bad).

Published change recor.d