Drupal\migrate\Plugin\MigratePluginManager is the base class for plugin managers for 4 different migrate plugin types: MigrateSource, MigrateDestination, MigrateProcessPlugin, and MigrateField. In order to convert the 4 plugin types to use attributes, MigratePluginManager needs to be updated first.
This is exploring the changes required for Drupal\migrate\Plugin\MigratePluginManager and children.
The plugins converted here are
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
godotislatePut up MR with one approach to tackle BC for migrate plugin manager. It duplicates the is_subclass_of() check from DefaultPluginManager. I think it's a little ugly because of that, but otherwise the other idea I had originally and rejected was to pass the $attribute parameter last into the MigratePluginManager constructor. That could require contrib/custom code to rework construcutors again later when annotations are removed, so I think this is a better stopgap.
Comment #5
quietone commentedLet's test this by converting one process plugin, 'explode'.
Comment #6
smustgrave commentedLeft a small change and one question.
Comment #7
godotislateMR comments addressed.
Comment #8
smustgrave commentedThanks for answering that for me @godotislate
Comment #9
quietone commentedThanks 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.
Comment #10
godotislateOriginally pushed conversion for at least attribute of each type, but tests failed on the source plugin. I cherry-picked from MR 6780 for #3421014: Convert MigrateSource plugin discovery to attributes, 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.
Comment #11
quietone commentedMigrate 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.
Comment #12
quietone commentedComment #13
quietone commentedThis 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.
Comment #14
godotislateA couple comments/questions on the MR.
Also, I wonder if the work to deal with sources having multiple providers shouldn't be done in #3421014: Convert MigrateSource plugin discovery to attributes, so iterating on that part doesn't block getting the other plugin types done.
Comment #16
sorlov commentedfixed feedbacks from MR
Comment #17
quietone commentedAdd the two generic plugins that also need the MigratePluginManager
Comment #18
quietone commentedAdded commas to meet Coding Standards per Function Declarations, which is a new addition to the standards.
Comment #19
smustgrave commentedLeft 1 question but will leave in review for additional eyes. Based on other attribute tickets change look good to me.
Comment #20
benjifisherI 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 #3426548: Convert the PHPStan baseline from NEON to PHP. 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.
Comment #21
godotislateI think all MR feedback has been addressed with changes or commented on.
Comment #22
benjifisher@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.
Comment #23
godotislateThis 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.Comment #24
godotislateAdded the test for backwards compatibility for annotated migrate source plugins.
Comment #25
smustgrave commentedAppears all feedback has been addressed. Went through one more time and I don't have any questions
Comment #26
alexpottAdded some comments to the MR.
Comment #27
godotislateAddressed most comments. Left questions open about
requirements_metfunctionality for anyone more familiar with that than me.Comment #28
quietone commentedChanging 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.Comment #29
quietone commentedTo help with the decision there are:
Comment #30
godotislateThat seems like a lot of additional files to look at, even if the diff in each individual file could be small. I think those can be done in the original migration plugin type conversion tickets and reviewed separately.
Comment #31
smustgrave commentedIs there any pro to combining? Seems like keeping separate makes the most sense from a reviewing standpoint.
Comment #32
catchI think it would be fine to do those in the four existing issues that are postponed on this one.
Comment #33
smustgrave commentedSweet, looking at the MR I believe all threads have been answer, there any outstanding questions?
Comment #34
smustgrave commentedGoing to go ahead and mark it as I didn't see any outstanding threads
Comment #35
alexpottCommitted 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.
Comment #39
quietone commentedThe issue to move the support for multiple providers to the plugin system is #2786355: Convert multiple provider plugin work from migrate into plugin dependencies in core
Comment #42
quietone commentedReverted because this was requiring source_module on all source plugins, when it is really just for migrate drupal.
Comment #44
quietone commentedI remembered #3009349: Revert migrate_drupal source annotations to attributes conversion. So I have updated that and it looks like it will help with the problem of the source_module.
Comment #46
godotislateI 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 #3421014: Convert MigrateSource plugin discovery to attributes. 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:
Remaining work to do in in #3421014: Convert MigrateSource plugin discovery to attributes:
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.
Comment #47
smustgrave commentedI'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.
Comment #48
alexpottCommitted and pushed 7dd6b190eb to 11.x and b7e0c8e94b to 10.3.x. Thanks!
Great idea to get most of this done.
Comment #51
alexpottI've updated all the all issues postponed on this one.
Comment #53
godotislate