Problem/Motivation

In the same sense as in #2891932: Refactor D7 Field source plugin, the FieldInstance source plugin should returns all instance, as unmodified as possible, and also the field definition.
It should also be returning an array of arrays of all the instances that are using the same base field because we will this in #2842222: [PP-1] D7 Plain text fields incorrectly migrated to D8 as Text (formatted).
Since three other field source plugins are fetching the field_config_instance table, and since we will also need the array of instances for processing the widget and formatter, we should extend the refactored FieldInstance class in those, which are:

  • FieldInstancePerFormDisplay
  • FieldInstancePerViewMode
  • ViewMode

Proposed resolution

Refactor FieldInstance source plugin by removing the extra processing and by adding the field definition and array of instances.
Refactor the three other classes by extending FieldInstance.

Remaining tasks

Write a patch, Review

User interface changes

None.

API changes

Some fields returned by the source plugins are changing to best mirror what's coming from the source.

Data model changes

None.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

maxocub created an issue. See original summary.

maxocub’s picture

Status: Active » Needs review
FileSize
8.11 KB
12.85 KB

Here's a first patch, and a patch including the one from #2891932: Refactor D7 Field source plugin for testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2891935-and-2891932-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

maxocub’s picture

Status: Needs work » Needs review
FileSize
17.32 KB
10.32 KB
2.21 KB

Fix the failing test and add the field_definition to the expected result for the FieldInstance source plugin test.

maxocub’s picture

New approach without extending the Field source plugin.

Status: Needs review » Needs work

The last submitted patch, 5: 2891935-5.patch, failed testing. View results

maxocub’s picture

Status: Needs work » Needs review
FileSize
13.24 KB
936 bytes
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
Status: Needs review » Needs work

I really like how this is shaping up. Some minor points noted.

  1. +++ b/core/modules/field/migration_templates/d7_field_instance.yml
    @@ -19,15 +19,15 @@ process:
    -      - instance_settings
    -      - widget_settings
    -      - field_settings
    +      - settings
    +      - widget
    +      - field_definition
    ...
           - default_value
    -      - widget_settings
    +      - widget
    

    This is a BC break that gets exposed to users of migrate_drupal. If someone starts migrating their site on 8.3, then in 2 months updates the site to 8.4, the field instance migrations are going to die a horrible death. We can either address this in a CR or keep a BC shim in place. Perhaps we can go with a CR since migrate_drupal is still experimental and the needed changes for the scenarios just mentioned are fairly minor.

  2. +++ b/core/modules/text/src/Plugin/migrate/cckfield/TextField.php
    @@ -43,7 +43,9 @@ public function getFieldFormatterMap() {
    +    $widget_type = isset($field_info['widget_type']) ? $field_info['widget_type'] : $field_info['widget']['type'];
    

    Are we sure we need this? I don't see where we are pulling 'widget_type'.

maxocub’s picture

Re #10.2: This TextField plugin is used both for D6 & D7 (for now at least). In D6, the source plugin return a widget_type key, but in D7 it doesn't. The D7 fieldInstance source plugin has been addapted to reproduce what the D6 one was doing, but in the name of returning the source as unmodified as possible, I remove the processing of this key in the D7 source and I added this check.

maxocub’s picture

Also, while working on #2842222: [PP-1] D7 Plain text fields incorrectly migrated to D8 as Text (formatted), I realized we would also need to return an array of all instances of the same base field with each instances.
So, in addition of the 'field_definition', we would also need 'instances' added to each row.

This is because we need to know if all instances are plain text or formatted text to know what type of widget & formatter to use.

heddn’s picture

Issue tags: +Needs change record

Chatted about this in IRC and we are OK with breaking BC and the necessary updates for the field_instance yml file. But that means we need a CR.

+++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
--- a/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php
+++ b/core/modules/field/src/Plugin/migrate/source/d7/FieldInstance.php

Mark this @internal.

maxocub’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
15.18 KB
3.82 KB

Added the @internal and the 'instances' array to each row as mentioned in #12.

heddn’s picture

Status: Needs review » Needs work

Going to work on a draft CR. Patch in #14 is looking good.

@maxocub in IRC: FieldInstancePerFormDisplay.php FieldInstancePerViewMode.php ViewMode.php all should maybe extend FieldInstance where possible

Back to NW.

maxocub’s picture

Status: Needs work » Needs review
FileSize
23.75 KB
9.15 KB

Here's the refactoring of FieldInstancePerFormDisplay, FieldInstancePerViewMode & ViewMode, all extending FieldInstance.

This was needed for #2842222: [PP-1] D7 Plain text fields incorrectly migrated to D8 as Text (formatted), to have access to all field instances using the same base field while we are processing the widget and the formatter.

Status: Needs review » Needs work

The last submitted patch, 16: 2891935-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

maxocub’s picture

Title: Refactor D7 FieldInstance source plugin » Refactor D7 FieldInstance source plugin and other field instance related source plugins
Issue summary: View changes
Status: Needs work » Needs review
FileSize
48.39 KB

Fix the tests and update the IS.

maxocub’s picture

FileSize
24.63 KB

The interdiff didn't completely uploaded with the last comment, so here it is.

Jo Fitzgerald’s picture

I'm gonna be 'that guy' - here are a few coding standards corrections.

heddn’s picture

Assigned: Unassigned » heddn

Going to review this.

maxocub’s picture

Auto-nitpicking: Forgot to add 'instances' to the FieldInstance fields().

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

Status: Needs review » Needs work
Issue tags: -Needs change record

One small question. Otherwise, this looks good to go. I also added a quick 8.5 test, just to make sure nothing fishing is going on here.

+++ b/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceDefaults.php
@@ -19,6 +19,11 @@ class FieldInstanceDefaults extends ProcessPluginBase {
+    if ($widget_type == 'email_textfield' && $default_value) {
+      $default_value[0]['value'] = $default_value[0]['email'];

This seems odd. Can we add a comment why this is needed?

maxocub’s picture

Status: Needs work » Needs review
FileSize
48.42 KB
916 bytes

Re: #24:
This snippet of code was previously in the FieldInstance source plugin, I moved it in the more logical FieldInstanceDefaults process plugin.
I added a comment to explain why it is needed.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

All my concerns addressed. LGTM and unblock our critical.

maxocub’s picture

@heddn: Thanks for the CR, I added some missing changes.

heddn’s picture

Assigned: heddn » Unassigned

Un assigning

xjm’s picture

In #2842222-107: [PP-1] D7 Plain text fields incorrectly migrated to D8 as Text (formatted), that critical issue was marked postponed on this issue. If this issue is a hard blocker, it needs to be critical itself.

Promoting this one to critical, at least for now so that is visible and can be evaluated.

heddn’s picture

This is a hard blocker. So bumping priority is a good suggestion.

maxocub’s picture

Priority: Normal » Critical

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: 2891935-25.patch, failed testing. View results

maxocub’s picture

Status: Needs work » Reviewed & tested by the community

Seems like an unrelated fail, back to RTBC.

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed 4ff7617 on 8.5.x
    Issue #2891935 by maxocub, Jo Fitzgerald, heddn: Refactor D7...