Problem/Motivation

Some of Migrate’s cckfield plugin's method naming are not describing clear enough what they do and therefore may confuse developers using the API (therefore giving poor DX).

Proposed resolution

See discussion in #23 - #27

• Rename processFieldValues() to defineValueProcessPipeline()
• Rename processField() to alterFieldMigration()/li>
• Rename processFieldInstance() to alterFieldInstanceMigration()
• Rename processFieldWidget() to alterFieldWidgetMigration()
• Rename processFieldFormatter() to alterFieldFormatterMigration()
• Update tests

Review
Commit

None.

API changes

Yes.

1. processCckFieldValues() will be deprecatedThis wad deprecated in #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins
2. processFieldValues() will be deprecated
3. processField() will be deprecated
4. processFieldInstance() will be deprecated
5. processFieldWidget() will be deprecated
6. processFieldFormatter() will be deprecated

None.

Original report

Those who know me know that I’m fairly fanatical about good DX in core APIs (or, indeed, any APIs). Because of that, certain aspects of Migrate’s cckfield plugins have been bugging me for a long time.

I have three main bones to pick:

1. processField() is poorly named. Its actual purpose is altering the migrations that transform CCK fields (in D6) and Field API field definitions (in D7) into D8 field_storage_config entities. This should be renamed to processFieldMigration() or alterFieldMigration().
2. processFieldInstance() has the same problem. It should be called processFieldInstanceMigration() or alterFieldInstanceMigration().
3. processCckFieldValues() is on another planet entirely. Its purpose is to define the processing pipeline for values of the field type that the plugin handles, but this does not come through at all in the method name. I think this should be called something like defineFieldValueProcess() or defineValueProcessPipline().

The doc blocks for each of these methods in MigrateCckFieldInterface need to be improved as well. These plugins are crucial for contrib migrations from D6 and D7, so the interface should include short snippets of example code in the doc comments.

CommentFileSizeAuthor
#634.64 KBrobpowell
#6359.02 KBrobpowell
#57801 bytesrobpowell
#5758.58 KBrobpowell
#542.66 KBphenaproxima
#5458.72 KBphenaproxima
#5158.28 KBphenaproxima
#4912.01 KBrobpowell
#4950.65 KBrobpowell
#454.11 KBrobpowell
#4450.48 KBrobpowell
#3850.2 KBJo Fitzgerald
#3050.19 KBJo Fitzgerald
#3018.16 KBJo Fitzgerald
#2845.83 KBJo Fitzgerald
#2833.62 KBJo Fitzgerald
#2513.12 KBJo Fitzgerald
#2515.3 KBJo Fitzgerald
#2313 KBJo Fitzgerald

phenaproxima created an issue. See original summary.

 Issue summary: View changes

Yeah I think we can do better here as well, I think the process in processField initially came about because we are updating the "process" array for the field.

Anyway, I like the alter* method suggestions in the summary, I think alter is a better word since we're altering the migration not the actual data.

 Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

 Issue tags: +neworleans2016, +Migrate BC break
 Status: Active » Postponed

As with #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins, renaming stuff would disrupt the patch for #2631736: Cckfield Plugins must distinguish core versions, which is both more important and close to RTBC - let's postpone considering this until that is in.

 Status: Postponed » Active

Unblocked.

 Status: Active » Postponed
 Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

 Issue tags: -Migrate BC break

If I'm not mistaken, these would all be simple renames and easily covered by a BC layer passing through to the new names, so not really a BC break.

 Status: Postponed » Active

Unblocked!

 Status: Active » Postponed

Rename patch was reverted.

 Issue tags: +Migrate BC break
 Issue tags: -Migrate BC break

This can be done with a BC shim layer, so removing the BC break tag.

 Issue summary: View changes

Changes:

• Updated summary
• Given that benjy in #3 liked the alter* versions more, I've picked them as clear tasks
• The third method name is still up to decide or are open to other suggestions is the final task before
• Added task Provide BC layer, if doesn't land to 8.2.x (right?)

• I would like to know how we wont break BC if it doesn't land to 8.2.x?

EDIT: Whoops completely forgot 8.2 was released, lol :)

 Issue summary: View changes
 Issue summary: View changes
 Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

 Status: Postponed » Active

No longer blocked.

 Issue summary: View changes

 Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

 Status: Active » Needs review
FileSize
13 KB

WIP. Before I go too far can someone confirm that I am doing the right thing, please?

So far I have:

