Convert MigrateSource plugin discovery to attributes

Seems to have test failures.

Note: BC approach to handle annotations and attributes in MigratePluginManager class is being evaluated in https://www.drupal.org/project/drupal/issues/3424509

We should revisit and rebase this PR after this is resolved.

Slack thread for comms

Blocked on #3424509: Update MigratePluginManager to include both attribute and annotation class

There are currently 5 source plugins that do not have source_module in their annotations, all in test modules. One plugin, with ID no_source_module is meant to throw an exception for missing source module in Drupal\Tests\migrate_drupal_ui\Functional\SourceProviderTest. Otherwise the source_module in source plugins were optional, except if migrate_drupal is enabled and the migrations are tagged and configured to be enforced. See Drupal\migrate_drupal\MigrationPluginManager::processDefinition() and ::getEnforcedSourceModuleTags(). Now that source_module is a required property in the MigrateSource attribute from #3424509: Update MigratePluginManager to include both attribute and annotation class, this functionality becomes redundant. Is requiring source_module also a BC break? It's somewhat mitigated, since contrib or custom source plugins missing that property in the annotation can have that property added when they are converted to attributes. But maybe a CR is needed to document the new requirement?

source_module has been required since 8.5, see https://www.drupal.org/node/2911881 and https://www.drupal.org/node/2914530. However, it should just been when migrate_drupal is installed.

Added the part of the reverted commit for #3424509: Update MigratePluginManager to include both attribute and annotation class that handled migrate source plugins and automated/multiple providers.

As for what to do about the source_module property in the migrate source attribute? I think there are two options:

• Make source_module an optional property
• Wait for #3009349: Revert migrate_drupal source annotations to attributes conversion to resolve moving source_module from migrate to migrate_drupal

IMO, we can make source_module an optional property now. There is code in the migrate_drupal module (See Drupal\migrate_drupal\MigrationPluginManager::processDefinition() and ::getEnforcedSourceModuleTags()) that already requires that property on migrate_drupal sources.

Deprecating the property from the attribute can be handled in #3009349: Revert migrate_drupal source annotations to attributes conversion if needed there. There may also be BC concerns if contrib/custom modules have implemented source plugins that use source_module somehow in a custom way that has nothing to do with migrate_drupal.

Made changes to the migrate source attribute:

• source_module is optional
• minimum_version defaults to NULL instead of '' (this is need to pass tests because there is an isset check)
• fixed some tests to match attribute/definition property order
• Properly filter out plugins based on the providers property (as well as provider, singular)

I believe all the remaining tests failures are occurring because there are source plugins using the Drupal\content_translation\Plugin\migrate\source\I18nQueryTrait from the content_translation module that is not enabled. These are the plugins:

• Drupal\menu_link_content\Plugin\migrate\source\d6\MenuLinkTranslation
• Drupal\menu_link_content\Plugin\migrate\source\d7\MenuLinkTranslation
• Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation
• Drupal\taxonomy\Plugin\migrate\source\d7\TermLocalizedTranslation
• Drupal\block_content\Plugin\migrate\source\d6\BoxTranslation

An fatal error occurs when Drupal\Component\Plugin\Discovery\AttributeClassDiscovery::parseClass() tries to instantiate a ReflectionClass on these plugin classes using that trait. This is similar to #3255804: Hidden dependency on block_content in layout_builder. Likely there will be need to be an overall solution for plugin classes.

I think we could make a MigrateSourceDrupal attribute that has the source_module. I have make a test of that at https://git.drupalcode.org/project/drupal/-/merge_requests/5316. Unfortunately that has failing functional tests and those tests pass locally. I don't know why yet.

Made an effort to see if I could at least get tests to pass:

• Restored annotations to the plugins that use I18nQueryTrait
• Add exception handling to AttributeClassDiscovery::parseClass when the ReflectionClass is instantiated. This safely handles classes that extend unknown classes, of which there was at least one: Drupal\field\Plugin\migrate\source\d6\FieldInstancePerViewMode extending Drupal\node\Plugin\migrate\source\d6\ViewModeBase

