Problem/Motivation

We wanted to keep the LoadEntity simple and not pluginify field types so we moved the per field type processing into methods on the LoadEntity. That works OK for core but after writing a sample module for contrib (https://www.drupal.org/sandbox/benjy/2422307) it was obvious the DX isn't really good enough.

Right now as a contrib module providing a custom field type you have to hook yourself into 4 migrations to make sure everything is migrated appropriately. Any less and things blow up.

Proposed resolution

Add a new plugin type and call that from LoadEntity for per CCK field type processing.

Remaining tasks

Reviews

User interface changes

n/a

API changes

New plugin type for adding support for custom field types in migrate.

Files: 
CommentFileSizeAuthor
#26 interdiff.txt1.29 KBbenjy
#26 2423435-26.patch14.31 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,261 pass(es). View
#23 interdiff.txt2.1 KBbenjy
#23 2423435-23.patch14.18 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,517 pass(es). View
#20 interdiff.txt4.93 KBbenjy
#20 2423435-20.patch13.66 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,989 pass(es). View
#15 2423435_15.patch13.2 KBchx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,992 pass(es). View
#14 2423435-14.patch15.97 KBbenjy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,995 pass(es). View

Comments

chx’s picture

Title: Add CckField Plugin type to Migrate » Add CckField Plugin type to migrate_drupal

This is not a migrate thing; it's a migrate_drupal thing.

benjy’s picture

FileSize
19.32 KB

OK, first version of the patch, leaving as postponed because it depends on #2422227: Add setProcessOfProperty to Migration entity

Few notes:

* I added methods like isFieldMigration(), isFieldWigetMigration to MigrationStorage so we could easily add the D7 migrations when they come about. We could also fire a drupal_alter here to allow other modules to add migrations in case D5 migrations ever come in contrib. Or, they could override the storage controller.

* Core isn't using the new plugin type yet. If we're happy on the general approach i'll convert a few field types. It does work though, I tested with a contrib port of iFrame: https://www.drupal.org/sandbox/benjy/2422307

benjy’s picture

Status: Postponed » Needs review
FileSize
17.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,683 pass(es), 3 fail(s), and 11 exception(s). View
3.87 KB

Added a migration id to plugin method map instead of the individual methods.

Status: Needs review » Needs work

The last submitted patch, 3: cck-field-processing-3.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
17.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,730 pass(es), 3 fail(s), and 323 exception(s). View
1.96 KB

Further clean-up, not looked at the fails yet.

Status: Needs review » Needs work

The last submitted patch, 5: cck-field-processing-5.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
18.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,706 pass(es), 0 fail(s), and 11 exception(s). View
1.68 KB

Nasty little problem where the source plugin's config gets cached because the storage controller calls getSourcePlugin() and then if anything else tries to set the source config after the migration has been loaded it is never used.

The attached patch shows the problem but I'm not convinced it's the correct approach?

Another approach could be to add a setSourceConfig() method onto the migration and then invalidate the source plugin cache from there? I don't much like $entity->set('source', $source) anyway.

Status: Needs review » Needs work

The last submitted patch, 7: 2423435-7.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
18.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,765 pass(es). View
881 bytes

The problem was trying to access fieldData() for the migration with the load plugin attached when we only want that for the bundle migration.

We could have just checked for that migration like below but I decided to checkout against $migration->load

-      if ($source_plugin instanceof CckFieldMigrateSourceInterface) {
+      if ($source_plugin instanceof CckFieldMigrateSourceInterface && $entity_id !== 'd6_cck_field_values') {
chx’s picture

Wouldn't !$migration->get('load') suffice? Also, I am surprised that superflous processing causes tests to break. Of course we shouldn't do superflous processing so this needs to stay but isn't processing a no-op when applied to migrations we do not need/want to process?

benjy’s picture

Wouldn't !$migration->get('load') suffice?

Yes, that would also work.

Also, I am surprised that superflous processing causes tests to break

It isn't the actual processing that causes the error. It's an undefined index CckFieldValues::fieldData() when $this->configuration['bundle'] is not set. Eg for the top level d6_cck_field_values migration.

benjy’s picture

FileSize
4.92 KB
20.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,722 pass(es). View

Switched to using getDerivativeId() instead of checking the load plugin array.

Added a setSourceConfig method which is only really used in tests and invalidated the sourcePlugin cache.

benjy’s picture

Status: Needs review » Postponed

This is nearly ready but postponed on: #2422227: Add setProcessOfProperty to Migration entity

Just need to convert some core field types to use this.

benjy’s picture

Status: Postponed » Needs review
FileSize
15.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,995 pass(es). View

OK, this is unblocked now. Re-rolled against HEAD.

chx’s picture

FileSize
13.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,992 pass(es). View

Do we perhaps want to override set() instead? I think we want to.

benjy’s picture

Yes I like that, I think it's better than requiring another public method on the entity.

benjy’s picture

Issue summary: View changes
amateescu’s picture

Here's a small review, nothing alarming stands out :)

  1. +++ b/core/modules/migrate/src/Plugin/MigratePluginManager.php
    @@ -77,4 +70,20 @@ public function createInstance($plugin_id, array $configuration = array(), Migra
    +      'destination' => 'Drupal\migrate\Plugin\MigrateDestinationInterface',
    +      'process' => 'Drupal\migrate\Plugin\MigrateProcessInterface',
    +      'source' => 'Drupal\migrate\Plugin\MigrateSourceInterface',
    +      'id_map' => 'Drupal\migrate\Plugin\MigrateIdMapInterface',
    +      'entity_field' => 'Drupal\migrate\Plugin\MigrateEntityDestinationFieldInterface',
    
    +++ b/core/modules/migrate_drupal/src/Plugin/MigratePluginManager.php
    @@ -24,11 +21,11 @@ class MigratePluginManager extends BaseMigratePluginManager {
    +      'load' => 'Drupal\migrate_drupal\Plugin\MigrateLoadInterface',
    +      'cckfield' => 'Drupal\migrate_drupal\Plugin\MigrateCckFieldInterface',
    

    Afaik, we have to include the leading slash everywhere we use a class or interface name as a string.

  2. +++ b/core/modules/migrate_drupal/src/MigrationStorage.php
    @@ -84,6 +98,10 @@ public function loadMultiple(array $ids = NULL) {
    +    // Allow modules providing cck field plugins to alter the required
    +    // migrations for successfully migration a field type.
    

    'successfully migration' sounds a bit weird :)

  3. +++ b/core/modules/migrate_drupal/src/MigrationStorage.php
    @@ -113,4 +131,83 @@ public function save(EntityInterface $entity) {
    +   * @param array $entities
    +   *   An array of migration entities.
    ...
    +    /** @var \Drupal\migrate\Entity\Migration $migration */
    

    We can save the use of /** @var ... */ by using \Drupal\migrate\Entity\Migration[] instead of array in the docblock.

  4. +++ b/core/modules/migrate_drupal/src/MigrationStorage.php
    @@ -113,4 +131,83 @@ public function save(EntityInterface $entity) {
    +        $cck_plugins = $this->getCckFieldPlugins();
    ...
    +        $plugins = $this->getCckFieldPlugins();
    

    Looks like we can move these at the top of the method to save some calls in the foreach.

  5. +++ b/core/modules/migrate_drupal/src/MigrationStorage.php
    @@ -113,4 +131,83 @@ public function save(EntityInterface $entity) {
    +  protected function getMigrationPluginMethodMap() {
    +    return [
    +      'd6_field' => 'processField',
    +      'd6_field_instance_widget_settings' => 'processFieldWidget',
    +      'd6_field_formatter_settings' => 'processFieldDisplay',
    +    ];
    +  }
    

    Are we missing a d6_field_instance here?

  6. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
    @@ -0,0 +1,60 @@
    +   * Apply any custom processing to the field display migration.
    ...
    +  public function processFieldDisplay(MigrationInterface $migration);
    ...
    +   * Get a map between D6 formatters and D8 formatters for this field type.
    ...
    +  public function getFieldDisplayMap();
    

    How about renaming everything to use 'formatter' instead of 'display'?

amateescu’s picture

Also, related to the last point (#18.6), it seems that we're missing a getFieldWidgetMap() method :)

benjy’s picture

FileSize
13.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,989 pass(es). View
4.93 KB
  1. I looked at some other classes that extend DefaultPluginManager and that doesn't seem to be the case. Is it documented somewhere?
  2. Fixed.
  3. Great, thanks.
  4. I purposely put those inside the if() statements because calling the method multiple times seemed like less of an issue since the result is cached. But loading all the plugins if we don't need to seemed worse.
  5. OK, renamed to formatter.
  6. We don't have a getFieldWidgetMap() because there is only ever a 1 to 1 mapping between the incoming widget and the new widget. See d6_field_instance_widget_settings. But for the formatters, we can have many formatters for the one field type.
amateescu’s picture

So, I looked at d6_field_instance_widget_settings and that seems to hardcode the 1:1 mapping for *known widgets*, but isn't it possible, at least theoretically, that a contrib module provided two widgets in D6 and combined them into a single one (with more options) in D8?

benjy’s picture

Sure, in that use you'd just add two entries to this same map, same as we've done for datetime_default

I added getFieldFormatterMap() to try give a slightly nicer DX. It allows contrib authors to skip implementing processFieldFormatter() and simply allows them return an array from getFieldFormatterMap() so the parent can handle updating the process array.

processFieldWidget() also has a default implementation in CckFieldPluginBase that sets a sane default based on your module name.

Nothing stops contrib modules doing what they want in processFieldWidget() and processFieldFormatter() and in that case they could ignore the return value from getFieldFormatterMap() altogether.

Maybe i've just confused things by adding getFieldFormatterMap(), we could change how that works?

benjy’s picture

Assigned: benjy » Unassigned
FileSize
14.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,517 pass(es). View
2.1 KB

OK, discussed this on IRC and we've now added a getFieldWidgetMap() to be consistent. I should also point out the lack of tests here, we'll be using the API in core for the link field etc which will give us implicit coverage, created a follow-up #2448501: Use the migrate cckfield plugin type for the link field

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, looks good to me now :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate_drupal/src/MigrationStorage.php
    @@ -10,6 +10,8 @@
    +use Drupal\migrate\Entity\MigrationInterface;
    

    Not used.

  2. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
    @@ -0,0 +1,76 @@
    +  /**
    +   * Apply any custom processing to the cck bundle migrations.
    +   *
    +   * @param \Drupal\migrate\Entity\MigrationInterface $migration
    +   *   The migration entity.
    +   */
    +  public function processCckFieldValues(MigrationInterface $migration, $field_name, $data);
    

    Missing docblocks for $field_name and $data

benjy’s picture

Status: Needs work » Needs review
FileSize
14.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,261 pass(es). View
1.29 KB

Fixed.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Alex's concerns are addressed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Re #23 I think the lack of explicit test coverage might come back to bite us but maybe when #2448501: Use the migrate cckfield plugin type for the link field lands it will be more obvious how to add explicit coverage.

Committed 04a17f8 and pushed to 8.0.x. Thanks!

  • alexpott committed 04a17f8 on 8.0.x
    Issue #2423435 by benjy, chx: Add CckField Plugin type to migrate_drupal
    

Status: Fixed » Closed (fixed)

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