Revert migrate_drupal source annotations to attributes conversion

Here's an attempt at getting all mentions of source_module out of migrate. I didn't think too hard about BC or deprecations or anything like that. Not sure what tests might fail. We'll see. This'll be fun.

I ran the test I moved from migrate to migrate_drupal and fixed it up so it would pass. Other than that though I'm flying blind.

As for the destination_module, I'm not totally sold that it needs to be removed. We'll still be migrating into Drupal even aren't migrating from Drupal. Alas it appears the migration meeting mentioned in the IS is from the before times when the migration meetings were ephemeral calls with no minutes, so I couldn't find anything on the record about the thoughts there.

removing un-unused use.

This patch should take care of a lot of those failures. I didn't test all of them locally, but enough to feel like this is progress.

This should make everything green.

It's green!

Here's an updated patch that takes deprecation seriously. It also adds test coverage for the new DrupalEmbeddedData class, for which I had not yet taken the time to add a test.

To be clear, I have still only worked on source_module.

Dag nabbit. Fixing the namespace of the new test.

NW on comma, but also maybe PP on #3250582: ResponsiveImageStyles source plugin must extend DrupalSqlBase

This one needs a re-roll due to the related issue that has been fixed: #3250582: ResponsiveImageStyles source plugin must extend DrupalSqlBase

Also sounds like @quietone is planning on updating the deprecation notice:

+++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
@@ -118,6 +118,11 @@ public function getIds();
+   *
+   * @deprecated in drupal:9.4.0 and is removed from drupal:10.0.0.
+   *   Instead, you should use
+   *   Drupal\migrate_drupal\Plugin\DrupalSourceInterface::getSourceModule().
+   * @see https://www.drupal.org/node/3009349

In addition to changing the drupal versions, we should update the link as well. It's pointing at this issue. We should make a change record and point at the change record instead. Tagging for the CR.

Added reroll of patch #22 on Drupal 9.5.x. keep the status needs work for change record.

I like the new approach, @quietone. I have a couple three comments.

1. There are a couple source plugins that still declare source_module that aren't extending DrupalSqlBase. Specifically:

• MigrateExternalTranslatedTestSource
• CacheableEmbeddedDataSource

We should probably remove source_module from their annotations.

2. I like the idea of deprecating the md_empty plugin if possible. :)

3. The CR is still reflecting the old "all source modules must go!" approach. I'll tag for a CR update.

