Problem/Motivation

Once #2423435: Add CckField Plugin type to migrate_drupal lands we should use the new API for our field types in core that need extra processing.

Proposed resolution

Implement the new plugin type.

Remaining tasks

Write patch

User interface changes

n/a

API changes

n/a

CommentFileSizeAuthor
#30 interdiff-2448501-28-30.txt1.35 KBphenaproxima
#30 2448501-30.patch9.88 KBphenaproxima
#28 interdiff-2448501-27-28.txt494 bytesphenaproxima
#28 2448501-28.patch8.77 KBphenaproxima
#27 interdiff-2448501-14-41.txt2.65 KBphenaproxima
#27 2448501-41.patch8.78 KBphenaproxima
#14 interdiff.txt731 bytesultimike
#14 2448501-14.patch6.59 KBultimike
#1 2448501-1.patch1.24 KBultimike
#4 2448501-4.patch6.29 KBultimike
#6 2448501-6.patch5.33 KBultimike
#10 interdiff.txt3.24 KBultimike
#10 2448501-10-FAIL.patch5.33 KBultimike
#10 2448501-10.patch6.58 KBultimike
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
FileSize
2448501-1.patch1.24 KB

While working with a sandbox module to test cckfield plugins, benjy discovered a bug in the MigrationStorage class. I've attached a fix for it here as a start for this task.

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

I started looking at this today and need some help with a plan of attack. I figure it can mostly be based on Benjy's iFrame sandbox module, so that's what I'm starting with.

For the class LinkField extends CckFieldPluginBase what do we actually need? Should the formatter and other settings maps remain in the migrate.migration.d6_field_formatter_settings.yml or should those be moved into the class?

The processors seems straight-forward - I'm not sure anything needs to be changed in CckLink.

I'm happy to do the work on this one, and it will be a learning exercise for me, so I'm looking forward to it. Just need a little direction.

Thanks,
-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented

Ignore my last comment - I think I'm making some good progress...

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Active » Needs review
FileSize
2448501-4.patch6.29 KB

Ok - let's give this a shot. Patch attached.

-mike

Status: Needs review » Needs work

The last submitted patch, 4: 2448501-4.patch, failed testing.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
FileSize
2448501-6.patch5.33 KB

Ok - let's give this another shot. This time with more unserializing.

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review

Update status.

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented

How about a fail patch with the change in MigrationStorage not applied? Do we have coverage for that now?

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/cckfield/LinkField.php
@@ -0,0 +1,68 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function processField(MigrationInterface $migration) {
+    // See d6_field.yml.
+    $process[0]['map'][$this->pluginId][$this->pluginId] = 'link';
+    $migration->mergeProcessOfProperty('type', $process);
+  }

    This is the same as the base class implementation, so lets remove it. 

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/cckfield/LinkField.php
@@ -0,0 +1,68 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function processFieldWidget(MigrationInterface $migration) {
+    // See d6_field_instance_widget_settings.yml.
+    $process['type']['map'][$this->pluginId] = 'link_default';
+    $migration->mergeProcessOfProperty('options/type', $process);
+  }

    The processFieldWidget() in the parent uses getFieldWidgetMap() and the default implementation for getFieldWidgetMap() is all we need.

    Lets remove this as well.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
FileSize
2448501-10.patch6.58 KB
2448501-10-FAIL.patch5.33 KB
interdiff.txt3.24 KB

Updated patch attached. Also, and interdiff as well as a "fail" patch for the MigrationStorage change. Some notes:

  1. I couldn't get the fail patch working for a bit, until I found that LoadEntity was adding the Link field processor. This is why MigrationStorage worked for Link, but not for the iFrame plugin. I removed the Link processor stuff from MigrationStorage. The FAIL patch is therefore the full patch minus the MigrationStorage changes.
  2. I removed the redundant methods from LinkField as you requested above.
  3. I updated the doc block for LinkField as well.
  4. I manually tested the iframe sandbox module with this change as well - all is good.

-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review

The last submitted patch, 10: 2448501-10-FAIL.patch, failed testing.

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented
Status: Needs review » Reviewed & tested by the community

Yes this much better, glad I reviewed again. So simple for new field types.

Super tiny nitpick. More than 80chars.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/cckfield/LinkField.php
@@ -0,0 +1,49 @@
+    // See d6_field_formatter_settings.yml and CckFieldPluginBase processFieldFormatter().
ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Reviewed & tested by the community » Needs review
FileSize
2448501-14.patch6.59 KB
interdiff.txt731 bytes

Without coding standards, we have anarchy. Super tiny nitpick fixed.

-mike

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented
Status: Needs review » Reviewed & tested by the community

Great!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2448501-14.patch, failed testing.

ultimike queued 14: 2448501-14.patch for re-testing.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Reviewed & tested by the community

Moving back to RTBC after testbot failure.

-mike

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2448501-14.patch, failed testing.

isntall queued 14: 2448501-14.patch for re-testing.

The last submitted patch, 14: 2448501-14.patch, failed testing.

mlhess queued 14: 2448501-14.patch for re-testing.

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented
Status: Needs work » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/migrate_drupal/src/MigrationStorage.php
@@ -10,6 +10,7 @@
 use Drupal\migrate\MigrationStorage as BaseMigrationStorage;

@@ -154,7 +155,7 @@ protected function applyCckFieldProcessors(array $entities) {
+      if ($source_plugin instanceof CckFieldMigrateSourceInterface && strpos($entity_id, SourcePluginBase::DERIVATIVE_SEPARATOR)) {

Given what strpos returns i.e. false or the position I think at least a comment is needed here. Also why doesn't getDerivativeId() work? And is it indicative of a bigger problem?

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented
Status: Needs work » Needs review

The reason is because the source plugin doesn't change its id when we're using the LoadEntity to dynamically create migrations such as cck_field_values:page etc.

Therefore we need to check the migration entity id.

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
Status: Reviewed & tested by the community » Needs review
FileSize
2448501-41.patch8.78 KB
interdiff-2448501-14-41.txt2.65 KB

This updates the patch in #14 by removing the call to \Drupal::service() in MigrationStorage::getCckPluginManager(). Entity handlers can use dependency injection, so I didn't see any reason to use \Drupal in there.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
FileSize
2448501-28.patch8.77 KB
interdiff-2448501-27-28.txt494 bytes

Fixed incorrect class name in MigrationStorage::__construct()'s doc comment. And the wrong comment number in the patch and interdiff file names (I must be improperly caffeinated).

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented
Status: Needs review » Needs work

That's fine by me, but if we're doing that lets remove the getCckPluginManager() method altogether.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
Status: Needs work » Needs review
FileSize
2448501-30.patch9.88 KB
interdiff-2448501-28-30.txt1.35 KB

Got rid of getCckPluginManager() and a line of extra white space.

benjy’s picture

benjy CreditAttribution: benjy at CodeDrop commented
Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Migrate is not frozen in beta. Committed c689008 and pushed to 8.0.x. Thanks!

  • alexpott committed c689008 on 8.0.x 
    Issue #2448501 by ultimike, phenaproxima, benjy: Use the migrate...

Status: Fixed » Closed (fixed)

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

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
Related issues: +#2528688: Create cckfield plugin for file fields
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima commented
Related issues: +#2529144: Create cckfield plugin for text fields

Added another follow-up issue.