- First commit to issue fork.
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 π Add a fallback classloader that can handle missing traits Active . 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.
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.
- πΊπΈUnited States neclimdul Houston, TX
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.
- πΊπΈUnited States nicxvan
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 π Add a fallback classloader that can handle missing traits Active (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.
- πΊπΈUnited States nicxvan
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: https://www.drupal.org/project/drupal/issues/3524219 π Add plugin class module dependencies to dependency calculation Active
- π³πΏNew Zealand quietone
I read the MR comments and made some suggestion and left a question.
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.
- π³πΏNew Zealand quietone
@godotislate, thanks. I resolved all the threads.
The follow up should be π Add plugin class module dependencies to dependency calculation Active . It's a child issue, but I'll add it as a related issue as well.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Left a couple of questions/comments 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.