Fix sub-optimal DX in MigrateFieldInterface

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.

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.

Unblocked.

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.

Rename patch was reverted.

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

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

Added tasks to wait until #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins gets fixed

No longer blocked.

What about proccessFieldWidget() and processFieldFormatter()?

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

I liked defineValueProcessPipline from the IS better.

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.

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.

Updated IS.

Renamed instances of:

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

The BC layer still needs tests.

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

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.

Updated the IS.

Update the remaining tasks.

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

Re-roll.

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

I am working on this as part of SprintWeekend2018.

here's the interdiff

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

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

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

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.

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

Alright, should be all set now.

If someone could also review the change record, that would be great #2944598: Update method names to be more meaningful in MigrateFieldInterface

Looks like this is just updating the tests to account for the link to the change record. Or am I wrong? Tagging novice for that. And removing the CR tag as we already seem to have a CR.

Unassigning since this seems like anyone could pick up the novice tasks.

@heddn, I can fix the failed errors if someone will weigh in on the syntax change of the error message. I believe when I picked this up the error message was :

Deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x. Use defineValueProcessPipeline() instead.

per the example on https://www.drupal.org/core/deprecation, I saw that the versions ended in .0 so I went ahead and updated that without being 100% clear if that was the right step forward. Here is an example of what an error message looks like now (with the change record link):

Deprecated in Drupal 8.6.0, to be removed before Drupal 9.0.0. Use defineValueProcessPipeline() instead. See https://www.drupal.org/node/2944598.

See how the versions changed from 8.5.x to 8.6.0 and 9.0.x to 9.0.0, is that OK?

Changing the version is correct. Because 8.5.0 was already released. And API changes to interfaces should occur in minor releases. Not point releases within a minor.

I removed the changes to file d7/ImageField.php because issue 2934145 changed this file and it looks like the change in this ticket is no longer needed:

