Update MigratePluginManager to include both attribute and annotation class

Put up MR with one approach to tackle BC for migrate plugin manager.

Let's test this by converting one process plugin, 'explode'.

Left a small change and one question.

MR comments addressed.

Thanks for answering that for me @godotislate

Thanks for the reviews!

I think this should include the conversion of a source, destination and field plugin as well as discussed in Slack.https://drupal.slack.com/archives/C1BMUQ9U6/p1709180072609059. Lets make sure that all plugin types that use this manager can work.

Originally pushed conversion for at least attribute of each type, but tests failed on the source plugin. I cherry-picked from MR 6780 for 📌 Convert MigrateSource plugin discovery to attributes Needs work , and it looks like work in that issue for the Attribute class is incomplete, including an outstanding question on how to handle multiple provders. So I reverted the changes for Migrate source, and test are passing for one plugin of each of the other three.

Migrate source plugins have their own discovery using, \Drupal\migrate\Plugin\Discovery\AnnotatedClassDiscoveryAutomatedProviders and they also have to handle multiple providers, \Drupal\migrate\Annotation\MultipleProviderAnnotationInterface. I am not familiar with those and am just starting to take a look.

This may need to be split up but I did find it helpful to work on all these at one time. The most problematic is the source plugins because it requires multiple providers which meant changes to discovery. And adding a class property, $providers.

A couple comments/questions on the MR.

Also, I wonder if the work to deal with sources having multiple providers shouldn't be done in 📌 Convert MigrateSource plugin discovery to attributes Needs work , so iterating on that part doesn't block getting the other plugin types done.

fixed feedbacks from MR

Add the two generic plugins that also need the MigratePluginManager

Added commas to meet Coding Standards per Function Declarations → , which is a new addition to the standards.

Left 1 question but will leave in review for additional eyes. Based on other attribute tickets change look good to me.

I reviewed MR 6827. I have a bunch of minor suggestions for code cleanup, which I will add to the MR as suggestions. This is the first time that I have looked at attributes for plugin definitions, so take my suggestions with a grain of salt. (That is, do not be surprised if some suggestions seem like bad ideas.)

This MR has conflicts with 📌 Convert the PHPStan baseline from NEON to PHP Fixed . One commit adds an exception to the PHPStan baseline, and a later commit removes that exception. If you remove those two changes, then you should be able to rebase cleanly.

I think all MR feedback has been addressed with changes or commented on.

@godotislate:

Thanks. I am not sure when I will have time to re-review. Also, I am not sure I am qualified to declare the issue RTBC. As I said, this is the first time I have looked at attributes.

Do we need additional test coverage, or are the existing tests good enough now that we have converted a few plugins?

I think we should at least add a test to confirm that the system works with a combination of annotation and attribute plugins. Once we convert Drupal core to attributes, there will still be contrib modules using annotations.

Do we need additional test coverage, or are the existing tests good enough now that we have converted a few plugins?

I think we should at least add a test to confirm that the system works with a combination of annotation and attribute plugins. Once we convert Drupal core to attributes, there will still be contrib modules using annotations.

This thought did occur to me. We think things are fine now, because in the MR we have a few plugins converted to attributes and most of the rest left with annotations, with all tests passing. But once all the core annotations are removed, annotation support for migration multiple providers might not be directly tested anymore. Looks like for other plugin types there is Drupal\Tests\Component\Annotation\AnnotatedClass\DiscoveryTest, so we'll need something similar for migration source plugins.

Added the test for backwards compatibility for annotated migrate source plugins.

Appears all feedback has been addressed. Went through one more time and I don't have any questions

Added some comments to the MR.

Addressed most comments. Left questions open about requirements_met functionality for anyone more familiar with that than me.

Changing status for another review.

Also, it needs to be decided if all the migrate plugins will be converted in this issue. This issue is already rather large, 37 files + 763 − 105.

To help with the decision there are:

• 118 MigrateSource plugins
• 24 MigrateFielld plugins
• 79 MigrateProcessPlugins plugins
• 31 MigrateDestination plugins

That seems like a lot of additional files to look at. I think those can be done in the original migration plugin type conversion tickets and reviewed separately.

Is there any pro to combining? Seems like keeping separate makes the most sense from a reviewing standpoint.

I think it would be fine to do those in the four existing issues that are postponed on this one.

Sweet, looking at the MR I believe all threads have been answer, there any outstanding questions?

Going to go ahead and mark it as I didn't see any outstanding threads

Committed and pushed c9d53fd473 to 11.x and d7f89942a7 to 10.3.x. Thanks!

I've had a good look and the changes to the plugin managers to support multiple providers and I think this is the best we can do. It would be lovely to move this upstream from migrate to core somehow.

godotislate → closed merge request !6827

The issue to move the support for multiple providers to the plugin system is #2786355: Move multiple provider plugin work from migrate into core's plugin system →

Reverted because this was requiring source_module on all source plugins, when it is really just for migrate drupal.

quietone → reopened merge request !6827

I remembered 📌 Move source_module and destination_module from Migrate to Migrate Drupal Needs work . So I have updated that and it looks like it will help with the problem of the source_module.

I think the best way to keep moving forward is to de-scope everything to do with migrate source plugins from this issue and handle that in 📌 Convert MigrateSource plugin discovery to attributes Needs work . The remainder of the reverted commit for this issue can be brought back here.

I've put up MR 7327 for the proposed changes for this issue:

• MigratePluginManager
• Destination, Process, and Field attributes
• Sample destination, process, and field plugins
• Doc block references to destination, process, and field annotation classes

Remaining work to do in in 📌 Convert MigrateSource plugin discovery to attributes Needs work :

• Automated/multiple provider handling
• MigrateSourceManager
• Source plugins
• Source attribute
• Docblock references to source annotations

A lot of the migrate source work will just be restoring the remaining diff from the reverted commit. Along with dealing with the source_module property in the attribute, and then converting the remainder of the plugins.

I'm in favor of breaking out more. Makes reviews simpler and know we shouldn't assume anything but I can put money the other postponed tickets will be addressed before 11.x release, they got all the attention right now haha.

So looking at MR 7327 tests are green from the re-scope.

Committed and pushed 7dd6b190eb to 11.x and b7e0c8e94b to 10.3.x. Thanks!

Great idea to get most of this done.

I've updated all the all issues postponed on this one.

godotislate → closed merge request !7327

godotislate → closed merge request !6827

Automatically closed - issue fixed for 2 weeks with no activity.