Allow contrib modules to provide plugins on behalf of optional modules

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.

PHP, seriously.

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

This adds the module handler.

Some more missing usages of the module handler.

Just another rerole, hopefully it is green now.

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

Filed #2056713: Random test failure in Drupal\rest\Tests\NodeTest

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

#2056713: Random test failure in Drupal\rest\Tests\NodeTest is in.

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

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

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

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

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.

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.

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

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

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

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

Patch no longer applies.

Mcreroll.

Got an nginx error. Let's try again.

Yes rtbc++

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

The image toolkits moved to lib/Drupal/Core.

Back to RTBC.

Committed/pushed to 8.x, thanks!

Updated issue summary.

Tagging to write up the 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

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.

Wrong patch I think.

Sad kittens.

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

Lets try this one

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 :

This will do it.

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

Looking into this now.

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.

These fixes seems to be pretty great!

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

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

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

Tagging for change record.

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.

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?

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.

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

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

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.

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.

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

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.

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.

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.

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

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.

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

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

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.