• replaced processFieldValues() with defineFieldValueProcess()
• set processFieldValues() as deprecated in all classes that used it (but not the interface).
 Title: Fix sub-optimal DX in MigrateCckFieldInterface » Fix sub-optimal DX in MigrateFieldInterface

I liked defineValueProcessPipline from the IS better.

FileSize
15.3 KB
13.12 KB

Changed defineFieldValueProcess() to defineValueProcessPipeline(). Otherwise am I on the right track? Shall I do similar for processField() and processFieldInstance()? What about proccessFieldWidget() and processFieldFormatter()?

Yea, I think this is the right track.

I too was wondering about that. Both are doing a $migration->mergeProcessOfProperty('options/type',$process); so they seem like a good candidate for renaming, by replacing process with alter. It makes sense to me, at least.

 Status: Needs review » Needs work

...they seem like a good candidate for renaming, by replacing process with alter

I agree. Let's do this. How about names like alterFieldInstanceMigration()?

 Issue summary: View changes Status: Needs work » Needs review
FileSize
33.62 KB
45.83 KB

Updated IS.

Renamed instances of:

• processField()
• processFieldInstance()
• processFieldWidget()
• processFieldFormatter()
 Status: Needs review » Needs work Issue tags: +Needs tests

The BC layer still needs tests.

 Status: Needs work » Needs review
FileSize
18.16 KB
50.19 KB

Retained previous tests for BC.

While making the changes I noticed that:
modules/text/tests/src/Unit/Plugin/migrate/field/d6/TextFieldTest.php
and
modules/text/tests/src/Unit/Migrate/d6/TextFieldTest.php
are almost identical. Is there any need for both? I realise that is not within the scope of this issue, but is it worth creating a new issue to remove one or the other?

The issue of the 'duplicate' test files d6/TextFieldTest.php seems to apply to the d7 versions as well. And it looks like I separated the test into two versions in #2833206: Convert Migrate's cckfield plugins to use the new field plugins in #70

(8.5.x)$diff Migrate/d6/TextFieldTest.php Plugin/migrate/field/d6/TextFieldTest.php 3c3 < namespace Drupal\Tests\text\Unit\Migrate\d6; <code>--- > namespace Drupal\Tests\text\Unit\Plugin\migrate\field\d6; 14d13 < * @group legacy v@cm {/opt/sites/d8/core/modules/text/tests/src/Unit} (8.5.x)$ diff Migrate/d7/TextFieldTest.php Plugin/migrate/field/d7/TextFieldTest.php
3c3
< namespace Drupal\Tests\text\Unit\Migrate\d7;
---
> namespace Drupal\Tests\text\Unit\Plugin\migrate\field\d7;
5c5
< use Drupal\Tests\text\Unit\Migrate\d6\TextFieldTest as D6TextFieldTest;
---
> use Drupal\Tests\text\Unit\Plugin\migrate\field\d6\TextFieldTest as D6TextFieldTest;
10d9
<  * @group legacy

Seems like a separate issue to me, but it is late and I should be asleep...

 Issue tags: -Needs tests +Needs issue summary update

We could also update the IS with the proposed renames? That would make this easier to review.

@heddn the proposed renames are in the "Remaining tasks" section.

 Issue summary: View changes Issue tags: -Needs issue summary update

Updated the IS.

 Issue summary: View changes

 Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

 Assigned: Unassigned » heddn Status: Needs review » Needs work Issue tags: +Needs reroll

I think this is pretty close. But let's re-roll it again first.

 Status: Needs work » Needs review Issue tags: -Needs reroll
FileSize
50.2 KB

Re-roll.

 Status: Needs review » Needs work

The last submitted patch, 38: 2631698-38.patch, failed testing. View results

 Issue tags: +Novice

Looks like a rename of iterator => sub_process is in order here. Tagging Novice.

I am working on this as part of SprintWeekend2018.

 Issue tags: +SprintWeekend2018
 Issue tags: +SprintWeekendBOS
FileSize
50.48 KB
FileSize
4.11 KB

here's the interdiff

 Status: Needs work » Needs review
 Status: Needs review » Needs work

Looks great. Thank you, @robpowell!

