Updated: Comment #4

Problem/Motivation

In Drupal 7, it was common for modules like Views to provide plugins on behalf of other modules. Like node views handlers:

if (!function_exists('node_views_api')) {
  function node_views_api() { return views_views_api(); }
}

We need the annotated plugin version of that.

Proposed resolution

Force DefaultPluginManager to check for a module being enabled.

Remaining tasks

None

User interface changes

N/A

API changes

Plugins whose 'provider' is a disabled module will not be available

To do this the following method signatures are changing:

  • DefaultPluginManager::__construct() now has the module handler injected
  • DefaultPluginManager::alterInfo() no longer requires the module handler

N/A

Files: 
CommentFileSizeAuthor
#78 interdiff-2053153-78.txt3.17 KBdamiankloip
#78 2053153-78.patch44.12 KBdamiankloip
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,832 pass(es).
[ View ]
#68 plugin-2053153-68.patch38.94 KBaspilicious
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

tim.plunkett’s picture

Status:Active» Needs review
Issue tags:+Plugin system, +D8CX
StatusFileSize
new1.25 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
tim.plunkett’s picture

StatusFileSize
new1.21 KB
PASSED: [[SimpleTest]]: [MySQL] 57,796 pass(es).
[ View ]

We already have the module handler! Woot.

aspilicious’s picture

I tested this with Display Suite plugins. (The reason why we started this discussion in irc)
It works great so it would be awesome to see this in core to prevent duplication in contrib.

tim.plunkett’s picture

StatusFileSize
new829 bytes
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

@EclipseGC suggested doing this directly in DefaultPluginManager.

dawehner’s picture

StatusFileSize
new3.18 KB
new3.02 KB
FAILED: [[SimpleTest]]: [MySQL] 56,325 pass(es), 104 fail(s), and 43 exception(s).
[ View ]

PHP, seriously.

As tim mentioned on IRC, this basically makes the module handler required for the default plugin manager.

Status:Needs review» Needs work

The last submitted patch, drupal-2053153-5.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new23.57 KB
new26.33 KB
FAILED: [[SimpleTest]]: [MySQL] 56,998 pass(es), 4 fail(s), and 28 exception(s).
[ View ]

This adds the module handler.

Status:Needs review» Needs work

The last submitted patch, plugin-2053153-7.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.81 KB
new29.14 KB
FAILED: [[SimpleTest]]: [MySQL] 57,495 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Some more missing usages of the module handler.

Status:Needs review» Needs work

The last submitted patch, drupal-2053153-9.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new29.32 KB
PASSED: [[SimpleTest]]: [MySQL] 57,825 pass(es).
[ View ]

Just another rerole, hopefully it is green now.

Status:Needs review» Needs work
Issue tags:-Needs tests, -Plugin system, -D8CX

The last submitted patch, plugin-2053153-11.patch, failed testing.

olli’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, plugin-2053153-11.patch, failed testing.

olli’s picture

Status:Needs work» Needs review
Issue tags:+Needs tests, +Plugin system, +D8CX
tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests

Random failures aside, this looks great. Thanks!

tim.plunkett’s picture

Issue summary:View changes

Updated for new approach

alexpott’s picture

Issue tags:+Needs tests

This change is important for contrib to be able to provide plugins for optional core modules such as views.

alexpott’s picture

I did not add the needs tests tag d.o! this patch has tests

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:-API change, -Approved API change

Needs a reroll

git ac https://drupal.org/files/plugin-2053153-11.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 30020  100 30020    0     0  18877      0  0:00:01  0:00:01 --:--:-- 24327
error: patch failed: core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php:33
error: core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php: patch does not apply
tim.plunkett’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new29.32 KB
FAILED: [[SimpleTest]]: [MySQL] 56,692 pass(es), 413 fail(s), and 318 exception(s).
[ View ]

Straight reroll, I added another method to BlockManager which changed the patch context.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, plugin-2053153-20.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests+API change, +Approved API change
StatusFileSize
new30.29 KB
PASSED: [[SimpleTest]]: [MySQL] 57,629 pass(es).
[ View ]
new997 bytes

Meh, we have a new one.

damiankloip’s picture

Needs a reroll, but just wondering, should the alter be moved after the filtering out of disabled module definitions?

Dave Reid’s picture

