Move multiple provider plugin work from migrate into core's plugin system

Created on 18 August 2016, almost 9 years ago
Updated 28 April 2025, 23 days ago
πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

plugin system

Created by

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • Merge request !11960Resolve #2786355 "Move multiple provider" β†’ (Open) created by godotislate
  • Pipeline finished with Failed
    23 days ago
    Total: 405s
    #483444
  • 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.

  • Pipeline finished with Failed
    23 days ago
    Total: 564s
    #483576
  • Pipeline finished with Failed
    23 days ago
    #483592
  • Pipeline finished with Failed
    23 days ago
    Total: 279s
    #484111
  • Pipeline finished with Failed
    23 days ago
    Total: 152s
    #484116
  • Pipeline finished with Success
    23 days ago
    Total: 945s
    #484119
  • 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
  • Pipeline finished with Success
    21 days ago
    Total: 400s
    #485079
  • Pipeline finished with Canceled
    21 days ago
    Total: 149s
    #486086
  • Pipeline finished with Failed
    21 days ago
    Total: 184s
    #486087
  • Pipeline finished with Failed
    21 days ago
    Total: 1350s
    #486089
  • Pipeline finished with Success
    20 days ago
    Total: 686s
    #486105
  • Pipeline finished with Failed
    18 days ago
    Total: 204s
    #487742
  • Pipeline finished with Failed
    18 days ago
    Total: 137s
    #487753
  • Pipeline finished with Failed
    18 days ago
    Total: 541s
    #487791
  • Pipeline finished with Success
    18 days ago
    Total: 615s
    #488066
  • Pipeline finished with Success
    18 days ago
    Total: 707s
    #488136
  • Pipeline finished with Success
    17 days ago
    Total: 714s
    #488483
  • 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.

  • Pipeline finished with Success
    14 days ago
    Total: 1018s
    #490700
  • πŸ‡ΊπŸ‡Έ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.

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

  • πŸ‡ΊπŸ‡Έ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.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Has anyone reviewed the change record?

  • Pipeline finished with Canceled
    about 23 hours ago
    #501577
  • 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.

  • Pipeline finished with Failed
    about 22 hours ago
    Total: 462s
    #501609
  • Pipeline finished with Success
    about 22 hours ago
    #501622
  • I think I addressed or answered the outstanding MR comments.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    @godotislate, thanks. I resolved all the threads.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    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 πŸ“Œ 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

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    thanks @godotislate

  • Applied the suggestion and answered the Qs on the MR.

  • Pipeline finished with Failed
    about 8 hours ago
    Total: 332s
    #502351
  • Pipeline finished with Success
    about 8 hours ago
    Total: 427s
    #502362
  • Pipeline finished with Failed
    about 8 hours ago
    Total: 514s
    #502392
  • Pipeline finished with Failed
    about 7 hours ago
    Total: 188s
    #502404
  • Pipeline finished with Failed
    about 7 hours ago
    #502408
  • Pipeline finished with Failed
    about 7 hours ago
    Total: 253s
    #502413
  • Pipeline finished with Failed
    about 7 hours ago
    #502421
  • Pipeline finished with Failed
    about 6 hours ago
    Total: 522s
    #502429
  • Pipeline finished with Running
    about 6 hours ago
    #502442
  • Removing an unnecessary conditional gate revealed a couple bugs it was hiding. Push commits to fix. This is ready to be looked at again.

Production build 0.71.5 2024