Migration for email fields is missing

Postponed on #2447727: Add base class for migrating reference fields.

I applied the patch 2447727-164_0 and this one to the latest 8.1.5 and also tried with the latest dev version...
Initiating the migration, I get the following PHP fatal error:

Declaration of

Drupal\Core\Field\Plugin\migrate\cckfield\Email::ProcessCckFieldValues()

must be compatible with

Drupal\migrate_drupal\Plugin\MigrateCckFieldInterface::ProcessCckFieldValues(Drupal\migrate\Plugin\MigrationInterface $migration, $field_name, $data)

in

/usr/local/www/drupal81/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/Email.php on line 65"

Beeing more a drupal user, than a developer or PHP expert, I´m a little bit lost, where and how to fix this...
What I find in the Email.php is:

public function processCckFieldValues(MigrationInterface $migration, $field_name, $data) {...

What I find in the MigrateCckFieldInterface.php is:

public function processCckFieldValues(MigrationInterface $migration, $field_name, $data);

When I got the PHP fatal error right, those should be "compatible"... and at least for me they look somehow compatible... ;-)
Applying only the patch 2447727-164_0, the migration works, so it´s pretty obvious, that the issue is caused by adding this email field patch.

Any ideas?

The current patch here is based on 8.0.x (Migrations as entities), needs to be rerolled for 8.1.x (Migrations as plugins).

Is the email field the same for both D6 and D7?

The *issue* does not depend on the node/user reference issue, the specific patch assumes changes in that patch. Unblocking this can proceed.

I have re rolled against 8.2.x

One of the changes i have done use Drupal\migrate\Entity\MigrationInterface; TO use Drupal\migrate\Plugin\MigrationInterface;

1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/Email.php
   @@ -0,0 +1,65 @@
   +    $debug = 1;
   

   Is this necessary?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/Email.php
   @@ -0,0 +1,65 @@
   \ No newline at end of file
   

   Nit.

And still needs tests.

Just a quick one addressing #15, although we still need tests.

Simple interdiff but just in case...

Postponed on #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins to avoid rerolling. And still needs tests.

cckfield patch is in!

Converted from "cckfield" to "field".

#23 looks great, thanks Jo!

Setting to needs work for tests.

8.3 is release so changing version to 8.4.

Add tests and added a comma in the plugin annotation.

+++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
@@ -3292,7 +3292,7 @@
+  'field_test_email_email' => 'PrincessRuwenne@castle.com',

Just a nit. I think we are supposed to use @example.com for the domain name. I seem to recall reading that in some code guidance at some point. Well, it doesn't call it out specifically for email addresses, but there is https://www.drupal.org/docs/develop/standards/coding-standards#example. Which, with the spirit of the guidance would make me think that @example.com is preferred.

Changed email address as suggested.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/field/Email.php
   @@ -0,0 +1,61 @@
   +  /**
   +   * @inheritDoc
   +   */
   

   Nit: These all need to be {@inheritdoc}.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/field/Email.php
   @@ -0,0 +1,61 @@
   +      'process' => [
   +          'value' => [
   +            [
   +              'plugin' => 'skip_on_empty',
   +              'method' => 'process',
   +              'source' => 'email',
   +            ],
   +          ],
   +        ]
   

   I don't understand why we'd skip process on empty here. An empty value is a valid value for an email field, so why is skipping needed? At the very least, we should have a comment explaining this logic.

Comment #29

1. Done
2. In Content Types(nodes), there is a chance of the type Email fields are emapty (if its not a required one)

If it is empty, we can migrate that without the skip, no? Moving back to NW for a comment or removal.

Agreed with @heddn. It is OK for us to migrate empty values in the field. Skipping is unnecessary.

@heddn and @phenaproxima, So lets get remove the

+              // if the email fields are not required one in content type.
+              'plugin' => 'skip_on_empty',

?

A little more than that. It will probably look like this (direct mapping):

$process = [
  'plugin' => 'iterator',
  'source' => $field_name,
  'process' => [
    'value' => 'value',
  ],
];

Ok, Lets get that done.

Just needed to rename value to email. Interdiff mysteries continue - it show changes to drupal6.php when I didn't touch the file.

Cool. I see nothing worrisome here!

This made me learn about https://en.wikipedia.org/wiki/The_Paper_Bag_Princess which sounds way cool. Plus one for cool test data ;)

FILE: core/lib/Drupal/Core/Field/Plugin/migrate/field/Email.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 55 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it
----------------------------------------------------------------------

:)

Cool. Back to RTBC.

Correction.
The princess in The Paper Bag Princess is Princess Elizabeth.

I took the name Princess Ruwenne from the Wikipedia page for The Paper Bag Princess because I was too lazy to go find my copy of the book. The name seemed wrong, but I just couldn't remember her name, so I used what was on Wikipedia. Today, I got out my copy of the book and confirmed that her name is Princess Elizabeth. I'm not interested in changing the test code (maybe I'll use the name for the d8_dump) but the Wikipedia page is now correct.

Finally, bonus points to Gábor Hojtsy for searching for Princess Ruwenne. His comment got me thinking about the story and the name, leading me to make these corrections.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x, thanks!

This broke 8.3.x

Let's see what its complaining about on 8.3.x. Here's the same patch from #42 again. I'm optimistically tagging as novice; this is probably something fairly trivial to fix.

Doesn't seem super obvious.

1) Drupal\Tests\node\Kernel\Migrate\d7\MigrateNodeTest::testNode
Failed asserting that null is identical to 'default@example.com'.

/var/www/html/core/modules/node/tests/src/Kernel/Migrate/d7/MigrateNodeTest.php:154

1) Drupal\Tests\node\Kernel\Migrate\d6\MigrateNodeTest::testNode
Failed asserting that null is identical to 'PrincessRuwenne@example.com'.

/var/www/html/core/modules/node/tests/src/Kernel/Migrate/d6/MigrateNodeTest.php:85

+++ b/core/lib/Drupal/Core/Field/Plugin/migrate/field/Email.php
@@ -0,0 +1,54 @@
+use Drupal\migrate_drupal\Plugin\migrate\field\FieldPluginBase;
...
+class Email extends FieldPluginBase {

The email plugin extends FieldPluginBase which doesn't exist in 8.3.x. The issue that creates the base class is #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins which has been committed to 8.4.x and marked as Patch (to be ported) to 8.3.x.

Seems reasonable to postpone this on #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins, which is already tagged as a blocker.

Rerunning now that #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins got merged to 8.3.x.

Everything seems legit here. Back to RTBC for 8.3.x.

Merged to 8.3.x now that it was proven it passes there with its dependency on #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins landed. Thanks all!