Problem/Motivation

Number, email, and reference field types are defined in the Core library, and when it came time to create MigrateField plugins for these fields, they were placed in the Core library.

However, these plugins extend MigrateFieldPluginBase from the migrate_drupal module, and depend on Interfaces from migrate and migrate_drupal modules. Code that depends on code in core modules should not be located in the Core Libraries.

Proposed resolution

Move plugins supporting field migrations of core fields into the field module, configure deprecations on the current classes in case anyone has extended them.

Remaining tasks

Do it.

User interface changes

none

API changes

None, but location of the plugins will change. This is allowed per BC policy, but we can leave deprecated BC shims in place until Drupal 9.

Data model changes

None.

CommentFileSizeAuthor
#7 interdiff.3008950.3-7.txt1.79 KBmikelutz
#7 3008950-7.drupal.Move-MigrateField-Plugins-out-of-Core-Library.patch11.34 KBmikelutz
#3 interdiff.3008950.2-3.txt1.99 KBmikelutz
#3 3008950-3.drupal.Move-MigrateField-Plugins-out-of-Core-Library.patch11.37 KBmikelutz
#2 3008950-2.drupal.Move-MigrateField-Plugins-out-of-Core-Library.patch9.38 KBmikelutz

Comments

mikelutz created an issue. See original summary.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Active » Needs review
StatusFileSize
new 3008950-2.drupal.Move-MigrateField-Plugins-out-of-Core-Library.patch9.38 KB
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
StatusFileSize
new 3008950-3.drupal.Move-MigrateField-Plugins-out-of-Core-Library.patch11.37 KB
new interdiff.3008950.2-3.txt1.99 KB

Adding a test for the @trigger_errors.

Status: Needs review » Needs work

The last submitted patch, 3: 3008950-3.drupal.Move-MigrateField-Plugins-out-of-Core-Library.patch, failed testing. View results

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs work » Needs review
quietone’s picture

quietone commented
Status: Needs review » Needs work

Just two things.

  1. +++ b/core/modules/field/src/Plugin/migrate/field/d7/EntityReference.php
@@ -0,0 +1,34 @@
+ * @see https://www.drupal.org/node/3009286

    Remove. copy/paste error I suspect. 

  2. +++ b/core/tests/Drupal/Tests/Core/Field/MigrateFieldDeprecationTest.php
@@ -0,0 +1,45 @@
+ * @group Field

    Both 'Field' and 'field' are in core. Which is correct?

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3008950-7.drupal.Move-MigrateField-Plugins-out-of-Core-Library.patch11.34 KB
new interdiff.3008950.3-7.txt1.79 KB

Good catch. Yes, copy/paste error, I really thought I got them all. :-/ I also found another copy/paste in the tests where I used the same plugin id for each. That string doesn't matter in this context, but it looks like it makes more sense now.

As far as I can tell, @group field is the field module, while @group Field is the lib/Drupal/Core/Field library. The plugins themselves are tested through the migrate_drupal and field modules field migration tests, (with implicit coverage coming in #2951550: Make service for field discovery for use in migrate entity derivers). The test I added is in the core test suite as part of the core field library, to test that those deprecated classes in the core field library are triggering the error, so I believe @group Field is correct, and it matches the group of the other tests in the directory where i put it.

quietone’s picture

quietone commented
Status: Needs review » Needs work

And you too on fixing the test strings.

I have applied the patch, tested and read the code and have one, hopefully, last question. When reading the CR I was surprised that there was no description. It took a moment to reread the title for it all to make sense. That makes we wonder if there should be a description that says that this is informative and no code changes are required. I mean, at first I thought it was an error that there was no description. This reminds of those times reading official documents where it is written 'this page intentionally left blank'. Or I am just being weird?

Setting to NW for this question.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Issue tags: +Needs change record updates

I just forgot to finish it. I created it to reserve the URL for the documentation, and didn't finish writing it. I'll do it in the morning.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs work » Needs review
Issue tags: -Needs change record updates

CR Updated. Thanks again for reviewing this, @quietone!

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

Sweet, thanks. I made one small change to the CR, removing the '****' since it seems unnecessary to explaining the original directory of the plugins.

Off we go!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 00cfac4 and pushed to 8.7.x. Thanks!

  • catch committed 00cfac4 on 8.7.x 
    Issue #3008950 by mikelutz, quietone: Move MigrateField Plugins out of...
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

I published the associated CR. Thanks for the commit!

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
commented

This change sounds disruptive from the CR, but the patch uses deprecation so I think it's not. I'll clarify the CR to explain the old ones are deprecated.