Problem/Motivation

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.

Proposed resolution

  1. Update Drupal\migrate\Plugin\MigratePluginManager constructor to include attribute and annotation class names

The plugins converted here are

  • Book destination plugin
  • DateField field plugin
  • EntityFile destination plugin
  • Explode process plugin
  • NullIdMap
  • Sql

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3424509

Command icon Show commands

Start 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:

About issue forks

Comments

godotislate created an issue. See original summary.

godotislate opened merge request !6827

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented
Status: Active » Needs review

Put 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.

quietone made their first commit to this issue’s fork.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

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

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Left a small change and one question.

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented
Status: Needs work » Needs review

MR comments addressed.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Thanks for answering that for me @godotislate

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Reviewed & tested by the community » Needs work

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.

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented
Status: Needs work » Needs review

Originally pushed conversion for at least attribute of each type, but tests failed on the source plugin. I cherry-picked from MR 6780 for #3421014: [PP-1] 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.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs review » Needs work

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.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue summary: View changes
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review

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.

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented
Status: Needs review » Needs work

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 #3421014: [PP-1] Convert MigrateSource plugin discovery to attributes, so iterating on that part doesn't block getting the other plugin types done.

sorlov made their first commit to this issue’s fork.

sorlov’s picture

sorlov CreditAttribution: sorlov at Skilld commented
Status: Needs work » Needs review

fixed feedbacks from MR

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue summary: View changes

Add the two generic plugins that also need the MigratePluginManager

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

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

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

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

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher as a volunteer commented
Status: Needs review » Needs work

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

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented
Status: Needs work » Needs review

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

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
CreditAttribution: benjifisher as a volunteer commented

@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.

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented
Status: Needs review » Needs work
Issue tags: +Needs tests

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.

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented
Status: Needs work » Needs review
Issue tags: -Needs tests

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

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work

Added some comments to the MR.

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review

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.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

To help with the decision there are:

  • 118 MigrateSource plugins
  • 24 MigrateFielld plugins
  • 79 MigrateProcessPlugins plugins
  • 31 MigrateDestination plugins
godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented

That 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.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

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

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

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

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

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

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed d7f89942 on 10.3.x 
    Issue #3424509 by godotislate, quietone, sorlov, smustgrave, benjifisher...

  • alexpott committed c9d53fd4 on 11.x 
    Issue #3424509 by godotislate, quietone, sorlov, smustgrave, benjifisher...

godotislate closed merge request !6827

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Related issues: +#2786355: Move multiple provider plugin work from migrate into core's plugin system

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

  • quietone committed 580bfcb7 on 11.x 
    Revert "Issue #3424509 by godotislate, quietone, sorlov, smustgrave,...

  • quietone committed effc39d1 on 10.3.x 
    Revert "Issue #3424509 by godotislate, quietone, sorlov, smustgrave,...
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Version: 10.3.x-dev » 11.x-dev
Status: Fixed » Needs work

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

quietone reopened merge request !6827

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Related issues: +#3009349: [PP-2] Move source_module from Migrate to Migrate Drupal

I remembered #3009349: Move source_module from Migrate to Migrate Drupal. So I have updated that and it looks like it will help with the problem of the source_module.

godotislate opened merge request !7327

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented
Status: Needs work » Needs review

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 #3421014: [PP-1] 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:

  • 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 #3421014: [PP-1] Convert MigrateSource plugin discovery to attributes:

  • 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.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

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

Great idea to get most of this done.

  • alexpott committed b7e0c8e9 on 10.3.x 
    Issue #3424509 by godotislate, quietone, sorlov, smustgrave, alexpott,...

  • alexpott committed 7dd6b190 on 11.x 
    Issue #3424509 by godotislate, quietone, sorlov, smustgrave, alexpott,...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

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

godotislate closed merge request !7327

godotislate’s picture

godotislate CreditAttribution: godotislate at Digital Polygon commented
Issue summary: View changes

godotislate closed merge request !6827

Status: Fixed » Closed (fixed)

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