Filed a duplicate issue in #2083405: Modules should easily be able to provide plugin implementations on behalf of core modules. This is really important for any contrib module that provides it's own API.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett
StatusFileSize
new2.5 KB
new32.6 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Assigning so I don't lose track

Status:Needs review» Needs work
Issue tags:-API change, -Plugin system, -D8CX, -Approved API change

The last submitted patch, plugin-2053153-25.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review

#25: plugin-2053153-25.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API change, +Plugin system, +D8CX, +Approved API change

The last submitted patch, plugin-2053153-25.patch, failed testing.

tim.plunkett’s picture

I can install minimal and standard, PHP 5.3.14 and 5.4.4, from drush and from the UI.
:(

tim.plunkett’s picture

Status:Needs work» Postponed

Will figure this out after #2084513: Annotation class loading could be more elegant., which changes all of these method signatures.

tim.plunkett’s picture

Status:Postponed» Needs review
StatusFileSize
new30.27 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Rerolled after that

Status:Needs review» Needs work

The last submitted patch, plugins-2053153-31.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new2.02 KB
new32.29 KB
FAILED: [[SimpleTest]]: [MySQL] 57,447 pass(es), 167 fail(s), and 343 exception(s).
[ View ]

Updated the recently added SearchPluginManager.

Status:Needs review» Needs work

The last submitted patch, plugin-2053153-33.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new35.32 KB
FAILED: [[SimpleTest]]: [MySQL] 58,824 pass(es), 2 fail(s), and 12 exception(s).
[ View ]
new3.62 KB

Also missed AggregatorPluginManager, and had the order wrong for ImageToolkitManager.

Status:Needs review» Needs work

The last submitted patch, plugin-2053153-35.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new1.07 KB
new35.46 KB
FAILED: [[SimpleTest]]: [MySQL] 59,125 pass(es), 1 fail(s), and 6 exception(s).
[ View ]

...

Status:Needs review» Needs work

The last submitted patch, plugin-2053153-37.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new1.2 KB
new35.57 KB
PASSED: [[SimpleTest]]: [MySQL] 59,383 pass(es).
[ View ]

More stupid mistakes.

Status:Needs review» Needs work
Issue tags:-API change, -Plugin system, -D8CX, -Approved API change

The last submitted patch, plugin-2053153-39.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:+API change, +Plugin system, +D8CX, +Approved API change

#39: plugin-2053153-39.patch queued for re-testing.

thedavidmeister’s picture

Status:Needs review» Needs work

Do we need to do anything here for things like ActionManager that don't extend DefaultPluginManager?

Also, nitpick but:

// If this plugin was provided by a Drupal module, ensure it is enabled.

I would read that to mean "if the module is not enabled, enable it now", rather than "Do this thing only if the module is enabled".

__construct for TypedDataManager has no documentation. Could we fix that since we're touching that function here and we've modified the docs in other places?

It looks like everything extending DefaultPluginManager has been updated in #39, which is good.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new3.34 KB
new36.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,180 pass(es).
[ View ]

Modules can no longer be disabled anyway, so rewrote that.
Managers that don't extend DefaultPluginManager yet will be fixed in their subissue of https://drupal.org/node/2010412

jibran’s picture

Patch looks good RTBC for me. Let's see what other thinks.

thedavidmeister’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Plugin system

I'm happy with the interdiff. This functionality is probably going to be very important for organisation/DX in contrib and custom projects. As per #42 and #44, RTBC :)

aspilicious’s picture

This will brake every plugin manager in contrib again so it's better to get this in as fast as possible.

David Hernández’s picture

#43: plugin-2053153-43.patch queued for re-testing.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Mcreroll.

damiankloip’s picture

Issue tags:-Needs reroll
StatusFileSize
new36.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2053153-49_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Got an nginx error. Let's try again.

damiankloip’s picture

Status:Needs review» Reviewed & tested by the community
aspilicious’s picture

Yes rtbc++

catch’s picture

#50: 2053153-49.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+API change, +D8CX, +Approved API change

The last submitted patch, 2053153-49.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new34.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,599 pass(es).
[ View ]

The image toolkits moved to lib/Drupal/Core.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

catch’s picture

Title:Allow contrib modules to provide plugins on behalf of optional modules» Change notice: Allow contrib modules to provide plugins on behalf of optional modules
Priority:Normal» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed/pushed to 8.x, thanks!

catch’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
Issue summary:View changes
Cottser’s picture

xjm’s picture

Issue tags:+Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

Les Lim’s picture

Title:Change notice: Allow contrib modules to provide plugins on behalf of optional modules» Allow contrib modules to provide plugins on behalf of optional modules
Status:Active» Needs work
Issue tags:+Needs reroll

So I was just about to write up a quick and easy change record for this, but despite #57, it doesn't look like any of this was actually committed. Definitely needs a re-roll now.

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new43.01 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Wow, that sucks.

jibran’s picture

Wrong patch I think.

tim.plunkett’s picture

ahhhhhh ffuuuuuu i can't find the patch locally. And that one was a bear to reroll.

Status:Needs review» Needs work

The last submitted patch, 62: plugin-2195753-19.patch, failed testing.

xjm’s picture

Issue tags:-Missing change record

Sad kittens.

aspilicious’s picture

Assigned:Unassigned» aspilicious

I'l start the initial reroll as I'm the one who actually needs this in contrib asap. :)

