Add support for migrate d7 telephone field

Attempt to fix tests by including the telephone>telephone migration in migration tests.

Re-roll of patch #7 against 8.7.x in hopes it will apply cleanly.

For anyone who gets here as puzzled as I, in the "extra" section of your composer file you must add:

        "patchLevel": {
            "drupal/core": "-p2"
        },

To force the patch to be applied to the right place. Sorry for the confusion.

Nice work! I have been working on this, as well. I guess at this point this needs to go into 9.1.x, so first of all I rerolled the patch.

Secondly, here's a review of the patch:

1. +++ b/core/modules/telephone/src/Plugin/migrate/field/d7/TelephoneField.php
   @@ -0,0 +1,29 @@
   + *   type_map = {
   + *     "telephone" = "telephone",
   + *   },
   

   This is not necessary if the field type matches in D7 and D8.

2. +++ b/core/modules/telephone/src/Plugin/migrate/field/d7/TelephoneField.php
   @@ -0,0 +1,29 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getFieldFormatterMap() {
   +    return [
   +      'phone' => 'basic_string',
   +    ];
   +  }
   

   This is leftover from the Phone migration and should be removed.

I fixed those things and also added specific test coverage for the telephone module upgrade. Would love some reviews. If people here can review my test coverage then maybe we can collectively RTBC, because I RTBC the rest (barring the nitpicks above) as I came up with the nearly identical code when testing locally.

+++ b/core/modules/telephone/migrations/state/telephone.migrate_drupal.yml
@@ -1,3 +1,4 @@
+    telephone: telephone

For anyone following along at home: I was previously wondering why the previous patch didn't contain this hunk. I guess something changed in the test coverage in the meantime because this hunk is what makes the rerolled patch pass the tests.

Also fixes the remaining tests.

Re-rolled for 8.8

So the previous patch was missing a formatter mapping. In D7 we have text_plain which maps to string in D8. Note that I found #3130240: Some field plugins declare an invalid default widget and formatter which is confusing (but should not impact this patch directly).

In order to provide some test coverage I configured the text_plain formatter to be used in the teaser view mode. Unfortunately, that doesn't make Upgrade7Test fail, so uploading only that change separately to see if it fails any other test that I missed.

So the test-only patch in #15 nicely failed how it should, so this now includes the test-interdiff from above (in addition to the regular interdiff).

It also fixes the failure in FieldDiscoveryTest (see the new interdiff from this comment) so it should be green.

Not exactly sure why I didn't see this failure earlier, in particular in #15, but this should fix the remaining failure. (Not attaching a separate interdiff for the 8.8.x patch, it's identical, but for the different number of entity view displays.)

Neither the 8.8 nor 9.1 patches apply to 8.9. It is quite tedious to re-roll the whole patch, and does not seem worth doing given the move to Drupal 9. So for anyone who needs the functionality in the short term, this patch contains the functional portions of the master and applies against 8.9.x (8.9.3 as I write).

This is the first time I have seen this. There are tests in MigrateField and MigrateFieldInstance test for telephone fields so this may already be done. It requires some investigation.

This is a task because telephone is in core now and moving to the migration system where all things migrate live.

Rerolled #21 for 9.2.x. Some of the migrate tests fail for me locally, but they also fail without this patch, not sure why - let's see what testbot thinks.

I went ahead and fixed the test but I am still not sure we need this. Will come back to this after the migrate meeting.

@quietone this is still needed, I am migrating a D7 site that uses Telephone fields (as opposed to Phone which is supported already), and without this patch the field instance fails to migrate.

@longwave, ah, thanks. I see now. So, both phone and telephone moved to core?

This needs a reroll.

Re #30: If I remember correctly, the history is as follows: First there was only Phone module for D7. Then that functionality got moved into D8 as the D8 telephone module and with some slightly different IDs for formatters, etc. And then someone backported the core telephone module and created the D7 Telephone module with the identical IDs as the D8 one. So "newer" D7 sites would have probably used D7 Telephone module (vs. Phone) in the hopes of better D8 compatibility (somewhat ironically, given this issue).

I think (though am not sure) that the backport, i.e. the creation of the D7 Telephone module happened after there was already a migration for the Phone module in place, but in any case it is missing. I assume it was just an oversight, because people thought that we do not need a migration because the IDs are the same.

Rerolled.

@tstoeckler, thank you for the history. I've added this to my list to review. Should get to it early next week.

Also adding this to the D7 meta.

Does Wednesday count as early in the week?

