Problem/Motivation

In #2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted), we need to have access to the field instances when we migrate field bases.

Proposed resolution

Refactor the Field source plugin so that it returns the field definition as pure a form as possible, and also an array of arrays of all instances of the field as unmodified as possible.

Remaining tasks

Write a patch, Review

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

maxocub created an issue. See original summary.

maxocub’s picture

Status: Active » Needs review
FileSize
4.51 KB

Here's a first patch.

maxocub’s picture

Oups, forgot some TODOs.

maxocub’s picture

Add instances to the expected result in the Field source plugin test.

phenaproxima’s picture

  1. +++ b/core/modules/field/src/Plugin/migrate/source/d7/Field.php
    @@ -19,36 +19,46 @@ class Field extends DrupalSqlBase {
    +      ->fields('fc', ['field_name', 'type', 'module', 'locked', 'data', 'cardinality', 'translatable'])
    

    I would prefer if we simply return all fields in field_config, without discriminating. There is no need for the plugin to have an opinion about which fields to return, and the more information we can provide to the process pipeline, the happier we will ultimately be.

  2. +++ b/core/modules/field/src/Plugin/migrate/source/d7/Field.php
    @@ -19,36 +19,46 @@ class Field extends DrupalSqlBase {
    +    $query->addField('fci', 'field_name', 'instance_field_name');
    +    $query->addField('fci', 'entity_type', 'instance_entity_type');
    +    $query->addField('fci', 'bundle', 'instance_bundle');
    +    $query->addField('fci', 'data', 'instance_data');
    

    Similarly, I'd prefer to select all data from field_config_instance. I realize we probably can't do this in a single query, so I'd be fine with fetching fci.* for each row in initializeIterator(). Performance matters when there are a million nodes or users, but I'm hard-pressed to imagine a Drupal site with a million fields.

  3. +++ b/core/modules/field/src/Plugin/migrate/source/d7/Field.php
    @@ -19,36 +19,46 @@ class Field extends DrupalSqlBase {
    +      $field_id = $result['entity_type'] . ':' . $result['field_name'];
    

    In the name of simplicity, I'm in favor of breaking backwards compatibility rather boldly here. To wit, let's not bother keying each row to a field name and entity type. Unless there's a compelling reason to do that (beyond BC concerns), screw it; just key it by field name, I say. Migrations will have to be adjusted, but in the cause of unscrambling this API, I'm at peace with that.

  4. +++ b/core/modules/field/src/Plugin/migrate/source/d7/Field.php
    @@ -19,36 +19,46 @@ class Field extends DrupalSqlBase {
    +      foreach (array_keys($this->getFieldFields()) as $property) {
    +        $rows[$field_id][$property] = $result[$property];
    +      }
    +
    +      foreach (array_keys($this->getFieldInstanceFields()) as $property) {
    +        $rows[$field_id]['instances'][$bundle][$property] = $result['instance_' . $property];
    +      }
    

    We won't need $this->getFieldFields() and $this->getFieldInstanceFields() if we simply do a select from field_config_instance here to retrieve the instance data.

  5. +++ b/core/modules/field/src/Plugin/migrate/source/d7/Field.php
    @@ -58,6 +68,56 @@ public function prepareRow(Row $row, $keep = TRUE) {
    +  public function count() {
    +    return $this->initializeIterator()->count();
    +  }
    

    If we make the query adjustments I asked for, I'm not sure why we need this...?

maxocub’s picture

What about this way? I think it is simpler and cleaner.
Of course we can't extend this class in FieldInstance as we suggested (as we're only fetching distinct rows), but I don't really see the advantage of using this class as a base for FieldInstance. It requires to much work in the initializeIterator() if we do.
What do you think?

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

heddn’s picture

Assigned: phenaproxima » heddn

Assigning to myself for review this week.

heddn’s picture

Assigned: heddn » Unassigned

I don't see anything in here I disagree with. In fact, I agree with #5.3. And wonder if we want to mark the Field class as either final or @internal. I cannot think of a case when anyone would want to change or extend it. But at the very least, marking with @internal gives us more flexibility if we ever decide to change this in the future.

Leaving in NR to get feedback on final or @internal.

heddn’s picture

Status: Needs review » Needs work

NW for adding @internal to Field class. Confirmed this is our desired direction in IRC with adam and maxime.

maxocub’s picture

Status: Needs work » Needs review
FileSize
8.73 KB
566 bytes

Added the @internal.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

I don't think we are breaking BC, no need for a CR. This looks good to go.

  • catch committed e6cd4b0 on 8.4.x
    Issue #2891932 by maxocub: Refactor D7 Field source plugin
    
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed e6cd4b0 and pushed to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

maxocub’s picture

maxocub’s picture