I'm not sure why some tests are still failing in CI. I've run a couple of them locally and they pass.

In any case, there does need to be a solution for any plugin class that either:

• Uses a Trait from another module that may not be installed
• Implements an Interface from another module that may not be installed
• Extends a class from another module that may not be installed

Handling extends is straightforward, since an exception is thrown in that case, but unknown interfaces and traits lead to fatal errors. Does it make sense to create a separate issue for that?

Left some comments on hopefully pragmatic solutions for these cases.

If that doesn't work out somehow then +1 to skip problematic ones in this first pass and deal with them in a separate and smaller issue, so that we can unblock the deprecation of plugin managers that don't support attributes.

Made the change to move the trait to the migrate module, and removed all the annotations from the plugin classes in question. I've also added a CR for the trait move.

I'm not sure what's going on, because the functional tests failing in CI are passing on my local.

The functional javascript failures I can reproduce on local, but I'm failing to see how they're related. They're also failing in the same way on my local against 11.x, so I'm not sure what's going on there.

Remaining todo:

• Getting tests passing on CI
• Add deprecation test for the trait?

There are some HEAD test fails that have been fixed and also some known random fails (#3438424: [random test failures] Race condition in state when individual keys are set with an empty cache but I also can't reproduce the migrate fails locally.

You can download the artifact of e.g. https://git.drupalcode.org/issue/drupal-3421014/-/jobs/1261546 and look at the browser output, but I'm not seeing anything obvious from that from a quick look.

I did take a look at the browser output yesterday, and what's happening is that on CI, when posting to the ID Conflict form, somehow it's being redirected back to first step. Locally, this redirect isn't happening. I assume the code responsible is this, but I have no idea why: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...

    // Get all the data needed for this form.
    $migrations = $this->store->get('migrations');

    // If data is missing or this is the wrong step, start over.
    if (!$migrations || ($this->store->get('step') != 'idconflict')) {
      return $this->restartUpgradeForm();
    }

I was wrong about which code is leading to the redirect. It's actually a bit further down in the same method: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/migra...

    if ($content_conflicts || $translated_content_conflicts) {
...
    }
    else {
      $this->store->set('step', 'review');
      return $this->redirect('migrate_drupal_ui.upgrade_review');
    }

What is expected is that $translated_content_conflicts should not be empty, but it looks like migration discovery from the previous form (CredentialForm.php) is not finding the same migration plugins on CI as on my local (ddev).

I've pushed some debug code to try to see what's going on, but no conclusions yet.

It's been a couple days since I last took a crack at it, but an update for others who might work on this:

I still haven't found the root cause for the test failures. I have found that certain migration plugins, such as d6_action are found by YamlDirectoryDiscovery in MigrationPluginManager::getDiscovery(), but those plugins are then filtered out by the NoSourcePluginDecorator. This means that the source plugins those migrations are using are not being found.

I've also found that the list of source plugins returned from $definitions = $this->getDiscovery()->getDefinitions(); in MigrateSourcePluginManager::findDefinitions() is unchanged after being filtered by the ProviderFilterDecorator, so it seems that setting multiple providers on the attribute recursively by namespace seems to be working correctly. But I'm not sure why discovery is not picking up the source plugins before that, and I still haven't been able to reproduce the failures locally. Since I run the local tests individually, but CI runs them serially, I had a guess that perhaps there was an issue with the file cache persisting incorrectly between tests, , but that does not seem to be the case.

Tests are finally all green. It was the file cache after all. For any plugin extending a class from an uninstalled module, catching the exception and returning NULL values was resulting in the file cache storing NULL as the value for the plugin definition. Since the cache persists as long as the file is not changed, this meant that even installing the relevant modules did not make the plugin definition discoverable. So instead of catching the exception in parseClass, the exception is caught by the calling function and the plugin is skipped from being stored in file cache. I'm still not completely sure why this wasn't happening locally, but I'll take the win.

I think this should be ready for review. Unless the deprecation of the I18nQueryTrait needs a test?

@godotislate, thanks for working on this. I skimmed through the MR and I appreciate the clear comments throughout. There are two comments on the MR that need attention.

Because of the extra changes needed to make source plugin work as attributes I think it would help other reviewers if the Issue Summary was updated with the key findings and changes.

I believe latest review comments have been addressed. Updated IS and putting back for review.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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, added deprecation test for I18nQueryTrait.

Whoops, meant to put back in Needs Review.

Should the changes to Discovery files be done in separate issue? Since it'll apply to everything.

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.

Should the changes to Discovery files be done in separate issue? Since it'll apply to everything.

That can be done if needed or preferred, but MigrateSource plugins currently are the only type with multiple providers, so the discovery changes only affect those and are a blocker.

Guess no need to break out and delay this ticket as probably important to include in 11.x and 10.3.x

Rebased just to make sure none of the recent change broke anything, didn't obviously.

This has gone through a few reviews it seems so believe this is good

We've had this issue with plugins and implicit dependencies before. The way in which static reflection allows you to load code that depends on non-existing things is extremely problematic. I think we need to come up with a better way to support this via discovery than eating exceptions. Or if we're going to eat exceptions then only do this in the migrate source case for now and open a follow-up issue to fix this in a better way.

Or if we're going to eat exceptions then only do this in the migrate source case for now and open a follow-up issue to fix this in a better way.

Made a change so that the exception is eaten only for migrate source in the attribute automated providers discovery classes. Made an effort to do that while minimizing code duplication, but it's still pretty ugly. I also tried to limit the exception being eaten based on the error message matching "Interface X not found" or "Class X not found."

I can open a follow up for a better general solution if this approach for migrate source looks fine for now. Or maybe that can be part of #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core?

Blocked on deprecating I18nQueryTrait and the plugin classes that use it, and moving them to migrate_drupal in #3258581: Move I18nQueryTrait from content_translation to migrate_drupal.

Updated IS with remaining tasks

I tried a POC on an alternate approach for attribute discovery in MR 8070.

The idea is that since only the attributes are needed, parsing can be done like this:

1. Use PhpToken::tokenize() on the plugin class file
2. Loop through the tokens to find the class declaration token
3. Copy all the class file contents before the class declaration to a string
4. Append a random suffix to the class name for uniqueness, and add a class declaration statement and {} to the string
5. Copy the string to a temporary file and include that file
6. Instantiate ReflectionClass on the new temporary class
7. Extract attribute data from reflection

So basically, core/modules/block_content/src/Plugin/migrate/source/d6/BoxTranslation.php:

<?php

namespace Drupal\block_content\Plugin\migrate\source\d6;

use Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation as D7BlockCustomTranslation;
use Drupal\migrate\Attribute\MigrateSource;

/**
 * Drupal 6 i18n content block translations source from database.
 *
 * For available configuration keys, refer to the parent classes.
 *
 * @see \Drupal\migrate\Plugin\migrate\source\SqlBase
 * @see \Drupal\migrate\Plugin\migrate\source\SourcePluginBase
 */
#[MigrateSource(
  id: 'd6_box_translation',
  source_module: 'i18nblocks',
)]
class BoxTranslation extends D7BlockCustomTranslation {