Unfortunately, I think this still needs some work, mostly to bring it in line with the core deprecation policy.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/field/Email.php
@@ -40,9 +40,17 @@ public function getFieldFormatterMap() {
public function processFieldValues(MigrationInterface $migration,$field_name, $data) { + return$this->defineValueProcessPipeline($migration,$field_name, $data); + }  As per the deprecation policy, let's always add a @trigger_error() call before calling defineValueProcessPipeline() from processFieldValues(). 2. +++ b/core/modules/datetime/tests/src/Unit/Plugin/migrate/field/DateFieldTest.php @@ -29,6 +29,10 @@ public function testUnknownDateType() { +$this->plugin->defineValueProcessPipeline($this->migration, 'field_date', ['type' => 'timestamp']); + + // Retain backwards compatibility testing. +$this->setExpectedException(MigrateException::class, "Field field_date of type 'timestamp' is an unknown date field type.");


This won't work, because the test method will exit as soon as defineValueProcessPipeline() throws its expected exception. I think we will have to copy this entire test method, and in the copy we'll call processFieldValues(). Those cloned methods should also mention in their doc comment that they are testing backwards compatibility and should be removed.

3. +++ b/core/modules/link/src/Plugin/migrate/cckfield/d7/LinkField.php
@@ -38,9 +38,14 @@ public function getFieldWidgetMap() {
public function processFieldInstance(MigrationInterface $migration) { +$this->alterFieldInstanceMigration(\$migration);
+  }


We also need to do the proper @trigger_error() thing here too.

Spoke to @phenaproxima, processFieldValues() is being inherited by FieldPluginBase.php. We can make the change recommended in #47.1 in FieldPluginBase::processFieldValues() and remove the child class declarations of processFieldValues().

FileSize
50.65 KB
12.01 KB

Went ahead and followed the same cleanup for four depracated methods, here's the total list:

• processField()
• processFieldInstance()
• processFieldWidget()
• processFieldFormatter()
• processFieldValues()

#47.2 This was the case in field/fieldDateTest.php and d6/fieldDateTest.php. To complete the requirements of the deprecation policy,

A unit test proving the deprecation notice will be triggered when the deprecated code is called.

#47.3 done

 Status: Needs work » Needs review
FileSize
58.28 KB

I've taken the fine work @robpowell did and tweaked it a bit.

The main thing here is that, rather than repeat entire test methods just to call a single deprecated method, I've created separate test classes to exercise the deprecated methods and assert that the proper deprecation errors are raised.

No interdiff because the changes are complex enough that it will be easier to parse this cleanly.

The last submitted patch, 49: 2631698-49.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

 Status: Needs review » Needs work

The last submitted patch, 51: 2631698-51.patch, failed testing. View results

 Status: Needs work » Needs review
FileSize
58.72 KB
2.66 KB

Let's try that again.

 Issue tags: -Novice

Also removing the Novice tag, because I no longer think it applies.

 Status: Needs review » Needs work

The last submitted patch, 54: 2631698-54.patch, failed testing. View results

FileSize
58.58 KB
801 bytes
+++ b/core/modules/taxonomy/tests/src/Unit/Plugin/migrate/cckfield/TaxonomyTermReferenceCckTest.php
@@ -45,10 +45,17 @@ protected function setUp() {
+   * @expectedDeprecation TaxonomyTermReference is deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.x. Use \Drupal\taxonomy\Plugin\migrate\field\TaxonomyTermReference instead.


processCckFieldValues does not return expectedDepraction removing it passes tests locally.

 Status: Needs work » Needs review
 Status: Needs review » Reviewed & tested by the community

Lots of changes are in place. Tests are passing. FieldPluginBase has deprecation triggers in place, so we can be pretty sure we caught all re-names. If we didn't, then we can grab them in follow-ups. I'm good with RTBC, as I don't see any issues in here.

 Status: Reviewed & tested by the community » Needs work Issue tags: +Needs change record

We need a change record here and all the deprecation notices should be pointing to it as per https://www.drupal.org/core/deprecation

I'm not 100% convinced we should be removing stuff from the interface. However of course things are still implemented by the base classes that everyone would extend so that's good. Love the deprecation work and testing - really good to have all the testing still in place. However I think that there might be a problem. For example, Example module has a migrate field plugin that implements \Drupal\migrate_drupal\Plugin\MigrateFieldInterface::processFieldValues how would it ever be called by node derivers?

I'll take care of this tonight

#60 .1

We need a change record here and all the deprecation notices should be pointing to it as per https://www.drupal.org/core/deprecation
 Assigned: heddn » robpowell
FileSize
59.02 KB
4.64 KB

Alright, should be all set now.

 Status: Needs work » Needs review

If someone could also review the change record, that would be great [#2944598]

 Status: Needs review » Needs work

The last submitted patch, 63: 2631698-63.patch, failed testing. View results