can public function getSourceModule() { be typehinted.

Should destination_module be a follow up?

Hiding all the patches for clarity.

Applied a typehint suggestion directly to the MR.

Rest appears fine.

Thanks for updating

Opened #3438187: Move destination_module from Migrate to Migrate Drupal as a follow up for destination_module.

Shouldn't we be issuing a deprecation for non migrate drupal source plugins that have a source_provider key?

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.

All feedback appears to be addressed

@quietone If we don't issue a deprecation as suggested in #50 how is someone to know that they should remove the source_module from their non-drupal plugins - like core has done?

I think we should do something like

      $source_id = $definition['source']['plugin'];
      $source_definition = $this->sourceManager->getDefinition($source_id);
       if (in_array('migrate_drupal', $source_definition['provider'], TRUE) && empty($source_definition['source_module'])) {
         throw new BadPluginDefinitionException($source_id, 'source_module');
       }
       if (!in_array('migrate_drupal', $source_definition['provider'], TRUE) && !empty($source_definition['source_module'])) {
        @trigger_error('Something about having an unnecessary key. See CR', E_USER_DEPRECATED);
       }

For the deprecation.

Since this has been 3 months to help it not stale going to mark it for committers queue since only open feedback is around the deprecation or not.

I think we need to rethink this, and #3421014: Convert MigrateSource plugin discovery to attributes 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 in 3421014, 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.

We (@benjifisher, @godotislate, @mikelutz, and @quietone, with some input from @larowlan) discussed this issue on Slack (https://drupal.slack.com/archives/C226VLXBP/p1736647603190789).

Despite Comment #60, we agreed to convert annotations to attributes first, and then to deal with source_module, so I am postponing this issue on #3421014: Convert MigrateSource plugin discovery to attributes. Once that issue is done, we will have to completely rewrite the MR here, but I think that will not be hard. We have already done the work of deciding what has to be done, and all we will have to do is re-implement it.

When I am feeling really optimistic, I think that if we can get both issues done before 11.2.0 is released, then we can skip the deprecation step. If source_module never exists as an attribute in a release of Drupal core, do we have to deprecate it?

Now that #3258581: Move I18nQueryTrait from content_translation to migrate_drupal is Fixed, we can change this one from PP-2 to PP-1.

Now that #3421014: Convert MigrateSource plugin discovery to attributes is fixed, we can return to this issue. In an earlier comment, I suggested that we would have to rewrite the MR for this issue, but I changed my mind. I rebased the MR, resolving conflicts between this issue and #3421014.

I think we need to update the issue summary with the current proposed resolution.

I think that Drupal\migrate_drupal\MigrationPluginManager::processDefinition() needs some further work. I have to think about whether @alexpott's comment on that issue is still valid, or if the recent changes make it obsolete.

I implemented @alexpott's suggestion: trigger a deprecation warning if there is a source_module annotation on a source plugin (or in its configuration) if it is not needed. That led to a complete rewrite of Drupal\migrate_drupal\MigrationPluginManager::processDefinition().

Then I created the MigrateDrupalSource attribute class, extending MigrateSource, and moved the source_module property to the new class. And I updated most code source plugins to use the new attribute class. This change breaks BC, but that is OK if we get it done before Drupal 11.2.0 is released.

Let's see what the testbot thinks.

Then I created the MigrateDrupalSource attribute class, extending MigrateSource, and moved the source_module property to the new class.

The unfortunate issue with an attribute being defined in a module separate from the one that has the the main attribute is this:

• Site has the migrate and my_module modules installed, but not migrate_drupal
• On cold caches, migrate source plugin discovery iterates through all the plugin directories and picks up definitions using the MigrateSource attribute. The MigrateDrupalSource attribute on any class is ignored as a comment, since the attribute class does not exist with the module uninstalled. This all works as expected without error
• However, once discovery iterates through a directory, the definition data for all the files in that directory are saved in a file cache in APCu, and the entries in that file cache are only invalidated by change in file mtime
• So, assuming you have a migrate source plugin Drupal\my_module\Plugin\migrate\source\MyMigrateDrupalSource that uses the MigrateDrupalSource attribute, the entry associated with the class saved in the file cache will be NULL
• If you install migrate_drupal, the file cache data is not invalidated, so the NULL value for MyMigrateDrupalSource is retrieved from cache. MyMigrateDrupalSource.php doesn't get parsed for the attribute data, and the definition is not discovered

There are some attempts to deal with a very similar problem in #3443882: Allow attribute-based plugins to discover supplemental attributes to set additional properties.

Can we manually clear that apcu cache in migrate drupal's hook_install? along with the source plugin discovery cache?

@godotislate:

Thanks for explaining that!

In Comment #66, I wrote:

This change breaks BC, but that is OK if we get it done before Drupal 11.2.0 is released.

The MigrateSource attribute was defined in #3421014: Convert MigrateSource plugin discovery to attributes (I know you know, but I want a convenient link), and that was committed to the 11.x branch only (not to 11.1.x, not in any release).

I was going to say that any module that uses MigrateDrupalSource should declare a dependency on migrate_drupal, and that we should say so in the change record. Then I realized that there are a bunch of core modules that define D6/D7 sources, and those modules should not depend on migrate_drupal.

I see your point.

What options do we have?

1. Clear that apcu cache in migrate_drupal's hook_install (Comment #68).
2. Fix #3443882: Allow attribute-based plugins to discover supplemental attributes to set additional properties and this issue before 11.2.0 is released.
3. Revert the part of #3421014 that converts most core source plugins to attributes, and the commit for this issue that updates those source plugins.

Any others?

I think the third option is the simplest. We discussed that option on #3421014, but I (and maybe others) did not understand the implications.

@mikelutz we can't rely on clearing the apcu cache because e.g. if apcu is cleared from the command line it won't affect web requests and vice versa.

However, the cache key is set in AttributeDiscoveryWithAnnotations::getFileCacheSuffix and I think we could key that by rootNamespaces which is iirc more or less the module list, that way it will always depend on the combination of modules enabled.

I was about to write that this might slow down the installer, but we barely/ever discover plugins during install so it shouldn't make much difference at all.

Getting #3443882: Allow attribute-based plugins to discover supplemental attributes to set additional properties would be great, but I think it's at an impasse for now on what approach to go forward with.

Another idea would be to remove the source_module property from the MigrateSourceattribute, and use hook_migrate_source_info_alter() implementations to add the source_module property to the relevant definitions. This would be similar to the approach taken for the Navigation module in #3443833: Provide a way for other modules to flag block plugin implementations as 'navigation safe', where hook_block_alter() implementations can be used to add the allow_in_navigation property to block plugin definitions. For migrate_drupal source plugins, this probably could be done as one single alter hook in migrate_drupal for all the relevant plugins throughout core modules.

However, the cache key is set in AttributeDiscoveryWithAnnotations::getFileCacheSuffix and I think we could key that by rootNamespaces which is iirc more or less the module list, that way it will always depend on the combination of modules enabled.

@catch Any concern about possibly having multiple entries, based on combination of modules installed, for just about every file in the plugin directories for every plugin type? Though this would be mitigated once #3490484: [meta] Lots of non-plugin PHP classes in plugin directories is addressed.

This could be isolated to migrate source plugin discovery, because it has its own classes AttributeClassDiscoveryAutomatedProviders and AttributeDiscoveryWithAnnotationsAutomatedProviders. On the other hand, it's probably not ideal to make migrate source discovery any more special than it already is.

I am still of the opinion that we shouldn't have converted any of d6 and d7 source plugins to attributes at all. Leave source_module in the annotation, leave all the sources as annotations, and depreciate them. They all have to go before D12 anyway. There was never any reason to deal with source module in attributes at all. We haven't done a release with the conversion yet, we could still execute it this way and not worry about it.

We haven't done a release with the conversion yet, we could still execute it this way and not worry about it.

I agree this seems to be the most straightforward way ahead in that it isn't dependent on resolving blocker issues on the plugin subsystem as a whole.

"catch Any concern about possibly having multiple entries, based on combination of modules installed, for just about every file in the plugin directories for every plugin type? Though this would be mitigated once #3490484: [meta] Lots of non-plugin PHP classes in plugin directories is addressed."

So yes,but I have an issue open for a database-backed filecache backend which needs a lot of work, but plugin discovery would be a good candidate. We could put it in memcache/redis in contrib too. Would be accessible to all processes and free up apcu for frequently accessed things.

I think Comment #73 and the third option in Comment #69 are the same thing. Let's do that. (As soon as I have a little spare time.)

For the record: another option is to move all the source plugins (except those in the migrate module) to the migrate_drupal module.

I do not have time now to make the agreed changes, but this commit should fix some of the failing tests.

Four of the migrations in the block_content module

• Use the embedded_data source plugin;
• Have the "Drupal 6" or "Drupal 7" migration tag (in fact, the migrations have both tags);
• Include source_module in the source configuration.

Maybe I should remove the source_module from those migrations, but for now I am changing the logic so that we do not get a deprecation message for these. See Comments #50, #56.

fwiw I think it's completely reasonable to leave the source plugins using annotations, we probably won't actually remove annotation support from core until Drupal 13 (although I really hope we can deprecate it by Drupal 12), and when it's deprecated it just means @group legacy on tests.

I am mostly reverting the source plugins to be the same as in the 11.1.x branch, but I have to be careful not to revert the changes from #3258581: Move I18nQueryTrait from content_translation to migrate_drupal and another issue or two.

This reverts what I said in the second half of Comment #66 along with a lot of what we did in #3421014: Convert MigrateSource plugin discovery to attributes.

As long as we are removing source_module from the MigrateSource attribute class, should we also get rid of these?

   * @param bool $requirements_met
   *   (optional) Whether requirements are met. Defaults to true. The source
   *   plugin itself determines how the value is used. For example, Migrate
   *   Drupal's source plugins expect source_module to be the name of a module
   *   that must be installed and enabled in the source database.
   * @param mixed $minimum_version
   *   (optional) Specifies the minimum version of the source provider. This can
   *   be any type, and the source plugin itself determines how it is used. For
   *   example, Migrate Drupal's source plugins expect this to be an integer
   *   representing the minimum installed database schema version of the module
   *   specified by source_module.

If so, do that in this issue or in a follow-up?

I am adding a few items to the "Remaining Tasks" in the issue summary.

I think a follow-up for #79 might be a good idea. I assume we're only using those for migrate_drupal but I couldn't answer the question whether contrib might possibly be relying on them etc. so if it doesn't make this issue easier to include here, splitting off sounds good.

@catch, @quietone: Thanks for the prompt replies!

Earlier, I added this question to the Remaining Tasks:

Decide when to issue a deprecation message because source_module is present, but should not be, See Comments #50, #56, #77.

I think the current version of the MR gets it right: only issue a deprecation if source_module is present AND migration_tags include neither "Drupal 6" nor "Drupal 7" AND migrate_drupal is not one of the providers.

Here is why that is the right decision. I ran into some failing tests because there are 4 migrations in the block_content module that use the embedded_data source plugin and have the source_module property. At first, I deleted source_property, but then the ValidateMigrationStateTest tests failed. So we need to allow source_module if there is an appropriate tag.

As a bonus, I finally found the right way to fix the deprecations: make sure that config is installed in MigrationPluginListTest::testGetDefinitions() so that MigrationPluginManager::getEnforcedSourceModuleTags() (the migrate_plus version) works.

Even though this issue still needs an update to the summary and the change record, I am going to say it is ready for review.

I am adding a detailed list of changes to the issue summary.

While I was preparing this summary, I noticed that a test had an @group migrate_drupal_ui annotation even though it was in the migrate module and this issue moves it to the migrate_drupal module. I changed it to @group migrate_drupal.

I added #3518890: Remove mimimum_version and requirements_met from MigrateSource attribute class, so I can cross off another one of the remaining tasks.

Marking this as critical and an alpha blocker, as this is undoing changes from earlier in the 11.2 development cycle that we absolutely do not want to see in a release.

I did a quick review of the new MR, and at a glance, everything looks good. I want to spend a little more time and/or have a couple of other sets of eyes on it before I RTBC, but I do think this is likely good to go.

I rewrote the change record.

I will also update Plugins converted from Annotations to Attributes in 11.2.0, the change record for #3421014: Convert MigrateSource plugin discovery to attributes.

I just rebased the MR on the current 11.x. There were merge conflicts from #3506605: Move the d8_config source plugin to the migrate module, and I also checked #3515213: Convert recently added test plugin to use attributes.

Details: #3506605 deprecates the d8_config source plugin in the migrate_drupal module. Part of that deprecation is overriding the constructor from the parent class and triggering a deprecation warning. Before that issue, the class did not override the constructor, so it did not need use statements for the constructor's arguments. The only merge conflicts were in the use statements at the top of the file.

#3506605 also adds a new source plugin. I forgot to remove source_module from that plugin during the rebase, but PHPStan reminded me.

Okay, I did some further review on this, and it's looking good. I did some git magic to compare this MR against what 11.x would look like with the annotation conversion reverted to make sure all the expected source plugins were reverted and they were.

RTBC from me.

Updated the issue summary.

Committed/pushed to 11.x, thanks!

Re-titling this because the eventual change was different to what happened, unfortunately didn't realise this before commit.