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)
Files: 
CommentFileSizeAuthor
#13 plugin-2043379-13.patch33.91 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,495 pass(es). View
#13 interdiff.txt2.82 KBtim.plunkett
#10 plugin-2043379-10.patch32.56 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,125 pass(es). View
#9 plugin-2043379-9.patch33.83 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,178 pass(es). View
#9 interdiff.txt2.94 KBtim.plunkett
#3 interdiff.txt881 bytestim.plunkett
#3 plugin-2043379-3.patch33.18 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,167 pass(es). View
#1 plugin-2043379-1.patch34.88 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,405 pass(es), 906 fail(s), and 541 exception(s). View

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
34.88 KB
FAILED: [[SimpleTest]]: [MySQL] 56,405 pass(es), 906 fail(s), and 541 exception(s). View

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
33.18 KB
PASSED: [[SimpleTest]]: [MySQL] 57,167 pass(es). View
881 bytes

Missed a spot.

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

Interdiff is just the bug fix.

tim.plunkett’s picture

Issue summary: View changes

Reduce scope

dawehner’s picture

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

dawehner’s picture

Issue summary: View changes

Updated issue summary.

neclimdul’s picture

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?

tim.plunkett’s picture

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

neclimdul’s picture

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

larowlan’s picture

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.

tim.plunkett’s picture

FileSize
2.94 KB
33.83 KB
PASSED: [[SimpleTest]]: [MySQL] 57,178 pass(es). View

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

tim.plunkett’s picture

FileSize
32.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,125 pass(es). View

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you very much.

alexpott’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.82 KB
33.91 KB
PASSED: [[SimpleTest]]: [MySQL] 57,495 pass(es). View

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

EclipseGc’s picture

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

alexpott’s picture

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Status: Fixed » Closed (fixed)

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

claudiu.cristea’s picture

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:

  • Plugin/ImageToolkit/Operation/gd/Crop.php
  • Plugin/ImageToolkit/Operation/gd/Resize.php
  • Plugin/ImageToolkit/Operation/imagemagick/Crop.php (may live in a contrib)
claudiu.cristea’s picture

claudiu.cristea’s picture

Issue summary: View changes

fix typo