Migrate Drupal 7 Field/Instance/View mode settings

Added a test of the d7_field migration.

Minor test cleanup.

Blocked by #2495755: Create MigrateDrupal7TestBase.

Merged #2495303: Migration for Drupal 7 field instances.

Merged #2466785: Migration Files for Drupal 7 Field Instance Widget Settings and added unit tests of the three source plugins.

This is a blocker to node migrations. Escalating to a Migrate critical.

Re-rolled and cleaned up. All I really did was move files around, so the interdiff is more confusing than revealing and I decided to omit it.

Boy...there was a bunch of minor stuff wrong with the tests :) Fixed now.

Go again?

...and again?

For laughs, since I generated it anyway, here are the diffs against the D6 version. Too much to really review though...

1. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceDefaults.php
   @@ -0,0 +1,42 @@
   +        // @todo Add here special conversion, if case
   

   D6 had a whole bunch of special cases here - will we be identifying D7's special cases in a followup issue? If so, we should create that issue and link it here.

2. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
   @@ -0,0 +1,55 @@
   +    // Remove 'user_register_form'.
   +    // @see https://drupal.org/node/2049485'
   +    unset($settings['user_register_form']);
   

   The referenced issue #2049485: Remove traces of the 'user_register_form' field setting was fixed 2 years ago :O. Something to remove from the D6 side as well...

3. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldSettings.php
   @@ -0,0 +1,38 @@
   +    if ($row->getSourceProperty('type') == 'image' && !is_array($value['default_image'])) {
   +      $value['default_image'] = array(
   +        'uuid' => '',
   +      );
   +    }
   

   A whole plugin just for this? Or are we anticipating more special cases being identified (D6 has a bunch of stuff here)?

4. +++ b/core/modules/field/src/Plugin/migrate/source/d7/Field.php
   @@ -0,0 +1,81 @@
   + * This source class doesn't really work for anything but the Drupal 8
   + * migration because it returns only active fields, one per entity type and
   + * field name.
   

   I don't really follow, what else might it have worked for?