+++ b/core/modules/file/src/Plugin/migrate/field/d7/ImageField.php
@@ -18,7 +18,7 @@ class ImageField extends FieldPluginBase {
   /**
    * {@inheritdoc}
    */
-  public function processFieldValues(MigrationInterface $migration, $field_name, $data) {
+  public function defineValueProcessPipeline(MigrationInterface $migration, $field_name, $data) {
     $process = [
       'plugin' => 'sub_process',
       'source' => $field_name,

This should fix the file.file and text.text failures. I ended up removing the changes introduced by this patch to ImageFieldTest.php because of the changes in issue 2934145

+++ b/core/modules/file/tests/src/Unit/Plugin/migrate/field/d7/ImageFieldLegacyTest.php
--- a/core/modules/file/tests/src/Unit/Plugin/migrate/field/d7/ImageFieldTest.php
+++ b/core/modules/file/tests/src/Unit/Plugin/migrate/field/d7/ImageFieldTest.php

+++ b/core/modules/file/tests/src/Unit/Plugin/migrate/field/d7/ImageFieldTest.php
@@ -31,7 +31,7 @@ protected function setUp() {
 
     $migration = $this->prophesize(MigrationInterface::class);
 
-    // The plugin's processFieldValues() method will call
+    // The plugin's defineValueProcessPipeline() method will call
     // mergeProcessOfProperty() and return nothing. So, in order to examine the
     // process pipeline created by the plugin, we need to ensure that
     // getProcess() always returns the last input to mergeProcessOfProperty().
@@ -43,10 +43,10 @@ protected function setUp() {

@@ -43,10 +43,10 @@ protected function setUp() {
   }
 
   /**
-   * @covers ::processFieldValues
+   * @covers ::defineValueProcessPipeline
    */
-  public function testProcessFieldValues() {
-    $this->plugin->processFieldValues($this->migration, 'somefieldname', []);
+  public function testDefineValueProcessPipeline($method = 'defineValueProcessPipeline') {
+    $this->plugin->$method($this->migration, 'somefieldname', []);

I'm worried that this is beginning to wander away from the point. Here's my take on it, based on the patch in #63, but also with an interdiff against #74.

Add @expectedDeprecation to fix final test failure.

Nearly there...

@Jo Fitzgerald thanks for picking this up, I too was concerned that I was removing too much. The multiple interdiffs really helped me review the changes you made vs my changes.

If possible, can you summarize in laymen terms what your patches do? I hit a lot of problems trying to wrap my head around the changes to d7/ImageFieldTest.php due to the other issue and how I should implement this patch on top of it. Can you speak to how you knew what to do here?

+++ b/core/modules/file/tests/src/Unit/Plugin/migrate/field/d7/ImageFieldTest.php
@@ -44,11 +44,11 @@ protected function setUp() {
-  public function testProcessFieldValues() {
-    $this->plugin->processFieldValues($this->migration, 'somefieldname', []);
+  public function testDefineValueProcessPipeline($method = 'defineValueProcessPipeline') {
+    $this->plugin->$method($this->migration, 'somefieldname', []);

+++ b/core/modules/datetime/tests/src/Unit/Plugin/migrate/field/d6/DateFieldTest.php
@@ -13,24 +14,28 @@
+   * @expectedDeprecation DateField is deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.x. Use \Drupal\datetime\Plugin\migrate\field\DateField instead.
    */
-  public function testUnknownDateType() {
...
     $this->setExpectedException(MigrateException::class, "Field field_date of type 'timestamp' is an unknown date field type.");

This seems strange. All the rest of the deprecations are about 8.6. What's this about?

I also reviewed the CR. It looks pretty nice. Almost there, only the question here before RTBC.

@heddn I'm not convinced it is relevant to this patch, it covers the @trigger_error() in Drupal\datetime\Plugin\migrate\field\d6\DateField. Shall I remove it?

Remove it, but if we need that test coverage, we should open a follow-up.

Removed out of scope test. Added follow-up to add the removed test: #2960056: Add deprecation test to d6/DateField

That was my only concern. So onto RTBC. We've got a CR, we've got docs and tests and deprecation notices and @see links to the CR.

#87 failed to apply on 8.6.x branch, so i have rerolled the patch.

Diff stats are the same. Back to RTBC.

Discussed in the migrate maintainers meeting today with alexpott, phenaproxima, heddn, maxocub, quieone. In summary, we all think the BC test layer is good. The consensus though it we don't want to do another CCK => field plugin rename here and cause a lot of heartache for folks. On the flip-side, the current method names are so dense and obtuse, there is much less adoption (outside of core fields) than we'd like to see. Contrib isn't using field plugins that much. And custom migrations have even less adoption. By renaming, we can actually provide some good tutorials and docs that are intuitive. And hopefully make it much easier for folks to use these things.

Still leaving in RTBC while we think about it.

Re-roll.

Diff stats identical. Back to RTBC.

Working with fields will be less confusing when we can use these descriptive method names. No more having to take a extra time to recall what the 'process' really does, the method name will tell you!

For all the same reasons #99, +1 from me.

1. Make correction to code that was triggering deprecation errors.
2. Fix a couple of coding standards errors.

I was also iffy on removing things from the interface here, but given migrate_drupal isn't stable yet, I think I agree with @alexpott's #60 "However of course things are still implemented by the base classes that everyone would extend so that's good." Just noting this here so it's clear what the decision was if it comes back to bite us.

The only people who could get in trouble would be people calling the classes implementing the interface directly, but in this case we don't expect people to do that, we expect most people to implement it, or just use the classes indirectly via the migrate system. And for people implementing the interfaces, having the deprecated methods isn't helpful at all.

So. Committed 14a769b and pushed to 8.6.x. Thanks!

The tests added here broke phpunit.

There were 2 failures:

1) Drupal\Tests\file\Unit\Plugin\migrate\field\d7\ImageFieldTest::testDefineValueProcessPipeline
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  ImageField is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use \Drupal\image\Plugin\migrate\field\d7\ImageField instead. See https://www.drupal.org/node/2936061.
+

2) Drupal\Tests\datetime\Unit\Plugin\migrate\field\d6\DateFieldTest::testUnknownDateType
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  DateField is deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.x. Use \Drupal\datetime\Plugin\migrate\field\DateField instead.
+

FAILURES!
Tests: 18512, Assertions: 29460, Failures: 2, Skipped: 2273, Incomplete: 1.

Legacy deprecation notices (71)

@neclimdul, I can't reproduce those errors, those tests pass for me locally.

Easy enough to reproduce. Try this:

git clone --branch 8.6.x https://git.drupal.org/project/drupal.git
cd drupal
composer install
composer run-script drupal-phpunit-upgrade # optional depending on php version.
./vendor/bin/phpunit -c core/phpunit.xml.dist --testsuite unit

I cannot reproduce either.

$ vendor/bin/phpunit -c core core/modules/file/tests/src/Unit/Plugin/migrate/field/d7/ImageFieldTest.php
PHPUnit 6.5.9 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\file\Unit\Plugin\migrate\field\d7\ImageFieldTest
.                                                                   1 / 1 (100%)

Time: 1.05 seconds, Memory: 6.00MB

OK (1 test, 2 assertions)


$ vendor/bin/phpunit -c core core/modules/datetime/tests/src/Unit/Plugin/migrate/field/d6/DateFieldTest.php
PHPUnit 6.5.9 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\datetime\Unit\Plugin\migrate\field\d6\DateFieldTest
.                                                                   1 / 1 (100%)

Time: 254 ms, Memory: 6.00MB

OK (1 test, 3 assertions)

try ./vendor/bin/phpunit -c core/phpunit.xml.dist --testsuite unit as noted in #111

Using the steps in #11 I got two failures.

There were 2 failures:

1) Drupal\Tests\file\Unit\Plugin\migrate\field\d7\ImageFieldTest::testDefineValueProcessPipeline
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  ImageField is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use \Drupal\image\Plugin\migrate\field\d7\ImageField instead. See https://www.drupal.org/node/2936061.
+

2) Drupal\Tests\datetime\Unit\Plugin\migrate\field\d6\DateFieldTest::testUnknownDateType
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  DateField is deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.x. Use \Drupal\datetime\Plugin\migrate\field\DateField instead.
+

FAILURES!
Tests: 18528, Assertions: 29808, Failures: 2, Skipped: 2227, Incomplete: 1.

The tests pass when specifying the configuration file, that is, the following resulted in passing tests.

vendor/bin/phpunit -c core/phpunit.xml.dist core/modules/file/tests/src/Unit/Plugin/migrate/field/d7/ImageFieldTest.php
vendor/bin/phpunit -c core/phpunit.xml.dist  core/modules/datetime/tests/src/Unit/Plugin/migrate/field/d6/DateFieldTest.php 

Two questions. One, what is the difference between running the test individually and running a whole testsuit? Two, why didn't testbot report an error?

Sure, good questions. Because of the legacy of Drupal 5 and 6 stupidnessstate complexity, testbot/run-tests.sh runs every test in its own process. For unit tests, phpunit runs in a single process because it is orders of magnitude faster and by definition those tests are supposed to be non deterministic/stateful (We change this behavior for kernel and web tests). Running tests individually obviously runs only that test and so in its own process.

Ideally we shouldn't test auto-loading because load order isn't explicit and is a sort of state we can't control. In reality our deprecation procedure needs tests and tests exactly that. When testing code inclusion(autoloading) like this, the tests must run in their own process. How to do that is documented here:
https://phpunit.de/manual/6.5/en/appendixes.annotations.html#appendixes....

assigning to myself

@runInSeparateProcess on works on the method. :(

No interdiff.

Rockin! Thanks

@heddn, @neclimdul in an ideal world the PHPUnit fix would have in a followup. Re-opening issues for this is not great bbecause it just adds noise and work for people to figure out what's going on.

Committed and pushed e41655d1a3 to 8.7.x and 361b3188b7 to 8.6.x. Thanks!

> I was also iffy on removing things from the interface here, but given migrate_drupal isn't stable yet, I think I agree with @alexpott's #60 "However of course things are still implemented by the base classes that everyone would extend so that's good." Just noting this here so it's clear what the decision was if it comes back to bite us.

The stable/not stable with migrate and migrate_drupal is a pretty thin line, I actually didn't know that only migrate.module is stable :)

I don't think the argument makes quite sense about the base class and things being good. This broke paragraphs, and to be able to be compatible with 8.5 and 8.6 requires us to not rename but duplicate the methods and have both around (at least that's what we will try now, no patch uploaded yet).

Because if you don't implement both, then your logic will not be called in either 8.5 or 8.6. And we would probably not have even noticed if we wouldn't have test for our migrations.

See #2985549: Fix incorrect view display settings in LibraryItem (has a bunch of other test fixes too, but those at least are test-only).

Not quite sure that fixing "sub-optimal DX" (per issue title) is worth this API change?

Due to the poorly previous names of functions, the adoption of field plugins has been very slow and very confusing. And vastly less adoption for that very reason. Field plugins have unfortunately seen a lot of iteration and lots of BC duck-tape holding it together. I'm sorry that paragraphs got effected.

Yeah, I get that, but this is punishing those that *did* implement them now :)

I think the BC layer goes into the wrong direction here, I don't think anything cares about code *calling* those methods, why would anything except migrate do that? What it should care about is existing implementations that override the old methods that are no longer called, either by having an actual BC layer for that (but that's really hard without causing loops or something) or we could just be honest about this being a hard API break and do something like if method_exists($this, 'oldMethod') throw new \Exception('Rename X to Y!').

Then instead of silently no longer being called, invoking the non-updated plugins would actually fail hard with clear instructions on how to update it.

Is there anyway to fix offer BC this was supposed to go in with a full BC layer. migrate_drupal has been beta stability which means we should only break API when we have to. This clearly is not a case of that so we invested time and effort in a BC layer. If that is not working then that is a problem.

We should probably either roll this back or open up a follow-up, but since the discussion is currently happening here, re-opening.

IMHO, one compromise that would not involve crazy reflection-based call chains is to simply implement a constructor in FieldPluginBase which detects if the concrete class is implementing the old methods, and throw deprecation notices if so. That would at least make the problem, and its change record, more visible.

I really like this option. The number of custom folks who have written their own field plugins is very likely low. I'm not aware of any who's done that. Perhaps Berdir or others can correct me, but only contrib (and even there, that was few) were able to reverse engineer the esoteric field plugins.

I'm fine with closing this again.

Deprecation messages are really tricky to get right here as this shows. Lets make sure we open issues in the known contrib modules to update this and move on.

The problem with the suggestion of looking for old methods is actually problematic because it's fine to implement both, which is what we are doing in the paragraphs patch that I'm committing now. So we would need to specifically look for plugins that implement *only* the old method of each pair.

Per #138, marking fixed again.

Published https://www.drupal.org/node/2944598 since this was committed in #125.