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
  • Add BC layer

Remaining tasks

Review
Commit

User interface changes

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

Data model changes

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.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Issue summary: View changes
benjy’s picture

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.

quietone’s picture

mikeryan’s picture

mikeryan’s picture

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.

mikeryan’s picture

Status: Postponed » Active

Unblocked.

phenaproxima’s picture

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.

mikeryan’s picture

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.

phenaproxima’s picture

Status: Postponed » Active

Unblocked!

mikeryan’s picture

Status: Active » Postponed

Rename patch was reverted.

heddn’s picture

Issue tags: +Migrate BC break
heddn’s picture

Issue tags: -Migrate BC break

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

iMiksu’s picture

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 for updating automatic tests (right?)
  • Added task Provide BC layer, if doesn't land to 8.2.x (right?)

What needs more information

  • 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 :)

iMiksu’s picture

iMiksu’s picture

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.

Jo Fitzgerald’s picture

Status: Postponed » Active

No longer blocked.

quietone’s picture

Issue summary: View changes

What about proccessFieldWidget() and processFieldFormatter()?

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.

Jo Fitzgerald’s picture

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).
heddn’s picture

Title: Fix sub-optimal DX in MigrateCckFieldInterface » Fix sub-optimal DX in MigrateFieldInterface

I liked defineValueProcessPipline from the IS better.

Jo Fitzgerald’s picture

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

quietone’s picture

Yea, I think this is the right track.

What about proccessFieldWidget() and processFieldFormatter()?

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.

phenaproxima’s picture

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()?

Jo Fitzgerald’s picture

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()
heddn’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

The BC layer still needs tests.

Jo Fitzgerald’s picture

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?

quietone’s picture

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

heddn’s picture

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

Jo Fitzgerald’s picture

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

quietone’s picture

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

Updated the IS.

quietone’s picture

Issue summary: View changes

Update the remaining tasks.

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.

heddn’s picture

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.

Jo Fitzgerald’s picture

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

heddn’s picture

Issue tags: +Novice

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

robpowell’s picture

I am working on this as part of SprintWeekend2018.

phenaproxima’s picture

Issue tags: +SprintWeekend2018
robpowell’s picture

Issue tags: +SprintWeekendBOS
robpowell’s picture

robpowell’s picture

FileSize
4.11 KB

here's the interdiff

robpowell’s picture

Status: Needs work » Needs review
phenaproxima’s picture

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.

robpowell’s picture

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().

robpowell’s picture

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

robpowell’s picture

Status: Needs work » Needs review
phenaproxima’s picture

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

phenaproxima’s picture

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

Let's try that again.

phenaproxima’s picture

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

robpowell’s picture

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

robpowell’s picture

Status: Needs work » Needs review
heddn’s picture

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.

alexpott’s picture

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?

robpowell’s picture

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
robpowell’s picture

Assigned: heddn » robpowell
robpowell’s picture

Alright, should be all set now.

robpowell’s picture

Status: Needs work » Needs review
robpowell’s picture

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