5. +++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
   @@ -0,0 +1,96 @@
   +  public function prepareRow(Row $row, $keep = TRUE) {
   

   Ummm, what is $keep? It's not part of the prepareRow() API in general, and unused here... And now I notice it's in Field.php above as well, also unused.

1. I believe that is exactly the intention. Agreed that we should have a follow-up issue.
2. Fixed.
3. I think we're anticipating more things happening here.
4. Don't know what was up with that, so I removed it.
5. Removed.

Significantly beefed up the test of d7_field_instance_widget_settings.

Added a couple of unit tests.

Added migrations for view modes and formatter settings, with TONS of test assertions. The patch is pretty daunting to review, but it's reasonably thorough. There will definitely be a lot of rough edges, and certain aspects of fields (mostly type-specific settings) will not migrate without writing additional plugins, but it's a start. I figure it's OK to commit this now in order to unblock the Big Four migrations (nodes, users, comments, and taxonomy terms), and migrate more granular field settings later.

Nits:

+    source:
+      - type

source: type would work just as well. (it gets converted into the same array)
+ hidden: ~
heh. the Symfony parser , as far as I am aware does not recognize this -- please check but it doesn't. Try and add a test for hidden: 0. That should skip the row.

    return $this->initializeIterator()->count();

trait? no pressure if not, it's just a 1 loc method. but perhaps other (d6?) uses too.

Overall looks very impressive.

1. +++ b/core/modules/field/migration_templates/d7_field_formatter_settings.yml
   @@ -0,0 +1,70 @@
   +        taxonomy_term_reference_link: entity_reference_label
   

   entity_reference_label? That wasn't in core D7?

2. +++ b/core/modules/field/migration_templates/d7_field_instance_widget_settings.yml
   @@ -0,0 +1,58 @@
   +    -
   +      plugin: field_instance_widget_settings
   +      source:
   +        - 'widget/type'
   +        - widget_settings
   

   Is this meant to be a list?

3. +++ b/core/modules/field/migration_templates/d7_field_instance_widget_settings.yml
   --- /dev/null
   +++ b/core/modules/field/migration_templates/d7_view_mode.yml
   

   nit: The d6 version is d6_view_modes with an "s"

4. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceDefaults.php
   @@ -0,0 +1,42 @@
   +        // @todo Add here special conversion, if case
   

   We usually have issues against todo's, is there some processing we know we're missing already? Or maybe this should just be a comment?

   D6 has quite a bit of processing but maybe D7/D8 align better?

5. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
   @@ -0,0 +1,49 @@
   +  /**
   +   * {@inheritdoc}
   +   *
   +   * Set the field instance defaults.
   +   */
   

   Just remove the comment, we don't do inheritdoc and comments together I don't think it works with the api doc generator.

6. +++ b/core/modules/field/src/Tests/Migrate/d7/MigrateFieldFormatterSettingsTest.php
   @@ -0,0 +1,205 @@
   +  protected function assertEntity($id) {
   ...
   +  protected function assertComponent($display_id, $component_id, $type, $label, $weight) {
   

   Missing function docs.

7. +++ b/core/modules/field/src/Tests/Migrate/d7/MigrateFieldInstanceWidgetSettingsTest.php
   @@ -0,0 +1,165 @@
   +  protected function assertComponent($display_id, $component_id, $widget_type, $weight) {
   

   and here

I ran out of time, will review again before commit. Great work so far though looks good and awesome work with all the extra test coverage!

#33:

1. Switched to 0. Tests still pass, so hey: alright!
2. Let's not bother for now; these sorts of sources (where the main action happens in initializeIterator()) should have a trait, yes, but this isn't the place to do it. I'd like to get @mikeryan's input on that too.

#35:

1. I believe it's in D7 too, but taxonomy term reference fields were their own thing. They need to be mapped to entity reference fields in D8, if I'm not mistaken.
2. Fixed.
3. Fixed.
4. D7 and D8 do align better, but additional processing will certainly be required and I think it's OK to do that piecemeal, in follow-up issues, rather than letting this patch turn into even more of a kaiju than it already is.
5. Fixed.
6. Fixed.
7. Fixed.

Could this be the reason for the failure?

1. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceDefaults.php
   @@ -0,0 +1,41 @@
   +class FieldInstanceDefaults extends ProcessPluginBase {
   ...
   +  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
   

   My only concern would be that we don't have any processing here, I'd like to see a follow-up to discuss that.

2. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
   @@ -0,0 +1,47 @@
   +        $settings = $instance_settings;
   +        $settings['default_image'] = array(
   +          'alt' => '',
   +          'title' => '',
   +          'width' => NULL,
   +          'height' => NULL,
   +          'uuid' => '',
   +        );
   

   It's not obvious why this is needed, can't the incoming instance settings have a default image?

3. +++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldSettings.php
   @@ -0,0 +1,36 @@
   +    if ($row->getSourceProperty('type') == 'image' && !is_array($value['default_image'])) {
   

   This looks better in that it has a check on default_image.

4. +++ b/core/modules/field/src/Plugin/migrate/source/d7/ViewMode.php
   @@ -0,0 +1,78 @@
   +    foreach ($result as $field_instance) {
   +      $data = unserialize($field_instance['data']);
   +      foreach (array_keys($data['display']) as $view_mode) {
   +        $key = $field_instance['entity_type'] . '.' . $view_mode;
   

   Should this check if the field is excluded similar to how the D6 source does? Also i'll not a current bug here in the D6 source #2498291: Fields hidden in some view modes abort migration (D6)

5. +++ b/core/modules/migrate_drupal/config/schema/migrate_drupal.source.schema.yml
   @@ -144,6 +144,9 @@ migrate_entity_constant:
   diff --git a/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
   
   diff --git a/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
   index aa45439..03cb043 100644
   
   index aa45439..03cb043 100644
   --- a/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
   
   --- a/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
   +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
   
   +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
   +++ b/core/modules/migrate_drupal/src/Plugin/migrate/builder/d6/CckBuilder.php
   @@ -7,6 +7,7 @@
   
   @@ -7,6 +7,7 @@
    
    namespace Drupal\migrate_drupal\Plugin\migrate\builder\d6;
    
   +use Drupal\Core\Database\Connection;
    use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
    use Drupal\migrate\Plugin\migrate\builder\BuilderBase;
    use Drupal\migrate\Plugin\MigratePluginManager;
   diff --git a/core/modules/node/migration_templates/d6_view_modes.yml b/core/modules/node/migration_templates/d6_view_modes.yml
   

   None of these are related to this patch?

Other than the above I think this is ready to go in. I'm heading to NZ for 3 weeks so I probably won't have time to review again before commit but great work!

1. Okay, I will add one. It may have to be a meta-issue because different field types will require different kinds of processing and I'm thinking it might be quicker and easier to commit that stuff piecemeal.
2. Fixed!
3. Fixed by #2.
4. As we discussed on IRC, this is accounted for by d7_field_formatter_settings' handling of the "hidden" formatter type, which will caused the row to be skipped. The tests cover this too.
5. Fixed.

Thanks, looks good!

I see test failures on the horizon. This patch removes the default_image stuff, since it will need a proper lookup into the {file_managed} table. That should be done in a follow-up issue, though.

Id's are preserved, what would the lookup be needed for?

To get the file's uuid, since it appears that the default image is referred to by uuid...?

Tentatively going back to RTBC because DrupalCI has passed all the tests. Testbot has been excruciatingly slow of late (taking hours upon hours).

Okay, finally. Back to RTBC.

This all looks like pretty standard Migrate stuff, and sounds like chx and benjy are +1. This is an important blocker to the "big 4" migrations, so let's get 'er done!

Committed and pushed to 8.0.x. Thanks!

Went to test this out on a real life database tonight. Wonder if this is related? #2553595: Field formatter migration stuck in an infinite loop