Problem/Motivation

Follow-up to #2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted)

D7 text and text_long fields with Plain text instances are now migrated as string and string_long fields.

Their widget types are incorrectly migrated as text_textfield and text_textarea instead of string_textfield and string_textarea.
Their formatter types are incorrectly migrated as text_default instead of string and basic_string.

Proposed resolution

To correctly map those widget/formatter types, we need to know both the field type and the widget/formatter type. This can be done with the new process_field plugin.

Remaining tasks

Code it. Review.

User interface changes

None.

API changes

New methods added to the FieldPluginBase:
* getFieldFormatterType()
* getFieldWidgetType()

Data model changes

None.

CommentFileSizeAuthor
#10 2906203-10.patch12.53 KBmaxocub
#8 2906203-8-review.txt12.53 KBmaxocub
#8 interdiff-2906203-6-8.txt3.06 KBmaxocub
#8 2906203-8.patch143.38 KBmaxocub
#6 interdiff-2906203-2-6.txt1.68 KBmaxocub
#6 2906203-6-review.txt11.27 KBmaxocub
#6 2906203-6.patch142.15 KBmaxocub
#2 2906203-2-review.txt10.49 KBmaxocub
#2 2906203-2.patch141.36 KBmaxocub
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2906203-2.patch141.36 KB
2906203-2-review.txt10.49 KB

Here's a first patch, including the patch in #2842222-136: D7 Plain text fields incorrectly migrated to D8 as Text (formatted).
New code is in the review file.

maxocub’s picture

maxocub CreditAttribution: maxocub commented 
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/FieldMigration.php
@@ -140,13 +140,13 @@ public function getProcess() {
         try {
-          $plugin_id = $this->cckPluginManager->getPluginIdFromFieldType($field_type, [], $this);
-          $manager = $this->cckPluginManager;
+          $plugin_id = $this->fieldPluginManager->getPluginIdFromFieldType($field_type, [], $this);
+          $manager = $this->fieldPluginManager;
         }
         catch (PluginNotFoundException $ex) {
           try {
-            $plugin_id = $this->fieldPluginManager->getPluginIdFromFieldType($field_type, [], $this);
-            $manager = $this->fieldPluginManager;
+            $plugin_id = $this->cckPluginManager->getPluginIdFromFieldType($field_type, [], $this);
+            $manager = $this->cckPluginManager;
           }

This code in FieldMigration seems wrong. Since the cckField plugins have been deprecated, we should look for fieldPluginManager before cckPluginManager.
I made the change here so my patch would work, but if someone think it should be it's own bug report, tell me and I'll open one.

maxocub’s picture

maxocub CreditAttribution: maxocub commented 
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/FieldPluginBase.php
@@ -48,6 +48,13 @@ public function processFieldWidget(MigrationInterface $migration) {
   /**
    * {@inheritdoc}
    */
+  public function getFieldFormatterType(Row $row) {

@@ -55,6 +62,13 @@ public function getFieldFormatterMap() {
   /**
    * {@inheritdoc}
    */
+  public function getFieldWidgetType(Row $row) {

Oups, I forgot to add docs here, since those are not on the interface.

I don't think we can add those to the interface without breaking BC, but is it OK to add them to the base class anyway?

Status: Needs review » Needs work

The last submitted patch, 2: 2906203-2.patch, failed testing. View results

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review
FileSize
2906203-6.patch142.15 KB
2906203-6-review.txt11.27 KB
interdiff-2906203-2-6.txt1.68 KB 
@@ -81,8 +93,9 @@ public function getFieldWidgetMap() {
    */
   public function processFieldFormatter(MigrationInterface $migration) {
     $process = [];
+    $plugin_id = preg_replace('/d[67]_/', '', $this->pluginId);
     foreach ($this->getFieldFormatterMap() as $source_format => $destination_format) {
-      $process[0]['map'][$this->pluginId][$source_format] = $destination_format;
+      $process[0]['map'][$plugin_id][$source_format] = $destination_format;
     }
     $migration->mergeProcessOfProperty('options/type', $process);
   }

The fact that the d6\TextField and d7\TextField plugin IDs are d6_text and d7_text is causing the tests to fail. The plugin ID is used in the static map and is supposed to represent the module. Since two plugins can't have the same ID, and since there's already a deprecated cck field plugin with the ID 'text', this ugly hack seems necessary. Anybody have a better idea?

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Status: Needs review » Needs work
Issue tags: +Migrate BC break 
  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/FieldMigration.php
@@ -140,13 +140,13 @@ public function getProcess() {
+          $plugin_id = $this->fieldPluginManager->getPluginIdFromFieldType($field_type, [], $this);
+          $manager = $this->fieldPluginManager;
...
+            $plugin_id = $this->cckPluginManager->getPluginIdFromFieldType($field_type, [], $this);
+            $manager = $this->cckPluginManager;

    Nice find and good fix. 

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/FieldPluginBase.php
@@ -46,6 +46,19 @@ public function processFieldWidget(MigrationInterface $migration) {
+  public function getFieldFormatterType(Row $row) {

@@ -53,6 +66,19 @@ public function getFieldFormatterMap() {
+  public function getFieldWidgetType(Row $row) {

    I think adding this to the interface is fine. It isn't much of a break to BC and we are implementing it on the base class. I feel it is better to document the API than to leave it un-documented, just for the sake of maintaining some semblance of BC. un-documented == not adding to interface. 

  3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/FieldPluginBase.php
@@ -67,8 +93,9 @@ public function getFieldWidgetMap() {
+    $plugin_id = preg_replace('/d[67]_/', '', $this->pluginId);

    Deserves a comment.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review
FileSize
2906203-8.patch143.38 KB
interdiff-2906203-6-8.txt3.06 KB
2906203-8-review.txt12.53 KB

Re #7:
2. Added the methods to the interface.
3. Added a comment.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Status: Needs review » Postponed

Looks good here. Not just waiting on parent issue to unblock.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Postponed » Needs review
FileSize
2906203-10.patch12.53 KB

Unpostponed and re-rolled.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Version: 8.5.x-dev » 8.4.x-dev
Status: Needs review » Reviewed & tested by the community

All my feedback was addressed, we were just waiting to un-postpone on #2842222: D7 Plain text fields incorrectly migrated to D8 as Text (formatted). On to RTBC.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Issue tags: -Needs change record

Added a change record.

  • catch committed 3148d1c on 8.5.x 
    Issue #2906203 by maxocub: Widgets/formatters: D7 Plain text fields...

  • catch committed d2f7a53 on 8.4.x 
    Issue #2906203 by maxocub: Widgets/formatters: D7 Plain text fields...

catch credited catch.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

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

(heddn I failed to assign commit credit before commit, too keen to get the patch in, but adding it on the issue at least...).

maxocub’s picture

maxocub CreditAttribution: maxocub for Acquia commented
maxocub’s picture

maxocub CreditAttribution: maxocub as a volunteer and for Acquia commented

Status: Fixed » Closed (fixed)

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