Just a few more items and this will be ready.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d7/FieldDiscoveryTest.php
@@ -241,6 +241,9 @@ public function addAllFieldProcessesAltersData() {
+                  'text_plain' => 'string',

Shouldn't 'string' be 'basic_string'. In Telephone, telephone_field_info also defines 'telephone_link' so another line is needed,
'telephone_link' => 'basic_string'

This also needs two additional assertions, one in \d7\MigrateFieldInstanceWidgetSettingsTest, and one in d7/MigrateFieldFormatterSettingsTest.php, both are an assertComponent at the end of the tests for the test_content_type,

Working on this.

Made the changes suggested in #34. Please review. Thanks.

@ayushmishra206, thanks for working on this. Instead of letting drupalci run the checks on the code, you can do it locally. Running core development checks (>= 9.2.x) has all the instructions.

@quietone Thanks for telling me this, I'll keep this in mind moving forward. I have made the changes causing the fail and tested it as well.

@ayushmishra206, thanks. Like the commit-code-check running the tests locally helps everyone. Some of the errors could have been fixed if you ran the tests locally. See https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/running-.... And one of the error would have been picked up if you are using an IDE. I use PhpStorm and it complains loudly (visually) when the number of parameters doesn't match the method signature.

Now, for the reported errors I started with FieldDiscoveryTest. That need to be sorted before the others.

1. I think the problem is that the first part of #34 was missed.

   +++ b/core/modules/telephone/src/Plugin/migrate/field/d7/TelephoneField.php
   @@ -0,0 +1,35 @@
   +      'text_plain' => 'string',
   

       return [
         'text_plain' => 'basic_string',
         'telephone_link' => 'telephone_link',
       ];

   string needs to be basic_string and a line for the telephone link is needed.

2. Now for the migration tests

   +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldFormatterSettingsTest.php
   @@ -178,6 +178,7 @@ public function testMigration() {
   +    $this->assertComponent('node.test_content_type.default', 'field_phone', 'telephone_link', 21);
   

   This needs to be field_telephone. And it is missing the 'label' parameter, which is probably the default, 'above'. I don't know what the weight value should be.
   $this->assertComponent('node.test_content_type.default', 'field_phone', 'basic_string', 'above', ??);

3. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceWidgetSettingsTest.php
   @@ -128,6 +128,7 @@ public function testWidgetSettings() {
   +    $this->assertComponent('node.test_content_type.default', 'field_phone', 'telephone_link', 21);
   

   s/field_phone/field_telephone and s/telephone_link/telephone_default/

This is one of the last two issue in the Meta for D7 migrations and I'm keen to get them committed. Therefor, worked on a reroll.

#41 is missing some of #38, fixed here and also addressed #40.

@longwave, thanks.

I am still not sure about the map in getFieldFormatterMap. I tested on D9 and a telephone can be 'string' or 'telehone_link'

  field_tel_link:
    weight: 112
    label: above
    settings:
      title: ''
    third_party_settings: {  }
    type: telephone_link
    region: content
  field_tel_plain:
    weight: 111
    label: above
    settings:
      link_to_entity: false
    third_party_settings: {  }
    type: string
    region: content

1. +++ b/core/modules/telephone/src/Plugin/migrate/field/d7/TelephoneField.php
   @@ -0,0 +1,36 @@
   +      'text_plain' => 'basic_string',
   

   So 'basic_string' here should be 'string', as it was originally. What confused me then was this in d7/MigrateFieldFormatterSettingsTest

       // Phone formatters are not mapped and should default to basic_string.
       $this->assertComponent('node.test_content_type.default', 'field_phone', 'basic_string', 'above', 2);

   and \Drupal\telephone\Plugin\migrate\field\d7\PhoneField maps to basic_string.

       return [
         'phone' => 'basic_string',
       ];
   

   Based on the testing on D9, field_phone should also be a 'string'. Agree? If so, that will need a followup.

2. +++ b/core/modules/telephone/src/Plugin/migrate/field/d7/TelephoneField.php
   @@ -0,0 +1,36 @@
   +      'telephone_link' => 'basic_string',
   

   Since there really is a telephone_link type so this be 'telephone_link' => 'telephone_link',

I hope that makes sense. And the tests will need updating of course.

I'm making that changes I suggested in #43.

#43.1 yeah that is what confused me too, there is both a basic_string and a string formatter but string is the right one here, I was misled by field_phone as well.

#43.2 yep that makes sense too.

I agree with both changes therefore this is RTBC.

Committed 9c5da7c and pushed to 9.2.x. Thanks!

Kicked off a test against 9.1.x - will backport if it passes. - the patch doesn't apply