aspilicious’s picture

Status:Needs work» Needs review
StatusFileSize
new38.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Lets try this one

tim.plunkett’s picture

Priority:Major» Critical
Issue tags:+beta blocker

This was "committed" before we even started tagging all of the beta blockers.
Now 4 months later, we still have to do this, and it will change all plugin managers :(

Status:Needs review» Needs work

The last submitted patch, 68: plugin-2053153-68.patch, failed testing.

aspilicious’s picture

Assigned:aspilicious» Unassigned
StatusFileSize
new42.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,686 pass(es), 42 fail(s), and 0 exception(s).
[ View ]

I need help with unit tests o_O.
I managed to remove the fatals with some "calculated guesses" ;) but the local task tests are still failing. It must be really simple to fix this but I keep starring at the dots of the unit test output :

aspilicious’s picture

Status:Needs work» Needs review
dawehner’s picture

StatusFileSize
new42.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,692 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new1011 bytes

This will do it.

Status:Needs review» Needs work

The last submitted patch, 73: plugin-2053153-73.patch, failed testing.

The last submitted patch, 71: plugin-2053153-71.patch, failed testing.

swentel’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/PluginTestBase.php
@@ -35,8 +36,8 @@ public function setUp() {
+    $this->mockBlockManager = new MockBlockManager();$module_handler = new ModuleHandler();

'$module_handler = ... ' should go on a new line

damiankloip’s picture

Looking into this now.

damiankloip’s picture

StatusFileSize
new44.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,832 pass(es).
[ View ]
new3.17 KB

Ok, so this fixes the issue but it's not pretty. I have overridden the findDefinitions() method so that is also checks for the theme in the provider.

The problem: Introducing the code in findDefinitions() that checks the module handler (moduleExists()) does not check the theme handler also. As ConfigMapperManager is a special snowflake that aggregates a list of module and them directories, and passes that to the YAML discovery.

damiankloip’s picture

Status:Needs work» Needs review
dawehner’s picture

Status:Needs review» Reviewed & tested by the community

These fixes seems to be pretty great!

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -267,6 +267,13 @@ protected function findDefinitions() {
+      if (isset($plugin_definition['provider']) && !in_array($plugin_definition['provider'], array('Core', 'Component')) && !$this->moduleHandler->moduleExists($plugin_definition['provider'])) {

I'm not sure this really is a safe assumption. Elsewhere in core we do if ($module_handler->module_exists($plugin_definition['provider']) { well_then_its_a_module() } which is also a little bit weird, but here it's the other way around. I think it is very unfortunate that we're baking in the assumption that a plugin has to come from an enabled module or from something called 'Core' or 'Component' (which is a bug in itself...)

tim.plunkett’s picture

Out of scope, we have to handle whatever AnnotatedClassDiscovery::getProviderFromNamespace() throws at us, which is this.

Plus, this was already "committed", just never pushed.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Ok, getting this in while it's hot. For real this time. :)

Committed and pushed (double-checked) to 8.x. Thanks!

Les Lim’s picture

Title:Allow contrib modules to provide plugins on behalf of optional modules» Change record: Allow contrib modules to provide plugins on behalf of optional modules
Status:Fixed» Active
Issue tags:+Missing change record

Tagging for change record.

damiankloip’s picture

Its not a bug. We made a decision that we wanted to support plugins in the core namespace. So that's intended behaviour.

Plus the whole discovery thing is a much bigger issue than this. Which as Tim mentioned, is certainly out of scope.

aspilicious’s picture

I'm confused what we need here:

Do these one need an update?
https://drupal.org/node/2034535
https://drupal.org/node/2087153

Do we need a seperate one?

tstoeckler’s picture

This is in no way out of scope. This is the first place in core that we're assuming that a plugin provide is either an installed module, or 'Core' or 'Component'. That is the problem and that problem is being introduced here.

Anyway, apparently I'm the only one that finds this problematic, so I'll shut up now.

Also, the fact that something was committed before is not in any way a technical justification for a patch.

Berdir’s picture

@aspilicious: Yeah, those two. While at it, I'd suggest you also update the DefaultPluginManager one to specify an annotation class, as it is common for almost all plugin managers.

damiankloip’s picture

@tstoeckler:

This is the first place in core that we're assuming that a plugin provide is either an installed module, or 'Core' or 'Component'. That is the problem and that problem is being introduced here.

No, that is not true. Otherwise, why would we need that bit of code in this patch?! If no plugins were ever found there it would not do anything... Core and Component plugin namespaces are already registered for plugins. If you don't believe me, just take a look at DrupalKernel::buildContainer().

tstoeckler’s picture

Sorry, I seem to be failing at getting the point across. My point is that a plugin 'provider' could just as well be something completely different, in theory. Neither an installed module, nor 'Core', nor 'Component'. On a custom website project, I could for example choose to always register a \Drupal\TstoecklersKickassPlugins namespace as well. That would have worked absolutely fine until this patch. We're baking very intimate Drupal knowledge into the generally Drupal-agnostic plugin system. That's all I'm saying.

I'm well aware that Core and Component can provide plugins, I pushed hard for that.

damiankloip’s picture

So you would override the container.namespaces parameter? ok. That's up to you I guess.

Seems like you might as well open up an issue if you feel strongly about it - we can talk about how you would actually do this there (seems riddled with issues). For 99.9% of cases this will not be applicable. We don't really support adding arbitrary plugin namespaces anyway? Although this technically can be done.

This is a much wider issue that should be addressed in isolation I think.

aspilicious’s picture

So those 2 are updated, I "modernised" the defaultPlugin one.

tstoeckler’s picture

yay, apparently I finally managed to make myself clear. Thanks @damiankloip for the patience :-)

Yeah, I don't really feel that strongly about it, I just think it is unfortunate, because we didn't have to make that assumption previously.

Berdir’s picture

The two updated change notices look fine to me.

I'm not sure if we want a new one as well. We need to at least document how this works somewhere in the plugin system documentation anyway (https://drupal.org/node/1637730 might be a place to add it (also needs an update for this anyway), or one of the related doc pages about discovery? Not sure..

Thinking about it, this is information that might also be interesting for 7.x -> 8.x upgrades (constructor argument change, but the concept of being able to declare plugins for another module and use the provider to have them filtered out). So 1+ to a very short change record that explains that this concepts now exists, possibly with a very simple example if at all and then point to the relevant documentation page.

chx’s picture

I am not sure about change record but I need to know what -- if anything -- I need to change in the migrate plugin managers and service definitions.

Berdir’s picture

@chx: See https://drupal.org/node/2034535/revisions/view/2778995/6979403

Instead of passing $module_handler to the alter, you pass it to the constructor. That's all. And you need to inject the module handler if you don't do so already.

chx’s picture

For the record, the issue title was very confusing for me at least. What happened here is: plugins now have a limited dependency system. Each plugin can depend on at most one module or theme. The value of provider in the annotation is the name of that module or theme.

chx’s picture

I have drafted a change record now that I understand what this issue is about.

chx’s picture

I fixed the change notice to say we do not support theme as provider (yet?).

jessebeach’s picture

Title:Change record: Allow contrib modules to provide plugins on behalf of optional modules» Allow contrib modules to provide plugins on behalf of optional modules
Status:Active» Fixed
Issue tags:-Needs change record, -Missing change record

Change notice: https://drupal.org/node/2218535

I added a code example to the existing draft change notice by chx and edited the language a little. I also added an API Changes section using the notes from the issue summary. There's also a new link to the Plugin documentation in Drupal 8.

I've added links to the two change notices mentioned by aspilicious in #86.

This satisfies all of the comments about the CN in this issue. I'm marking this as fixed. Please raise concerns if you have them and we'll adjust the wording if needed.

Status:Fixed» Closed (fixed)

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