Problem/Motivation

While working on #2560795: Source plugins have a hidden dependency on migrate_drupal, I discovered (with help from @chx and @tim.plunkett) that it's impossible (at least without doing a bunch of black magic) for an annotation class to extend another annotation class if they reside in different namespaces. This is a major gotcha for anyone who wants to define a new annotation by extending an existing one.

Proposed Resolution

AnnotatedClassDiscovery should accept an array of additional namespaces in which to search for annotation classes.

Remaining Tasks

^^ Write that, in patch form. Commit heartily, then sit back and have a drink.

API Changes

Additional argument to AnnotatedClassDiscovery::__construct() and greater flexibility in defining annotations.

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Issue summary: View changes
chx’s picture

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 ).

tim.plunkett’s picture

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!

chx’s picture

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').

phenaproxima’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
6.74 KB

Here's an initial, test-less patch. For dissection and discussion.

chx’s picture

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

dawehner’s picture

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

chx’s picture

The annotation reader takes namespaces...?

alexpott’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs review » Needs work
Issue tags: +rc target triage

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.

phenaproxima’s picture

Re-upping the patch just so testbot will take a crack at it.

Status: Needs review » Needs work

The last submitted patch, 12: 26009260-13.patch, failed testing.

phenaproxima’s picture

Issue tags: +Needs reroll
phenaproxima’s picture

Issue tags: +blocker

This blocks #2560795: Source plugins have a hidden dependency on migrate_drupal. Therefore, it is a blocker. Ain't logic fun?

alvar0hurtad0’s picture

Assigned: Unassigned » alvar0hurtad0

I'm working on it

alvar0hurtad0’s picture

Here's the reroll

Xano’s picture

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: [].

benjy’s picture

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

phenaproxima’s picture

Technically it's an API addition, so it'll probably be in 8.1 (depending on the strictness of the committer). Tagging appropriately.

benjy’s picture

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
alexpott’s picture

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.

benjy’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: -Needs tests
FileSize
3.83 KB
10.73 KB

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.

benjy’s picture

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

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Seems ready to me. Just need a change notice.

The last submitted patch, 6: 26009260-6.patch, failed testing.

benjy’s picture

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

dawehner’s picture

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.

catch’s picture

Status: Reviewed & tested by the community » Needs review

#28 looks like a CNR.

benjy’s picture

@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?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

Category: Feature request » Task
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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?

mikeryan’s picture

Priority: Normal » Major
mikeryan’s picture

@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.

benjy’s picture

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?

benjy’s picture

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.

mikeryan’s picture

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

jibran’s picture

Status: Needs work » Reviewed & tested by the community

+1 for #36.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

alexpott’s picture

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

mikeryan’s picture

Status: Needs work » Needs review
FileSize
10.94 KB
2.75 KB

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?

dawehner’s picture

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

hussainweb’s picture

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.

dawehner’s picture

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

hussainweb’s picture

@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()).

alexpott’s picture

Status: Needs review » Needs work
+++ 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.

mikeryan’s picture

Status: Needs work » Needs review
FileSize
10.27 KB
1.13 KB

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.

willwh’s picture

Status: Needs review » Reviewed & tested by the community

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

mikeryan’s picture

@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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

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

  • alexpott committed fa4ec00 on 8.2.x
    Issue #2600926 by benjy, mikeryan, phenaproxima, hussainweb,...
mikeryan’s picture

Issue tags: -blocker

Thanks @alexpott!

lhridley’s picture

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).

Status: Fixed » Closed (fixed)

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