Convert multiple provider plugin work from migrate into plugin dependencies in core

Task or feature request? I don't think this element qualifies as a bug report...

Stumbled into this from the @todo in MigrationPluginManager. How can a plugin have multiple providers? Doing so breaks the idea of what a provider is, a single source module which owns the plugin. To change that would fundamentally change a contract of the plugin design.

Also stumbled into this while investigating a surprisingly tricky SO question: https://drupal.stackexchange.com/questions/249721/why-is-my-migrate-sour...

I kind of agree with @neclimdul. Providers, as they currently are handled, aren't dependencies; they're a single and obvious source that is immediately apparent (because a plugin is inside its provider module's folder). This is why the plugin manager silently discards plugin definitions from non-existent providers. If a plugin doesn't exist, it's because you didn't enable the module.

But when there are multiple providers, it's hard to see what they are. The plugin manager doesn't tell you "The plugin you want is unavailable because you didn't enable the modules X, Y and Z"; it simply skips the definition on discovery.

What the migrate module really needs is a way for a plugin to have module dependencies, which are checked individually on instantiation (MissingDependencyException) rather than at discovery time. Currently, you can only get this by putting the plugin in its own module and adding the dependency there, which would effectively require an extra migrate_source_X module for every X module; the multi-provider logic is basically a hack to get around this, which results in hard-to-diagnose problems like the above.

Addendum: After reading the original issue that this follows up on, I have to point out: The undefined-class exception fixed by #2560795: Source plugins have a hidden dependency on migrate_drupal would ironically have been easier to understand than the invisible dependency checking that is happening now. This is an example where failing loudly is better.

Yeah, runtime would be too late as the plugin must already be instantiated (which requires the parent class). But do we really only have a choice between a runtime check and filtering out the definition silently? Couldn't the check happen at the time you try to load the plugin, just before it actually calls the constructor?

As it is, if I try to load a multi-provider plugin and it fails, I have no idea whether I spelled it wrong, or forgot to enable a module (and if so, which module). The plugin just doesn't exist.

(Maybe an alternative would be to move the provider check into ::createInstance() rather than ::findDefinitions(), and throwing InvalidPluginDefinition instead of a PluginNotFoundException. This would distinguish between a plugin that never existed at all, and a plugin that got discovered, but can't be loaded because of missing providers.)

With the move to discovery by attributes, dealing with multiple "providers" generally was necessitated since discovery uses reflection and PHP has to parse the class files. With missing providers, this can result in exceptions and/or fatal errors, and this has largely been handled by #3502913: Add a fallback classloader that can handle missing traits for attribute discovery. Remaining part to do for attribute discovery is to filter out plugin definitions that were cached in APCu filecache when the providers (modules) were previously installed but now aren't.

Put up MR with some of this work pulled from attempts in other issues to see what tests pass. Will revisit and address failing tests and check for any outstanding issues later when time permits.

Annotation discovery will be left intact with multiple providers until it is ultimately deprecated and removed.

OK, got all existing tests passing.

Quick summary of work done here (will update the IS later when I'm less tired):
I ran with the idea of plugins (via attribute discovery) having "dependencies" instead of multiple providers as first mentioned in #8. This led to introducing new methods on the plugin system's AttributeInterface and AttributeBase to get and set the dependencies.

Because there's an existing division between Component AttributeClassDiscovery and Core AttributeClassDiscovery, in that the core class handles the concept of "providers", I split dependency detection between the two. So the dependencies found in the component discovery class are the names of classes, interfaces, and traits in the plugin class's hierarchy. Then in the core discovery class, the providers for classes, interfaces, and traits are derived. The end result is that plugins that have missing provider dependencies (in other words, they rely on assets in uninstalled modules) will be excluded from discovery unless the providers are installed. Keeping track of the classes/traits/interfaces also allows for the flexibility to handle if a plugin class has a non-Drupal class in its hierarchy that is optionally in the codebase. This is not checked now, to avoid having to load classes into memory to check, but it could be done if needed.

The methods on the Attribute class to get/set dependencies makes the dependency information available within its definition for processes that need it, such as migrate_drupal detecting whether a plugin has a dependency on it before requiring a source_module property. Will probably need a change record if the changes to AttributeInterface go through.

Remaining to do:

• A test that confirms that installing a module causes plugins that have a dependency on it to be discovered (I actually think this exists), but then after that, uninstalling the module then removes the plugin from discovery

Not sure it can make it, but it would be nice to get in for 11.2 so that a couple classes being removed could be removed clean.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased. Back to NR.

I was really confused to see this being worked on but I see its replacing the multiple providers with dependencies which does seem like a good move. Not an actual review, just the approach makes a lot more sense.

Updating title to clarify the new approach and match updated IS.

I did a first pass here, nothing jumped out immediately, it's pretty clever, I definitely need to run through some things to better understand some bits though.

Added CR https://www.drupal.org/node/3523753.

One thought that came up is that there maybe should be a follow up to get the module dependencies found here to be added to dependency calculations for plugin class instances used in config.

Also mentioned this in a comment in #3502913: Add a fallback classloader that can handle missing traits for attribute discovery (comment #72), but the MR here does address a small bug from that issue. It might makes to create another issue to address that bug specifically with a subset of work done here.

Adding 11.2.0 release priority tag for #29 and bug in #34.

The example actually helped make this click.

We are now caching the dependencies of given plugins and skipping loading them if they are missing, and if I understand correctly it is now handling the uninstall case.

The only concern I have is how much will end up in these dependency trains, but I don't see a better way to do this, and it does seem pretty straightforward all things considered.

I've also reviewed the CR, I think it's clear but the committers can confirm as well.

I've asked @godotislate to create a follow up for

*Note that there may need to be a follow up to tie the uninstall of implicit dependencies like block_content in this example to dependency calculations for configurations using the plugin.

but I'm not going to block this on that, I'll keep an eye on it and follow up if it's not created shortly.

Note that there may need to be a follow up to tie the uninstall of implicit dependencies like block_content in this example to dependency calculations for configurations using the plugin.

Follow up for this: #3524219: Add plugin class module dependencies to dependency calculation

I read the MR comments and made some suggestion and left a question.

Has anyone reviewed the change record?

Has anyone reviewed the change record?

@nicxvan reviewed and asked me for some revisions. If there are still things that need clarifying, I can make additional changes.

I think I addressed or answered the outstanding MR comments.

@godotislate, thanks. I resolved all the threads.

Saving credits

Both the issue summary and #39 mention the need for a follow up but I don't see one in related issues - can someone add that please?

The follow up should be #3524219: Add plugin class module dependencies to dependency calculation. It's a child issue, but I'll add it as a related issue as well.

Left a couple of questions/comments on the MR

thanks @godotislate

Applied the suggestion and answered the Qs on the MR.

Removing an unnecessary conditional gate revealed a couple bugs it was hiding. Push commits to fix. This is ready to be looked at again.

Back to RTBC thanks

This looks good, I had very minor comments nits that I made suggestions for and applied before commit.

I briefly confused myself looking for recursion to get the class hierarchy and not seeing it, it's because that's handled by this while loop, not recursion https://git.drupalcode.org/project/drupal/-/merge_requests/11960/diffs?f...

Committed/pushed to 11.x, thanks!

For anyone who runs into this after updating to 11.3 and needs help figuring out which dependencies are missing from your test, I found the easiest thing to do is add a breakpoint to the following line before running the test:

https://git.drupalcode.org/project/drupal/-/blob/f4830b5037084b5330e22ce...

The $dependency variable will show the missing dependency that prevented the plugin from being discoverable.

Hope that helps!

Had a bizarre error.

Call to a member function set() on null
/data/app/core/modules/system/tests/modules/entity_test/src/Hook/EntityTestHooks.php:72

Thankfully I have weekly pipelines and was able to trace the issue to the change from this issue.

My fix was to installing the formal dependencies of entity_test, particularly, user.module.

The clue from @m.stenta helped a lot.

Too bad there isnt a more obvious way of discovering this issue. Im sure it will cause a lot of lost cycles.

Crosslinking regression