  /**
   * Drupal 6 table names.
   */
  const CUSTOM_BLOCK_TABLE = 'boxes';
  const I18N_STRING_TABLE = 'i18n_strings';

}

gets copied to /tmp/BoxTranslationadfrghq.php with these contents:

<?php

namespace Drupal\block_content\Plugin\migrate\source\d6;

use Drupal\block_content\Plugin\migrate\source\d7\BlockCustomTranslation as D7BlockCustomTranslation;
use Drupal\migrate\Attribute\MigrateSource;

/**
 * Drupal 6 i18n content block translations source from database.
 *
 * For available configuration keys, refer to the parent classes.
 *
 * @see \Drupal\migrate\Plugin\migrate\source\SqlBase
 * @see \Drupal\migrate\Plugin\migrate\source\SourcePluginBase
 */
#[MigrateSource(
  id: 'd6_box_translation',
  source_module: 'i18nblocks',
)]
class BoxTranslationhhgsagp {}

and reflection is instantiated on Drupal\block_content\Plugin\migrate\source\d6\BoxTranslationhhgsagp, so the attributes can be read, without concern of inheriting from any unknown class, interface, or trait.

This seemed to work OK with some limited local testing, but CI caught a significant error (among some other failures I haven't investigated). For a class like core/modules/system/tests/modules/cron_queue_test/src/Plugin/QueueWorker/CronQueueTestSuspendQueue.php:

<?php

namespace Drupal\cron_queue_test\Plugin\QueueWorker;

use Drupal\Core\Queue\Attribute\QueueWorker;
use Drupal\Core\Queue\QueueWorkerBase;
use Drupal\Core\Queue\SuspendQueueException;
use Drupal\Core\StringTranslation\TranslatableMarkup;

/**
 * A queue worker for testing suspending queue run.
 */
#[QueueWorker(
  id: self::PLUGIN_ID,
  title: new TranslatableMarkup('Suspend queue test'),
  cron: ['time' => 60]
)]
class CronQueueTestSuspendQueue extends QueueWorkerBase {

  /**
   * The plugin ID.
   */
  public const PLUGIN_ID = 'cron_queue_test_suspend';

  /**
   * {@inheritdoc}
   */
  public function processItem($data) {
    if ($data === 'suspend') {
      throw new SuspendQueueException('The queue is broken.');
    }
    // Do nothing otherwise.
  }

}

When the class contents are removed, the constant PLUGIN_ID is undefined, so self::PLUGIN_ID in the attribute causes an error. I'm not sure how to get around this, so I think this approach might need to be abandoned. I thought I'd document it here regardless.

Got my POC work in MR 8070 to go green. So along with the steps taken in #44 , the next thing I did was eliminate the idea of multiple and automated providers altogether in plugin attribute-based discovery and introduce the idea of manually defined dependencies instead. Complete summary looks something like this:

1. Use PhpToken::tokenize() on the plugin class file
2. Loop through the tokens to find the class declaration token
3. Copy all the class file contents before the class declaration to a string
4. Replace usage of "self" or "static" with the original class name in the string
5. Append a random suffix to the class name for uniqueness, and add a class declaration statement and {} to the string
6. Copy the string to a temporary file and include that file
7. Instantiate ReflectionClass on the new temporary class
8. Extract attribute data from reflection
9. Add a "Dependencies" attribute class that has a "modules" array property which is used to defined other modules the plugin class depends on
10. When parsing the class, look for the dependencies attribute first. If it exists, check to make sure all dependencies are installed, and if any dependencies are missing, prevent the plugin info from being saved to file cache
11. Update the default plugin manager to check that all dependencies are installed in findDefinitions()
12. Add the Dependencies attribute to all migrate source plugin classes that have an implicit dependency on migrate_drupal (or other modules)

This does require developers who write plugin classes to manually account for any implicit dependencies by listing them with the attribute, instead of having discovery determine the providers automatically. While this can be bit of a hassle, I don't think this is a major obstacle, because usually you can determine what dependencies need to be declared by checking the use statements.

This is still POC-ish, but putting back into Needs Review to get feedback on whether this is a good approach. If approach looks good, it might make sense to split out the core discovery part of the work to either #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core or #3395260: Investigate possibilities to parse attributes without reflection first before continuing here. But it was useful to test first with all the converted Migrate Source plugins here. Beyond that, there are probably some tests that need to be written for the revised discovery as well.

Updated IS with summary of alternate suggested approach.

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 MR 6780. There's also a question from #3258581: Move I18nQueryTrait from content_translation to migrate_drupal about how/whether to deprecate I18nQueryTrait (and the classes that use it), given that migrate_drupal will be moved out of core before D12.

Note that I have a proposed alternative approach in MR 8070 that doesn't require the files be moved at all, and I'd appreciate any feedback on whether it's worth going that route.

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 for #50.

Appears most feedback has been addressed but not 100% I can mark this one. But am moving to NW for the deprecations to be updated, unfortunately missed 10.3

cross posting from #3009349: Revert migrate_drupal source annotations to attributes conversion

I think we need to rethink this, and #3009349: Revert migrate_drupal source annotations to attributes conversion in light of #3371229: [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL

source_module is a migrate_drupal thing, as this issue points out, but despite the work here, the source_module key is still a thing defined in migrate as part of the source plugin annotations, and 3421014 is moving it over to the attributes defined in migrate. I'm working out the order of operations here, but I think the concept of source_module as a source plugin attribute/annotation key needs to be deprecated in Drupal 11 along with everything else migrate_drupal.

I have not followed the raw details on the annotation to attribute conversion for plugins, and I'm not sure what would break if we did this, but my gut says that here, we should NOT support source_module as a source plugin attribute, nor should we convert the d7 source plugins that we are deprecating in D11 over to attributes. I'm sure there's some core test somewhere that ensures core plugins which support attributes use them, and we may have to find a way around that for the sources we are deprecating, but I feel like it's worth it, and it saves us from having to deprecate source_module as it can just go away with annotations.

Based on #53, I created MR 9571 that differs from previous in the following ways:

• No D6/D7 source plugins are converted to attributes
• Remaining core source plugins in Migrate module are converted to attributes
• The source_module property was removed from the MigrateSource attribute
• Since so many source plugins are still using annotations, removed the backwards compatibility examples and test that was added
• Since none of the plugins converted to attributes depend on anything other than the core migrate module, automated/multiple provider attribute-based discovery was dropped. There is still a class to handle attributes + automated providers via annotations, but no automated providers via attributes. #3458177: Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency in a way handles "multiple providers" for all plugin types (except for those using other modules' Traits), so it might not be need to be dealt with for Migrate source plugins specifically

Unfortunately, removing the source_module property from the converted plugins embedded_data and content_entity is causing test failures.

Added a commit to MR 9571 that populates the "source_module" property to the definition via the attribute ::get() method. It is kind of ugly, but it does make it easier to remove with the removal of migrate_drupal. Tests are passing again.

I can also update the IS if this route of not converting d6/d7 plugins to attributes is preferred. Putting back in Needs Review for now for feedback.

For now, the canonical MR is 6780.

That MR is marked as a draft, and I also see MR 9571. I am adding the tag for an issue summary update to clarify the situation.

Per conversation with @benjifisher on Slack, setting this back to Needs work for IS update. Will solicit for feedback on best approach again after IS is updated.

Rebased MR 6780 and updated the IS.

I am adding two more related issues mentioned in the issue summary.

Added #3490322: Use nickic/php-parser for attribute-based plugin discovery to avoid errors as related issue - trying to handle missing trait dependency for all plugin discovery. It might make sense to postpone this issue on that one?

Also added #3443882: Allow attribute-based plugins to discover supplemental attributes to set additional properties because it would provided a way to address the source_module property.

Updated the IS. Removed references to MR 8070, because solutions to hitting fatal errors during plugin discovery from missing traits are being explored in #3490322: Use nickic/php-parser for attribute-based plugin discovery to avoid errors.

Questions from the issue summary:

The source_module property in source Annotations is specific to migrate_drupal. Should the source_module property be excluded from the new Attribute, especially since migrate_drupal is slated to be removed? See #53 and #3009349: Revert migrate_drupal source annotations to attributes conversion

That seems like a good idea, but I would like to decouple it from the other problems we have to handle in this issue. If we can get this issue done, then I think we can have a separate issue to remove source_module from Drupal\migrate\Attribute\MigrateSource, create a migrate_drupal class that extends it, and add source_module there.

... When Reflection is instantiated on classes that use unknown Traits, ... PHP will fatal error. There are four plugin classes in the menu_link_content, block_content, and taxonomy modules that use the I18nQueryTrait in the content_translation module. To convert these plugins to use attributes, either ... or these 4 plugins and the trait need to be moved to live in the same module (see #3258581: Move I18nQueryTrait from content_translation to migrate_drupal).

Why do we have to move the four source plugins? Isn't it enough to move the trait to migrate_drupal?

Alternatively, should these plugins (and/or all D6/D7 source plugins) remain unconverted and just keep using annotations until migrate_drupal is removed?

Are we going to start issuing deprecation notices when plugins use annotations instead of attributes? If so, then leaving a bunch of core plugins un-converted will break a lot of automated testing.

... does the automated/multiple provider discovery need to be recreated for Attributes?

We have to continue to support contrib and custom plugins, one way or another. I am not sure whether the automated discovery should be in Core, migrate, or migrate_drupal. In order to control scope, it might make sense to keep it in the migrate module for this issue, and then consider it along with the source_module attribute.

Why do we have to move the four source plugins? Isn't it enough to move the trait to migrate_drupal?

Investigated this and found that an exception is thrown for missing DrupalSqlBase which prevents the fatal error, if only the trait is moved. That work is in MR 10426 for #3258581: Move I18nQueryTrait from content_translation to migrate_drupal and is in Needs Revew.

@godotislate: If I understand Comment #69, then we do not need to move those four source plugins. We just need to move the trait and update the source plugins, which is done in #3258581: Move I18nQueryTrait from content_translation to migrate_drupal.

Let's mark this issue as postponed until #3258581 is Fixed, at which point the MR here will have to be updated. I just marked that issue RTBC.

Are we still using the convention of updating the title of postponed issues? I hope I got it right, adding "[PP-1]".

Now that #3258581: Move I18nQueryTrait from content_translation to migrate_drupal is Fixed, we can un-postpone this one. The MR needs to be updated (simplified) after the work in that issue.

This issue still needs an issue summary update, now that we have agreed on the approach. (Again, that means simplifying.) This issue already has the tag for that.

With #3258581: Move I18nQueryTrait from content_translation to migrate_drupal in, I've rebased MR 6780 and updated it for a couple new additional fixes to get tests passing.

This MR includes converting all existing migrate source plugins from annotations to attributes, though two new test plugins have been added with annotations to test backwards compatibility. In addition, the source_module property is included in the new MigrateSource attribute. There's been discussion that it should not be included because it is strictly for Migrate Drupal, but something like #3443882: Allow attribute-based plugins to discover supplemental attributes to set additional properties would need to get in for source_module to split from the MigrateSource attribute. Currently that issue does not have much momentum, and I'm not sure this issue should continue to be blocked just to wait on that one.

That being said, MR 6780 also adapts the multiple/automated provider discovery from annotations to attributes, which leads to some tangled pseudo-multiple-hierarchical code. Work in either #3502913: Add a fallback classloader that can handle missing traits for attribute discovery or #3490322: Use nickic/php-parser for attribute-based plugin discovery to avoid errors would help simplify this, but it's possible to move forward here regardless and then re-work once one of those issues gets in.

Regardless, MR 6780 has sat a while, so it's worth looking it over again to see if some things can be improved or simplified. I don't have time to do that right away, so I'm leaving this in NW for now for anyone to pick it up.

Rebased, wrapped one line to 80 chars and an issue summary update.

I have reviewed most of the changes in the MR. So far, I think all my suggestions are related to comments (including doc blocks).

There are about 115 files where the only changes are replacing annotations with attributes. Even if I ignore all the source plugins, that leaves 18 files, +468/-54 lines, which is more than enough for one MR. So I am convinced that dealing with source_module in a separate issue is the right decision.

I want to spend some more time reviewing the new automated tests, and I want to do some manual testing. Other than that, and the comment changes I suggested on the MR, I think this issue is ready to go. It is a good sign that the existing tests needed only minor updates.

Given the complexity of this MR and that it is close to ready, I think we should finish this issue instead of waiting on other issues that might make it simpler. We still have a nearly three months before 11.2.0-alpha1 is released. That should give us time to work on some of those issues after this one is fixed.

In addition to the comment changes I requested yesterday, I think the new test needs a little change. Back to NW for that, but I think those are all easy changes to make.

I did some manual testing:

1. Switch to 11.x or the feature branch.
2. Install the standard profile.
3. Enable the migrate module.
4. Use drush php to exercise the plugin manager.
5. Enable the migrate_drupal module.
6. Repeat Step 4.

In Steps 4 and 6, I used the following commands. I am adding linebreaks here for readability, but I used one line to reduce the output.

$source_plugins = Drupal::service('plugin.manager.migrate.source')->getDefinitions();
foreach ($source_plugins as $key => $plugin) {
  ksort($plugin);
  $source_plugins[$key] = $plugin;
}
$source_plugins

I am attaching the results as two text files (one for 11.x, one for the feature branch).

I see the following differences:

1. In 11.x, the value of the 'provider' key is an array. In the feature branch, that array is assigned to 'providers', and the value of the 'provider' key is the first element of that array.
2. In the feature branch, every plugin definition includes "minimum_version" => null,.
3. In 11.x, every plugin derived from content_entity includes "deriver" => "\Drupal\migrate\Plugin\migrate\source\ContentEntityDeriver",. In the feature branch, the leading '\' is missing.
4. In 11.x, every plugin derived from content_entity includes "source_module" => "migrate",. In the feature branch, it is "source_module" => "migrate_drupal",.

Of these, (1) is intentional. (2) may not be important, but perhaps it is worth being consistent. Probably it is worth changing (3). And (4) is a problem: #3498915: Move content_entity source plugin to migrate module moved the content_entity source plugin to the migrate module and updated the source_module annotation, but the current MR missed that. This issue definitely NW for that.

Accepted 2 suggestions and added a change record.

I did some more testing today.

Locally, I merged with the current 11.x branch.

Testing with migrate_drupal

First, I installed a Drupal 7 site and a Drupal 11 site (using the feature branch). On the Drupal 7 site, I created 100 article and page nodes, using the devel_generate module. On the Drupal 11 site, I installed the migrate_drupal_ui module. I then imported everything from the Drupal 7 site.

There were just a couple of migration messages (from the user migration, which ran into some unsupported permissions, and the d7_system_file migration). I got the same messages when I repeated the test with the 11.x branch.

All the content (users, files, taxonomy terms, nodes) imported as expected.

Testing with migrate_plus

I re-installed Drupal 11 (using the feature branch). I then added the Import CSV Recipe, which installs some contrib modules (migrate_plus, migrate_source_csv, migrate_source_ui) and a few migrate_plus.migration.* config entities. Following the instructions in the README, I created some users, taxonomy terms, and nodes using the example CSV files from the recipe. It all worked as expected.

Summary

I see no problems in today's manual testing.

I believe all latest MR feedback comments have been addressed.

@godotislate:

Thanks for the updates. I agree: you have addressed all my comments on the MR.

The only changes since my last round of testing (Comment #81; see also Comment #79) are a couple of comments and one test file, so I do not need to re-test.

Let's move this issue along!

This looks good.

Some of the test coverage added depends on migrate_drupal which we're intending to deprecate for removal in 12.x but it won't be the only test coverage depending on migrate_drupal so can be handled as part of that process.

There are a couple of things that are quite complex here but they're covered by existing open issues and/or the eventual deprecation of annotations.

Committed/pushed to 11.x, thanks!