Allow plugins to be discovered in any directory [#2043379]

In #1987298: Shorten directory structure and PSR-0 namespacing for plugins we combined the $owner and $type into $subdir.
However, \Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery::getPluginNamespaces() hardcodes $namespaces\Plugin\$subdir, preventing any true flexibility.
In addition, it also requires a $subdir, preventing plugins from residing in top level directories.

Now that #2042545: Annotation parsing should ignore @file doxygen is in, we can have plugin files wherever we want, but not by using AnnotatedClassDiscovery.

This patch does three things:

Makes $subdir optional, and moves it to be the second parameter for AnnotatedClassDiscovery
Forces the plugin manager to declare the full subdir (including Plugin\)
Adds test coverage for placing a plugin at the top level (Drupal\$provider)

Comment	File	Size	Author
#13	plugin-2043379-13.patch	33.91 KB	tim.plunkett
#13
#13	interdiff.txt	2.82 KB	tim.plunkett
#10	plugin-2043379-10.patch	32.56 KB	tim.plunkett
#10
#9	plugin-2043379-9.patch	33.83 KB	tim.plunkett
#9
#9	interdiff.txt	2.94 KB	tim.plunkett
#3	interdiff.txt	881 bytes	tim.plunkett
#3	plugin-2043379-3.patch	33.18 KB	tim.plunkett
#3
#1	plugin-2043379-1.patch	34.88 KB	tim.plunkett
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 16 July 2013 at 18:03

Status:

Active

» Needs review

File	Size
plugin-2043379-1.patch	34.88 KB

Comment #2

16 July 2013 at 19:26

Status:

Needs review

» Needs work

The last submitted patch, plugin-2043379-1.patch, failed testing.

Comment #3

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 16 July 2013 at 20:06

Status:

Needs work

» Needs review

File	Size
plugin-2043379-3.patch	33.18 KB

interdiff.txt	881 bytes

Missed a spot.

I also reduced the scope somewhat and did not move the tours/tips.

Interdiff is just the bug fix.

Comment #3.0

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 16 July 2013 at 20:06

Issue summary:

View changes

Reduce scope

Comment #4

dawehner

German

CreditAttribution: dawehner commented 16 July 2013 at 20:10

--- a/core/lib/Drupal/Core/Action/ActionManager.php
+++ b/core/lib/Drupal/Core/Action/ActionManager.phpundefined

@@ -28,7 +28,7 @@ class ActionManager extends PluginManagerBase {
+    $this->discovery = new AnnotatedClassDiscovery($namespaces, 'Plugin/Action', array(), 'Drupal\Core\Annotation\Action');

Does that mean that we will potentially be able to reuse the class for the EntityType patch later?

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -38,12 +31,12 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
+   * @param string $subdir

@@ -51,8 +44,10 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
+  function __construct(\Traversable $root_namespaces, $subdir = FALSE, $annotation_namespaces = array(), $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin') {

$subdir needs better docs. Any reason to not force the variable and skip the boolean part? Then we could also get rid of specifying the namespaces first, which feels wrong.

Comment #4.0

dawehner

German

CreditAttribution: dawehner commented 16 July 2013 at 20:10

Issue summary:

View changes

Updated issue summary.

Comment #5

neclimdul

He/Him

CreditAttribution: neclimdul commented 16 July 2013 at 20:16

Seems reasonable but I'm curious
1) why are we changing the argument order?
2) does this solve any problems or is this just a cleanup?

Comment #6

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 16 July 2013 at 20:31

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/Discovery/CustomDirectoryAnnotatedClassDiscoveryTest.phpundefined
@@ -0,0 +1,48 @@
+    $this->discovery = new AnnotatedClassDiscovery($namespaces);

This is the reason I swapped them.

Otherwise it would have read

$this->discovery = new AnnotatedClassDiscovery(FALSE, $namespaces);

If you don't think we should swap them, that's fine. But I needed to touch all of those to add in the Plugin/ for the subdir...

The answer to @neclimdul's #2 is what @dawehner said first.

It will allow us to change
$this->discovery = new AnnotatedClassDiscovery($namespaces, 'Plugin/Core/Entity', $annotation_namespaces, 'Drupal\Core\Entity\Annotation\EntityType');
to
$this->discovery = new AnnotatedClassDiscovery($namespaces, FALSE, $annotation_namespaces, 'Drupal\Core\Entity\Annotation\EntityType');

And move \Drupal\node\Plugin\Core\Entity\Node to \Drupal\node\Node where it belongs.

Comment #7

neclimdul

He/Him

CreditAttribution: neclimdul commented 16 July 2013 at 21:48

Gotcha. I didn't have any objection but neither point was in the summary so it didn't make any sense.

Comment #8

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 17 July 2013 at 03:51

Struggling to find anything wrong with this... so I guess that means +1 RTBC from my perspective.
I really like the possibilities this and custom annotation classes open up.

Comment #9

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 17 July 2013 at 04:05

File	Size
interdiff.txt	2.94 KB
plugin-2043379-9.patch	33.83 KB

Thanks for pointing out the $subdir, that needed some love.

Comment #10

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 17 July 2013 at 17:15

File	Size
plugin-2043379-10.patch	32.56 KB

After further discussion with @dawehner, decided to switch back the parameters after all.

Comment #11

dawehner

German

CreditAttribution: dawehner commented 17 July 2013 at 17:17

Status:

Needs review

» Reviewed & tested by the community

Thank you very much.

Comment #12

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 20 July 2013 at 20:15

Status:

Reviewed & tested by the community

» Needs work

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -38,12 +32,11 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
+   * @param string|bool $subdir
+   *   The plugin's subdirectory, for example Plugin/views/filter.

I think we need to explain that of the possible boolean values only FALSE is valid and what it does.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -38,12 +32,11 @@ class AnnotatedClassDiscovery extends ComponentAnnotatedClassDiscovery {
    * @param \Traversable $root_namespaces
    *   An object that implements \Traversable which contains the root paths
-   *   keyed by the corresponding namespace to look for plugin implementations,
-   *   \Plugin\$subdir will be appended to each namespace.
+   *   keyed by the corresponding namespace to look for plugin implementations.

I think we should keep the comment that says $subdir will be appended to the namespaces (if provided).

Comment #13

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 23 July 2013 at 20:14

Status:

Needs work

» Needs review

File	Size
interdiff.txt	2.82 KB
plugin-2043379-13.patch	33.91 KB

Revisiting this, we don't need it to be FALSE, we can have an empty string. It's more intuitive to me that way.

Comment #14

EclipseGc CreditAttribution: EclipseGc commented 23 July 2013 at 20:26

Status:

Needs review

» Reviewed & tested by the community

If that passes I'm very ++ to this. I'm not fond of the 'Plugin/Whatever' nomenclature having to be explicit, however, it's worth the flexibility we get from doing it. I'm RTBC assuming green on 13.

Eclipse

Comment #15

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 24 July 2013 at 22:59

Title:	Allow plugins to be discovered in any directory	» Change notice: Allow plugins to be discovered in any directory
Priority:	Normal	» Critical
Status:	Reviewed & tested by the community	» Active
Issue tags:		+Needs change record

Committed d88a2be and pushed to 8.x. Thanks!

Not sure if we need to update any change notices... eg. https://drupal.org/node/1704454 or review the documentation https://drupal.org/node/1637614

Comment #16

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 26 July 2013 at 06:29

Title:	Change notice: Allow plugins to be discovered in any directory	» Allow plugins to be discovered in any directory
Priority:	Critical	» Normal
Status:	Active	» Fixed
Issue tags:	-Needs change record

I don't think we need a new one. I updated both of those you linked, after searching through other change notices.

Comment #17

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 26 July 2013 at 06:30

Assigned:

tim.plunkett

» Unassigned

Comment #18

9 August 2013 at 06:31

Status:

Fixed

» Closed (fixed)

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

Comment #19

claudiu.cristea

Romanian

Arad 🇷🇴

CreditAttribution: claudiu.cristea commented 4 October 2013 at 06:38

Landed here because I searched a solution for my scenario: I need to discover annotation plugins in and bellow subdir. Supposing that the subdir is set to Plugin/ImageToolkit/Operation. I want that next classes to be discovered as being of the same plugin type: