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 📌 Update MigratePluginManager to include both attribute and annotation class Fixed

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

Blocked on 📌 Update MigratePluginManager to include both attribute and annotation class Fixed

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<code> is meant to throw an exception for missing source module in <code>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. See Drupal\migrate_drupal\MigrationPluginManager::processDefinition() and ::getEnforcedSourceModuleTags()<code>. Now that <code>source_module is a required property in the MigrateSource attribute from 📌 Update MigratePluginManager to include both attribute and annotation class Fixed , 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 📌 Update MigratePluginManager to include both attribute and annotation class Fixed 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 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work 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 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work 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

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 📌 Hidden dependency on block_content in layout_builder Needs work . 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 ( 📌 [random test failures] Use key/value directly instead of state in test modules Active 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 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 really affects those and is 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: Move multiple provider plugin work from migrate into core's plugin system → ?

Left a review on the MR - nice work - this looks like a gnarly one

Blocked on deprecating I18nQueryTrait and the plugin classes that use it, and moving them to migrate_drupal in 🐛 Move content_translation I18nQueryTrait to migrate module Active .

Updated IS with remaining tasks

godotislate → changed the visibility of the branch 3421014-alternate-attribute-parsing to hidden.

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 📌 Convert MigrateSource plugin discovery to attributes Active , 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: Move multiple provider plugin work from migrate into core's plugin system → or 📌 Investigate possibilities to parse attributes without reflection Active 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 🐛 Move content_translation I18nQueryTrait to migrate module Active 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 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work

I think we need to rethink this, and 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work in light of 📌 [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL Fixed

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 📌 Convert MigrateSource plugin discovery to attributes Active , 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. 🐛 Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency Needs review 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.

godotislate → changed the visibility of the branch 3421014-migrate-source-no-migrate-drupal to hidden.

Rebased MR 6780 and updated the IS.

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

Added 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active 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 ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active 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 because of missing traits is being explored in 📌 Use alternate parsing method for attribute-based plugin discovery to avoid errors Active .

godotislate → closed merge request !8070

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 📌 Convert MigrateSource plugin discovery to attributes Active and 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work

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 🐛 Move content_translation I18nQueryTrait to migrate module Active ).

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 🐛 Move content_translation I18nQueryTrait to migrate module Active and is in Needs Revew.