Problem/Motivation

We need to support migration of several different types of legacy reference fields to D8:

  • Node and user reference, for both D6 and D7
  • D7 taxonomy term reference fields
  • D7 entity reference fields

Proposed resolution

Create a base class for cckfield plugins that handle all reference-type fields, since they tend to be more similar than they are different.

Remaining tasks

Write the patch, tests, docs. Let's pull ReferenceBase out of the patchzilla in #191, clean it up, and turn it into its own patch.

User interface changes

None.

API changes

None.

Files: 

Comments

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea

Let's see.

claudiu.cristea’s picture

Title: Add migrate support for Drupal 6 node reference » Add migrate support for Drupal 6 node & user reference
Status: Active » Needs review
Related issues: +#2447729: Add migrate support for Drupal 6 user reference
FileSize
32.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid patch format in references_1.patch. View

As the userreference is very similar, I merged the 2 issues. Otherwise there will be a lot of conflicts between the 2 patches because they are touching the same code. Closing #2447729: Add migrate support for Drupal 6 user reference in favour of this.

Here's a first patch. Aspects that still need attention:

  1. I don't see a way to migrate the cases when the list of referenceable nodes/users are obtained from a view. I guess that the views migration has not ran in the moment that field storage & field config are migrated. Even migrating that setting we cannot check if the destination view is a candidate for being used as list for the entity reference field.
  2. I don't see a way to store the default value (I see that also filefield and imagefield, which are also references, were left this out). This is because the default value of an Entity Reference in D8 is stored with its UUID. But the UUID of the node/user is not known in the moment of migration. I don't know if right now we have something similar to stubs in D8 Migrate.
  3. The userreference setting referenceable_status cannot be migrated because we don't have a destination correspondent setting.
claudiu.cristea’s picture

Adding the parent, meta issue.

Status: Needs review » Needs work

The last submitted patch, 2: references.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
31.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,508 pass(es). View
claudiu.cristea’s picture

FileSize
57.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add_migrate_support_for-2447727-6.patch. Unable to apply patch. See the log in the details link for more information. View

I made d6_field_instance dependent on d6_node and d6_user to be able to get the default_value UUID. But I think I'm missing something here related to dependencies. Running only MigrateFieldInstanceTest works fine but the overall test fails.

Status: Needs review » Needs work

The last submitted patch, 6: add_migrate_support_for-2447727-6.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
73.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch add_migrate_support_for-2447727-8.patch. Unable to apply patch. See the log in the details link for more information. View

Didn't apply

Status: Needs review » Needs work

The last submitted patch, 8: add_migrate_support_for-2447727-8.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
628.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,845 pass(es), 36 fail(s), and 18 exception(s). View

c'mon

Status: Needs review » Needs work

The last submitted patch, 10: add_migrate_support_for-2447727-10.patch, failed testing.

benjy’s picture

@claudiu.cristea, just so you know, we don't manually manipulate the dump files anymore, see /core/scripts/migrate-dump-d6.sh

claudiu.cristea’s picture

@benjy, You maybe reviewed #5.

Ryan Weal’s picture

Not sure how related it is but I noted that taxonomy references are classified as "node reference" in D8, which sorta surprised me, but in any case, you may want to know about #2506001: Taxonomy term values referenced on nodes do not migrate (D6). Essentially everything comes through except the referenced values (the field is there, and all the taxonmy terms are there).

phenaproxima’s picture

Issue tags: +Barcelona2015

Not only would this be nice to have, it'd be reasonably easy. All that's needed is a couple of new cckfield plugins.

phenaproxima’s picture

Issue tags: +Needs tests
FileSize
102.22 KB

WIP patch -- adds test fields and values to the Drupal 6 database fixture, and the cckfield plugins to support node and user references.

singularo’s picture

+++ b/core/modules/entity_reference/src/Plugin/migrate/cckfield/ReferenceTrait.php
@@ -0,0 +1,32 @@
+        'target_id' => static::ENTITY_ID,

When trying this patch, I got an error:

PHP Fatal error: Undefined class constant 'ENTITY_ID' in /web/app/core/modules/entity_reference/src/Plugin/migrate/cckfield/ReferenceTrait.php on line 26

Changing this to:
'target_id' => static::entityId(),

Allowed things to continue, but not sure that is the correct thing to do.

webflo’s picture

Status: Needs work » Needs review
FileSize
72.6 KB

I tried to re-roll the patch.

webflo’s picture

phenaproxima’s picture

Status: Needs review » Needs work
+++ b/core/modules/field/migration_templates/d6_field.yml
--- /dev/null
+++ b/core/modules/migrate_drupal/src/Tests/Table/d6/ContentFieldNodeReference.php

This file is not needed -- drupal6.php is the database fixture and looks like it contains everything required for this patch to work.

Also still needs tests of the migrated reference fields. Other than that, looks great!

The last submitted patch, 18: 2447727-18.patch, failed testing.

The last submitted patch, 19: 2447727-19.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
11.65 KB
webflo’s picture

ER module is dead. Moved everything to Drupal\Core\Field\Plugin\migrate

Status: Needs review » Needs work

The last submitted patch, 24: 2447727-24.patch, failed testing.

The last submitted patch, 23: 2447727-23.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
19.63 KB
phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,38 @@
    +  protected function entityId() {
    +    return 'nid';
    +  }
    

    Needs @inheritdoc.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceTrait.php
    @@ -0,0 +1,32 @@
    +trait ReferenceTrait {
    +
    +  abstract protected function entityId();
    +
    

    entityId() needs a doc comment.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/TaxonomyTermReference.php
    @@ -0,0 +1,38 @@
    +  protected function entityId() {
    +    return 'tid';
    +  }
    

    @inheritdoc

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
    @@ -0,0 +1,38 @@
    +  protected function entityId() {
    +    return 'uid';
    +  }
    

    @inheritdoc

  5. +++ b/core/modules/field/migration_templates/d6_field.yml
    @@ -22,6 +22,10 @@ process:
    +        nodereference:
    +          nodereference_select: entity_reference
    +          nodereference_buttons: entity_reference
    +          nodereference_autocomplete: entity_reference
             number_integer:
               number: integer
               optionwidgets_select: list_integer
    @@ -37,6 +41,10 @@ process:
    
    @@ -37,6 +41,10 @@ process:
               optionwidgets_select: list_float
               optionwidgets_buttons: list_float
               optionwidgets_onoff: boolean
    +        userreference:
    +          userreference_select: entity_reference
    +          userreference_buttons: entity_reference
    +          userreference_autocomplete: entity_reference
    

    This static mapping is not necessary -- ReferenceTrait should implement getFieldType() and always return 'entity_reference'.

  6. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldSettings.php
    @@ -18,16 +22,65 @@
    +    $settings = $this->getSettings($field_type, $global_settings);
    

    Not sure what the point of keeping this is.

  7. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldSettings.php
    @@ -18,16 +22,65 @@
    +      // @todo: Implement transformFieldStorageSettings in every MigrateCckField
    +      // plugin.
    +      if ($plugin && method_exists($plugin, 'transformFieldStorageSettings')) {
    +        $settings = $plugin->transformFieldStorageSettings($value, $migrate_executable, $row, $destination_property);
    +      }
    

    The method_exists() check is not necessary because there is already a base implementation in CckFieldPluginBase :)

  8. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldSettings.php
    @@ -18,16 +22,65 @@
    +    catch (PluginNotFoundException $e) {
    +      // @todo: Nothing?
    +    }
    

    This @todo can be removed. In this case it's OK to return $global_settings unaltered.

  9. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
    @@ -89,4 +90,15 @@ public function processCckFieldValues(MigrationInterface $migration, $field_name
    +  public function transformFieldStorageSettings($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property);
    

    I'm not a huge fan of the way this method is named -- can it maybe be called getStorageSettings() or transformFieldSettings()?

    The parameters need to be documented.

    Also for consistency, let's lose the $migrate_executable and $destination_property parameters. Ideally, you can even remove the $value parameter and let the plugin read values directly out of $row.

The last submitted patch, 27: 2447727-27.patch, failed testing.

webflo’s picture

Addressed most of the changes from #28 but keep the old getSettings method to keep the patch size small. We can remove it in a follow up.

Status: Needs review » Needs work

The last submitted patch, 30: 2447727-30.patch, failed testing.

phenaproxima’s picture

A few minor things left, but overall this looks great! Nice work, @webflo.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceTrait.php
    @@ -0,0 +1,37 @@
    +  /**
    +   * Gets the name of the field property which holds the entity id.
    +   *
    +   * @return string
    +   */
    

    Nit: id should be ID. Fixable on commit.

  2. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldSettings.php
    @@ -18,16 +22,59 @@
    +    catch (PluginNotFoundException $e) {
    +      $settings = $this->getSettings($field_type, $global_settings);
    +    }
    +
    +    return $settings;
    

    Another nit -- why not just return the result of getSettings() in the catch block?

  3. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
    @@ -9,6 +9,7 @@
    +use Drupal\migrate\MigrateExecutableInterface;
    

    Is this import still needed?

The last submitted patch, 30: 2447727-30.patch, failed testing.

webflo’s picture

benjy’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceTrait.php
    @@ -0,0 +1,37 @@
    +trait ReferenceTrait {
    

    To be honest i see very little reason to have a trait here, what are we saving, 1 duplicate array definition? Also if we have to share code i'd rather add a MigrateFieldReferenceBase class.

  2. +++ b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
    @@ -89,4 +89,18 @@ public function processCckFieldValues(MigrationInterface $migration, $field_name
    +   * @see Drupal\field\Plugin\migrate\process\d6\FieldSettings::transform
    ...
    +  public function transformFieldStorageSettings(Row $row);
    

    We need to make sure this also fires for D7 field migrations.

webflo’s picture

FileSize
9.41 KB
37.08 KB

Added support for the field instance settings migration today.

Status: Needs review » Needs work

The last submitted patch, 36: 2447727-36.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
45.38 KB
9.89 KB
webflo’s picture

d6_field_instance migration depends now on d6_user_role

The last submitted patch, 38: 2447727-38.patch, failed testing.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,59 @@
    +class NodeReference extends CckFieldPluginBase {
    +
    +  use ReferenceTrait;
    

    As @benjy commented above, it might be better to simply have a base class for reference fields to extend, rather than using a trait.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,59 @@
    +  public function transformFieldInstanceSettings(Row $row) {
    

    Needs a doc comment or @inheritdoc.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,59 @@
    +    /**  @todo: Use d6_node_tyoe migration for mapping. */
    

    Typo in the migration ID. :) This should also use // syntax instead of /**/, and explain a little more about why the migration is needed.

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceTrait.php
    @@ -0,0 +1,37 @@
    +trait ReferenceTrait {
    

    Let's turn this into an abstract base class extending CckFieldPluginBase.

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
    @@ -0,0 +1,117 @@
    +    /**  @todo: Use d6_user_role migration for mapping. */
    

    Please switch to // syntax and explain why the migration is needed, just so the person who fulfills that todo knows what they're doing :)

  6. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
    @@ -0,0 +1,117 @@
    +    $migrationPlugin = \Drupal::service('plugin.manager.migrate.process')
    +      ->createInstance('migration', $migration_plugin_configuration, $migration);
    

    I'd rather that the process plugin manager were injected via ContainerFactoryPluginInterface.

  7. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/d6/FieldSettingsTest.php
    @@ -24,16 +25,22 @@ class FieldSettingsTest extends UnitTestCase {
    +    $cck_plugin_manager = $this->getMockBuilder(MigratePluginManager::class)
    +      ->disableOriginalConstructor()
    +      ->getMock();
    

    This is just a style thing, but you could get away with mocking PluginManagerInterface instead of the Migrate plugin manager.

    Also, it's preferable to use Prophecy for tests -- it's a bit more readable than PHPUnit's mocking system.

  8. +++ b/core/modules/file/src/Plugin/migrate/cckfield/FileField.php
    @@ -59,4 +59,64 @@ public function getFieldType(Row $row) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function transformFieldStorageSettings(Row $row) {
    +    return parent::transformFieldStorageSettings($row); // TODO: Change the autogenerated stub
    +  }
    

    This has no purpose. ;)

  9. +++ b/core/modules/file/src/Plugin/migrate/cckfield/FileField.php
    @@ -59,4 +59,64 @@ public function getFieldType(Row $row) {
    +   *   The size string, eg 10M
    

    Nit: Should be e.g.

  10. +++ b/core/modules/link/src/Plugin/migrate/cckfield/LinkField.php
    @@ -46,4 +47,24 @@ public function processCckFieldValues(MigrationInterface $migration, $field_name
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function transformFieldStorageSettings(Row $row) {
    +    return parent::transformFieldStorageSettings($row); // TODO: Change the autogenerated stub
    +  }
    

    Let's get rid of this stub.

  11. +++ b/core/modules/link/src/Plugin/migrate/cckfield/LinkField.php
    @@ -46,4 +47,24 @@ public function processCckFieldValues(MigrationInterface $migration, $field_name
    +    // $settings['url'] = $widget_settings['default_value'][0]['url'];
    

    Please remove this.

  12. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6TestBase.php
    @@ -90,6 +90,8 @@ protected function migrateFields() {
    +      'd6_filter_format'
    

    I believe that d6_filter_format is already executed by migrateContentTypes(), so there's no need to run it again.

webflo’s picture

Status: Needs work » Needs review
FileSize
47.51 KB
13.83 KB

I did not change the mocking in FieldSettingsTest yet because we rely on the PluginNotFoundException which is thrown by the MigratePluginManager.

webflo’s picture

Assigned: claudiu.cristea » webflo
FileSize
6.55 KB
53.42 KB

I created a new process plugin for the widget type conversion, because the widget conversion is needed in d6_field.yml and d6_field_instance_widget_settings.yml. I think we can remove MigrateCckFieldInterface::processFieldWidget.

webflo’s picture

FileSize
53.46 KB
mikeryan’s picture

#2612914: Images transfer but are not linked in content looks related to the general work here on handling entity references - this patch doesn't currently handle files, but given that it's extended to taxonomy terms perhaps we should take that step here as well (as opposed to fixing them in a follow-up)?

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Barcelona2015, -Needs tests

Looks good to me (forget my previous comment, addressing that in #2604484: Migrate Drupal 7 image and file fields).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 44: 2447727-44.patch, failed testing.

The last submitted patch, 44: 2447727-44.patch, failed testing.

webflo’s picture

The last submitted patch, 44: 2447727-44.patch, failed testing.

benjy’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,112 @@
    +  public function transformFieldStorageSettings(Row $row) {
    ...
    +  public function processFieldInstance(MigrationInterface $migration) {
    ...
    +  public function transformFieldInstanceSettings(Row $row) {
    

    We're already using the process prefix on other functions on this interface, shouldn't we keep inline with that and make these processFieldStorageSettings and processFieldInstanceSettings?

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
    @@ -0,0 +1,76 @@
    +   * @return string
    +   */
    

    Missing return comment.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
    @@ -0,0 +1,76 @@
    +  public function getFieldFormatterMap() {
    +    return array();
    +  }
    

    Why is this empty?

webflo’s picture

process* and transform* are two different things. process is called by the migration builder and transform is called by the migration process plugin.

benjy’s picture

process* and transform* are two different things. process is called by the migration builder and transform is called by the migration process plugin.

Sure, but we're still processing that data and we have process plugins that do the same thing, manipulate data during the migration. That said, process plugins have a method called "transform", happy to hear what others think?

KarenS’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,112 @@
    +class NodeReference extends ReferenceBase {
    

    Needs a 'use' statement for ReferenceBase.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/TaxonomyTermReference.php
    @@ -0,0 +1,48 @@
    +class TaxonomyTermReference extends ReferenceBase {
    

    Needs a 'use' statement for ReferenceBase.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
    @@ -0,0 +1,119 @@
    +class UserReference extends ReferenceBase {
    

    Needs a 'use' statement for ReferenceBase.

KarenS’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,112 @@
    +   * Look up migrated role IDs from the d6_node_type migration.
    

    Should be node types not role IDs.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,112 @@
    +   *   The source role IDs.
    

    Should be node types not role IDs.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,112 @@
    +   *   The migrated role IDs.
    

    Should be node types not role IDs.

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,112 @@
    +    // Configure the migration process plugin to look up migrated IDs from
    +    // the d6_user_role migration.
    

    Should be node types not role IDs.

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,112 @@
    +    $roles = [];
    +    foreach ($source_node_types as $role) {
    +      $roles[] = $migrationPlugin->transform($role, $executable, $row, NULL);
    +    }
    +    return array_combine($roles, $roles);
    +  }
    

    Should be node types not role IDs.

KarenS’s picture

It took me a while to realize this, but this code will only work to migrate D6 node and user references. It will not work for D7 node and user references because the field names, widgets, and formatters are named differently. I don't know what might be the best way to extend this code to work for both D6 and D7 style node and user references. It may be that we need totally different classes for each of them because of the differences, i.e. in D7 the original field is 'nodereference' or 'userreference', but in D7 it's 'node_reference' or 'user_reference'.

megastruktur’s picture

Hi guys! Sorry for a dumb question, but how can I test your patch? How do you test/use it?

I've got a d6 env with lots of noderef entities which I need to migrate to d8. Tried to write my own migration plugin but failed :(

anavarre’s picture

@megastruktur Easiest way is to make sure you run HEAD (latest dev version of Drupal) and download the patch you want to test against the Drupal codebase, e.g. https://www.drupal.org/files/issues/2447727-44.patch

See https://www.drupal.org/patch/apply - Here's an example of how to do it from within your Drupal codebase on Linux/Mac OSX: $ wget https://www.drupal.org/files/issues/<patchname>.patch && git apply -v <patchname>.patch

If you see only messages such as Applied patch /path/to/patched/file cleanly then things are okay and the patch is ready for you to test, review and report back here.

megastruktur’s picture

I've tested the 2447727-44 patch on fresh 8.1.x-dev version and it seems to work (everything is imported like a charm)! :)

Can I somehow update the previous migration? I mean, I've already migrated all necessary content and now I need to import noderef fields only. Is it possible?

Or will I have to
1. Remove nodes.
2. Clear migrate cache.
3. Make an import again.

*UPD*: please, tell me, is it essential to use dev version of Drupal? Or 8.0.1 is ok?

**UPD**:
1. It works for 8.0.1 stable release.
2. Cleaning content types, removing all fields and cleaning migrate tables from previously imported content didn't help: content types are imported again WITHOUT noderef fields.

***UPD***:
SOLUTION: rollback the migration and re-run it again. All fields are now in place.

pallavi_sugandhi’s picture

Thanks webflo

I applied the patch 2447727-44.patch on the latest Drupal 8 and then ran the migration process. It successfully imported the node reference fields into the D8 Instance

But still the Taxonomy term reference field is not mapped with the content. When I edit the node it shows -None- in the select list.

chx’s picture

Issue tags: +SprintWeekend2016
k4v’s picture

Updated the comments for #55, the change from #54 is not neccessary I think, the class is in the same package.

k4v’s picture

Status: Needs work » Needs review
k4v’s picture

Issue tags: +SprintWeekendBerlin

Status: Needs review » Needs work

The last submitted patch, 62: add_migrate_support_for-2447727-62.patch, failed testing.

k4v’s picture

k4v’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 66: add_migrate_support_for-2447727-66.patch, failed testing.

k4v’s picture

The order is relevant for the test and it was confusing to reuse $expected.

k4v’s picture

Status: Needs work » Needs review
benjy’s picture

Assigned: webflo » Unassigned

This looks great, has anyone tested with a real migration? I'd like to give it a test before we RTBC.

emptyvoid’s picture

I've setup a d6 site with real content, core and all modules are updated to the most recent releases. Running the migration, the fields and data are still ignored. I've applied the patch on a recent release of D8 and files were changed/added.

Perhaps I'm not going through the correct steps?

1. Rebuild manifest
drush migrate-upgrade
--legacy-root='/var/www/hostname/web'
--legacy-db-url='mysql://user@hostname/database'
--configure-only

2. Export to review
drush config-export

-- note none of the node reference fields were detected or written to the exports.

3. Run full migration
drush migrate-upgrade
--legacy-root='/var/www/hostname/web'
--legacy-db-url='mysql://user@hostname/database'

What additional information can I provide to help debug the integration?

webflo’s picture

I think migrate-upgrade skips existing migrations. Maybe you had some migrations already in place?

miscellainiac’s picture

We are working to migrate a small Drupal 6 site to Drupal 8. Using drush to rollback the migrations didn't help any, so we threw everything away and started from scratch again. Before migrating, we applied the latest patch linked here. All of our node references came through. For reference we have:
* Event (content) contains a Speaker link that is a node reference to a Person (content)
* Publication (content) contains an Authors link that is a node reference to a Person (content)
* Research (content) contains links to Member, Related Event, and Related Publications that are node references to a Person, Event, and Publication

infopicard’s picture

Hello,

I have the same requirement to migration a lot of CKK node references from D6->D8
but I'm not familar with patches ;-)
is it realistic that this migration option becomes release in one of the next drupal 8 releases (core or plugin)?

infopicard’s picture

Hi Benji,

what could I do to assist you. I have a big D6 website with a lot of node references in use.
My first migration was particular successfull excluding node reference field for example. (location fields also missed and some other issues, but this will be another battlefield)

Could I do any tests? Which environment should i use? I tested it with current D6 Version an D8.0.3
perhaps if I could get the code for this 8.0.3 (if even possible) I could check it - thx.

scottishlass’s picture

Status: Needs review » Active

Applying patch #69 failed for me on latest 8.0.x-dev. Does it need a re-roll?

Checking patch core/modules/field/src/Plugin/migrate/process/d6/FieldInstanceSettings.php...
error: while searching for:
$settings['prefix'] = $field_settings['prefix'];
$settings['suffix'] = $field_settings['suffix'];
...

scottishlass’s picture

Status: Active » Needs review

Apologies didn't mean to change status, setting back to Needs review

miscellainiac’s picture

Applying the patch to 8.0.4-dev fails; however if I pin Drupal to 8.0.3 then the patch is applied successfully.

infopicard’s picture

Ok Applying the patch to 8.0.3 was successfull

but update-migrate don't recognize my node reference fields

message: Source ID node,teaser,contenttype,field_1: Failed to lookup field type array ( 0 => 'nodereference', 1 => 'default', ) in the static map.

did I something wrong oder what info do you need to figure out the problem?

Edit: forget it. the node reference migrated successfully !!! THANKS!!!
I migrate aprox 5000 references in a couple of different fields. Perfect!

benjy’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

#74 and #80 both report this working as expected, looks like the patch needs a re-roll then RTBC from me.

k4v’s picture

k4v’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 82: add_migrate_support_for-2447727-82.patch, failed testing.

dimaro’s picture

Issue tags: -Needs reroll

The latest patch apply correctly.
Remove tag Needs reroll.

The last submitted patch, 82: add_migrate_support_for-2447727-82.patch, failed testing.

italiatina’s picture

Issue tags: +drupalconasia2016

Adding the tag drupalconasia2016 as we are working on this issue during the sprint.

The last submitted patch, 82: add_migrate_support_for-2447727-82.patch, failed testing.

The last submitted patch, 82: add_migrate_support_for-2447727-82.patch, failed testing.

The last submitted patch, 82: add_migrate_support_for-2447727-82.patch, failed testing.

The last submitted patch, 82: add_migrate_support_for-2447727-82.patch, failed testing.

dimaro’s picture

dimaro’s picture

Status: Needs work » Needs review

Apologies, update to run the test.

Status: Needs review » Needs work

The last submitted patch, 92: add_migrate_support_for-2447727-92.patch, failed testing.

quietone’s picture

Issue tags: +migrate-d6-d8
el1_1el’s picture

I've now used both 44 and 92 over the past 2 months. Both seem to be working fine for multiple nr and ur fields

benjy’s picture

We just need the two test fails investigating and then we can get this ready.

el1_1el’s picture

I got both the errors reported in 92 on git apply w core 8.0.4 but the patch applied cleanly after upgrading to 8.0.5

infopicard’s picture

I tested it successfully against 8.0.5 and a bigger D6 site with a lot of node references. It works fine for me!

quietone’s picture

User reference is enabled in the dump but Node reference is not. Surely, they should both be enabled.

cosmicdreams’s picture

@benjy: At the time of writing this comment, the testbot has retested this patch last on March 31st. That test run shows that all tests passed. I don't know why that isn't reflected in the main issue here, but if you click into the data on how the tests were last run you'd see that.

benjy’s picture

@cosmicdreams, looks pretty clear to me that two tests failed here: https://www.drupal.org/pift-ci-job/197704 Maybe you're not talking about the patch in #92?

I've re-tested the patch in #92 just to make sure.

Also, at this point we should probably be re-rolling this against 8.1

The last submitted patch, 92: add_migrate_support_for-2447727-92.patch, failed testing.

cr0ss’s picture

Hello,

I have D6 site with Blog node type referenced to Series node and D8.0.5 with applied #92 patch.

Steps I took:
1. Applied patch #92
2. drush cr all
3. drush migrate-upgrade --legacy-db-url=mysql://user:pass@localhost/d6 --configure-only
4. I got field_series: field_series in migrate.migration.d6_node__blog.yml
5. drush migrate-import d6_node__series
6. drush migrate-import d6_node__blog

None of references were created. What I do wrong?

malcomio’s picture

The patch in #92 applied cleanly against the current 8.0-x branch, on a fresh installation of the standard profile, and node and taxonomy term references were successfully migrated, after following these steps:

  1. Apply the patch in #92
  2. drush si standard
  3. drush en migrate_plus migrate_tools migrate_upgrade
  4. drush migrate-upgrade --configure-only --legacy-db-url=mysqli://root:root@localhost/d6 --legacy-root=http://drupal6.dev --legacy-db-prefix=drupal_
  5. drush mi --all

However, there were several problems with the migration - most notably that image fields were not migrated (which they are without the patch applied)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

malcomio’s picture

Looks like the patch needs significant changes to work with 8.1.x - was hoping to be able to re-roll it, but I think it's not a small job :(

cr0ss’s picture

Hey @malcomio, would you mind posting how the migrate.migration.d6_node__[node_type].yml that has node references field looks for you?

I'm still having a problem with migrating node_references, just gave it a try on a latest 8.0.x-dev version.

kgoel’s picture

Status: Needs work » Needs review
FileSize
59.88 KB

Rerolled.

Looking into fails.

Status: Needs review » Needs work

The last submitted patch, 109: 2447727-109.patch, failed testing.

vprocessor’s picture

Assigned: Unassigned » vprocessor
kgoel’s picture

Status: Needs work » Needs review
FileSize
60.67 KB
2.92 KB

Fails in #109 were mainly because of the change introduced in https://www.drupal.org/node/2676222

vprocessor’s picture

Hi guys, patch is ready

Status: Needs review » Needs work

The last submitted patch, 113: add_migrate_support_for-2447727-113.patch, failed testing.

The last submitted patch, 112: 2447727-112.patch, failed testing.

vprocessor’s picture

Assigned: Unassigned » vprocessor
vprocessor’s picture

removed duplicated interfaces

Status: Needs review » Needs work

The last submitted patch, 117: add_migrate_support_for-2447727-117.patch, failed testing.

malcomio’s picture

Status: Needs work » Needs review
FileSize
54.46 KB
2.68 KB

Applying the patch against 8.1.x, I get the following error when trying to run drush migrate-upgrade --configure-only

Error: Declaration of Drupal\Core\Field\Plugin\migrate\cckfield\ReferenceBase::processCckFieldValues() must be compatible with
Drupal\migrate_drupal\Plugin\MigrateCckFieldInterface::processCckFieldValues(Drupal\migrate\Plugin\MigrationInterface $migration, $field_name, $data) in
/var/www/drupalvm/drupal/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php, line 19

Seems to be happening because the Migration class is in the wrong namespace. I've re-rolled the patch, although I still need to test it more.

Status: Needs review » Needs work

The last submitted patch, 119: 2447727-119.patch, failed testing.

malcomio’s picture

With the patch applied, my migrations are falling over with an error that suggests that something is going wrong in the inheritance somewhere, and the D7 Vocabulary migration plugin is being used instead of the D6 one for some reason.

exception 'PDOException' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd6.drupal_taxonomy_vocabulary' doesn't exist' in                      [error]
/var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Statement.php:59
Stack trace:
#0 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Statement.php(59): PDOStatement->execute(Array)
#1 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Connection.php(610): Drupal\Core\Database\Statement->execute(Array, Array)
#2 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php(81): Drupal\Core\Database\Connection->query('SELECT COUNT(*)...', Array, Array)
#3 /var/www/drupalvm/drupal/core/lib/Drupal/Core/Database/Query/Select.php(476): Drupal\Core\Database\Driver\mysql\Connection->query('SELECT COUNT(*)...', Array, Array)
#4 /var/www/drupalvm/drupal/core/modules/migrate/src/Plugin/migrate/source/SqlBase.php(242): Drupal\Core\Database\Query\Select->execute()
#5 /var/www/drupalvm/drupal/core/modules/migrate/src/Plugin/Migration.php(561): Drupal\migrate\Plugin\migrate\source\SqlBase->count()
#6 /var/www/drupalvm/drupal/core/modules/migrate/src/Plugin/Migration.php(489): Drupal\migrate\Plugin\Migration->allRowsProcessed()
#7 /var/www/drupalvm/drupal/core/modules/migrate/src/MigrateExecutable.php(184): Drupal\migrate\Plugin\Migration->checkRequirements()
#8 [internal function]: Drupal\migrate\MigrateExecutable->import()
#9 /usr/local/share/drush/includes/drush.inc(720): call_user_func_array(Array, Array)
#10 /usr/local/share/drush/includes/drush.inc(711): drush_call_user_func_array(Array, Array)
#11 /var/www/drupalvm/drupal/modules/contrib/migrate_tools/migrate_tools.drush.inc(269): drush_op(Array)
#12 [internal function]: _drush_migrate_tools_execute_migration(Object(Drupal\migrate\Plugin\Migration), 'd7_taxonomy_ter...', Array)
#13 /var/www/drupalvm/drupal/modules/contrib/migrate_tools/migrate_tools.drush.inc(235): array_walk(Array, '_drush_migrate_...', Array)
#14 [internal function]: drush_migrate_tools_migrate_import()
#15 /usr/local/share/drush/includes/command.inc(366): call_user_func_array('drush_migrate_t...', Array)
#16 /usr/local/share/drush/includes/command.inc(217): _drush_invoke_hooks(Array, Array)
#17 [internal function]: drush_command()
#18 /usr/local/share/drush/includes/command.inc(185): call_user_func_array('drush_command', Array)
#19 /usr/local/share/drush/lib/Drush/Boot/BaseBoot.php(72): drush_dispatch(Array)
#20 /usr/local/share/drush/includes/preflight.inc(68): Drush\Boot\BaseBoot->bootstrap_and_dispatch()
#21 /usr/local/share/drush/drush.php(12): drush_main()
#22 {main}
webflo’s picture

kgoel’s picture

webflo’s picture

The last submitted patch, 122: 2447727-122.patch, failed testing.

The last submitted patch, 123: 2447727-123.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 124: 2447727-124.patch, failed testing.

malcomio’s picture

Good news is the patch seems to work for me on a clean install of 8.1.x.

Now for the testbot failures...

infopicard’s picture

I tested it successful too

  • clean install of 8.1.0
  • migrate a complex D6 Site with many node references - everything seems to be ok
webflo’s picture

Status: Needs work » Needs review
FileSize
927 bytes
62.64 KB

Field and Field Instance Migrations depends on d6_user_role and filter format, because user_reference fields user role related settings.

Status: Needs review » Needs work

The last submitted patch, 130: 2447727-130.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
63.29 KB
1.47 KB

E-Mail Widget got size attribute in #2578741: Add setting for size to email widget

webflo’s picture

The last submitted patch, 132: 2447727-132.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 133: 2447727-133.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
751 bytes
64.01 KB

Maybe we should sort the array or make the assertions smaller.

chithra_zyxware’s picture

Hi,

I tested a d6 migration with node and user references after applying the patch 2447727-135. It is working.

vprocessor’s picture

Assigned: vprocessor » Unassigned
quietone’s picture

A bit of a review. I focused on code sniffer because I've just set it up in PhpStorm.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,122 @@
    +use Drupal\migrate\Plugin\Migration;
    

    Unused.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,122 @@
    +    $migration_dependencies = $migration->get('migration_dependencies');
    ...
    +    $migration->set('migration_dependencies', $migration_dependencies);
    

    Where are get and set defined? PhpStorm can't find them.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
    @@ -0,0 +1,129 @@
    +use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
    ...
    +use Drupal\migrate\Plugin\Migration;
    ...
    +use Drupal\migrate\Plugin\MigratePluginManager;
    ...
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    

    Unused.

  4. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldWidgetSettingsTest.php
    @@ -30,28 +30,40 @@ public function testWidgetSettings() {
    +    $expected = [];
    

    Expected hasn't been used yet, so this is not strictly necessary.

  5. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldWidgetSettingsTest.php
    @@ -30,28 +30,40 @@ public function testWidgetSettings() {
         $expected = array('weight' => 1, 'type' => 'text_textfield');
    

    For consistency with the rest of the file, use short array syntax, [].

  6. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/d6/FieldSettingsTest.php
    @@ -2,9 +2,11 @@
     use Drupal\migrate\Plugin\MigrationInterface;
    ...
     use Drupal\Tests\UnitTestCase;
    

    Unused.

  7. +++ b/core/modules/link/src/Plugin/migrate/cckfield/LinkField.php
    @@ -34,11 +35,23 @@ public function getFieldFormatterMap() {
    diff --git a/core/modules/migrate/migrate.api.php b/core/modules/migrate/migrate.api.php
    
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/cckfield/CckFieldPluginBase.php
    @@ -4,6 +4,7 @@
    +use Drupal\migrate\MigrateExecutableInterface;
    

    Not used.

  8. Should nodereference be enabled in the test fixture?
    1. Should @file be removed in this issue or should that be in another issue?
    miscellainiac’s picture

    Hello all,

    We're trying to migrate once again from a D6 site to a D8 site, and surprised to find that this patch hasn't been rolled into a core release yet...

    Regardless, applying the patch is fine, but when we try to migrate the migration just dies with the following:

    Upgrading d6_field_instance
    Passed variable is not an array or object, using empty array instead 

    This is not helpful at all. We don't know what field is failing to migrate, if we knew that, we could remove the problematic fields and do some post-migration stuff to move that data over.

    Ryan Weal’s picture

    @miscellainiac were you getting this error before applying the patch? I suspect it is an unrelated issue, but it could also be related to the field settings if migrate isn't handling it correctly. If you are able to find out what field is failing can you post it here, or open a new issue if it is unrelated?

    miscellainiac’s picture

    @ryan-weal I should have put that in the post; yes without the patch we don't see this error. What's more, we've tried with an older version of Drupal Core (8.0.3) and the matching patch (also in this thread) and a compatible version of the migrate_upgrade module. Same error.

    I was hoping that someone might know where we can inject some code to dump out what field the module is trying to migrate. For example, I added some print_r statements to the code (in the patch) that processes a node reference so I can see what content type it chokes on. So we removed that content type, my print_r statement is never reached, but the migration still dies with the same error.

    At this point I have to binary-search slash-n-burn our content types and fields to figure out something that should just be dumped out to the console during the migration anyways...

    miscellainiac’s picture

    For some more information, I was able to narrow down the content type with the problem. With have a content type of Person - that content type has a User Reference field in it. If I remove that field from the content type, everything seems to migrate just fine. However, we performed a migration back with Drupal Core 8.0.3 and the patch here (different site though) and the User Reference field came over as expected...

    Ryan Weal’s picture

    Status: Needs review » Needs work

    Sounds like you found a bug. It would probably be worth asking for help in #drupal-migrate on IRC. If you are at the sprint in New Orleans tomorrow I'm sure there will be a migrate table.

    miscellainiac’s picture

    I didn't have much luck in the IRC channel last time we ran into migration issues, so I'm not inclined to spend any time there now. Could someone let me know where I can report the bug? It's very possible that the data is just bad in our case, but I was hoping for a more graceful failure scenario and some debug output so a developer can troubleshoot...

    quietone’s picture

    @miscellainiac, This may help with reporting the issue, How to create a good issue.

    tlyngej’s picture

    Re-roll the patch to the current 8.1.x code base.

    quietone’s picture

    Status: Needs work » Needs review

    Setting to NR to test the patch in #147.

    claudiu.cristea’s picture

    +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
    @@ -0,0 +1,122 @@
    +/**
    + * @file
    + * Contains \Drupal\Core\Field\Plugin\migrate\cckfield\NodeReference.
    + */
    +
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
    @@ -0,0 +1,76 @@
    +/**
    + * @file
    + * Contains \Drupal\Core\Field\Plugin\migrate\cckfield\ReferenceBase.
    + */
    +
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/TaxonomyTermReference.php
    @@ -0,0 +1,48 @@
    +/**
    + * @file
    + * Contains \Drupal\Core\Field\Plugin\migrate\cckfield\TaxonomyTermReference.
    + */
    +
    
    +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
    @@ -0,0 +1,129 @@
    +/**
    + * @file
    + * Contains \Drupal\Core\Field\Plugin\migrate\cckfield\UserReference.
    + */
    +
    
    +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldWidgetType.php
    @@ -0,0 +1,101 @@
    +/**
    + * @file
    + * Contains \Drupal\field\Plugin\migrate\process\d6\FieldWidgetType.
    + */
    +
    

    The @file statement should not be added anymore to classes with namespace definition.

    Status: Needs review » Needs work

    The last submitted patch, 147: 2447727-147.patch, failed testing.

    claudiu.cristea’s picture

    Patch doesn't apply anymore.

    dimaro’s picture

    Status: Needs work » Needs review
    Issue tags: -Needs reroll
    FileSize
    64.11 KB

    Rerolled #136.
    Hope it works!

    Status: Needs review » Needs work

    The last submitted patch, 152: add_migrate_support_for-2447727-152.patch, failed testing.

    webflo’s picture

    Assigned: Unassigned » webflo
    Status: Needs work » Needs review
    Issue tags: +DevDaysMilan
    FileSize
    62.32 KB
    webflo’s picture

    The last submitted patch, 154: 2447727-154.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 155: 2447727-155.patch, failed testing.

    webflo’s picture

    Status: Needs work » Needs review
    FileSize
    2.34 KB
    63.15 KB

    cckPluginManager needs the core version

    Status: Needs review » Needs work

    The last submitted patch, 158: 2447727-155.patch, failed testing.

    webflo’s picture

    Status: Needs work » Needs review
    FileSize
    63.19 KB

    Uploaded the wrong patch yesterday.

    Status: Needs review » Needs work

    The last submitted patch, 160: 2447727-160.patch, failed testing.

    webflo’s picture

    Status: Needs work » Needs review
    FileSize
    63.85 KB

    Updated the field_config and field_storage_config entity counts in MigrateUpgrade6Test::getEntityCounts

    chipway’s picture

    Sorry @webflo,
    I have not the right D6/D7 website copies to test it here in Milano and it took me a long time to read it, but Patch 2447727-162.patch applied successfully.
    As this patch seems to solve the issue, I would hope it to be committed asap to reduce review complexity and time.

    I will come back in a few weeks to try to go further in reviewing your great work.
    Thks

    webflo’s picture

    The last submitted patch, 164: 2447727-164.patch, failed testing.

    webflo’s picture

    +++ b/core/modules/link/src/Plugin/migrate/cckfield/LinkField.php
    @@ -38,11 +39,23 @@ public function getFieldFormatterMap() {
    diff --git a/core/modules/migrate/migrate.api.php.rej b/core/modules/migrate/migrate.api.php.rej
    
    diff --git a/core/modules/migrate/migrate.api.php.rej b/core/modules/migrate/migrate.api.php.rej
    new file mode 100644
    
    new file mode 100644
    index 0000000..64fc422
    
    index 0000000..64fc422
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/migrate/migrate.api.php.rej
    
    +++ b/core/modules/migrate/migrate.api.php.rej
    +++ b/core/modules/migrate/migrate.api.php.rej
    @@ -0,0 +1,10 @@
    
    @@ -0,0 +1,10 @@
    +diff a/core/modules/migrate/migrate.api.php b/core/modules/migrate/migrate.api.php	(rejected hunks)
    +@@ -77,7 +77,7 @@
    +  * The definition of how to migrate each type of data is stored in configuration
    +  * entities. The migration configuration entity class is
    +  * \Drupal\migrate\Entity\Migration, with interface
    +- * \Drupal\migrate\Entity\MigrationInterface; the configuration schema can be
    ++ * \Drupal\migrate\Plugin\MigrationInterface; the configuration schema can be
    +  * found in the migrate.schema.yml file. Migration configuration consists of IDs
    +  * and configuration for the source, process, and destination plugins, as well
    +  * as information on dependencies. Process configuration consists of sections,
    diff --git a/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php b/core/modules/migrate_drupal/src/Plugin/MigrateCckFieldInterface.php
    

    Added a wrong file to the patch.

    Status: Needs review » Needs work

    The last submitted patch, 167: 2447727-167.patch, failed testing.

    mikeryan’s picture

    Status: Needs work » Needs review

    Retesting, failure looks unconnected.

    Status: Needs review » Needs work

    The last submitted patch, 167: 2447727-167.patch, failed testing.

    quietone’s picture

    Status: Needs work » Needs review

    Passing test now, back to NR.

    phenaproxima’s picture

    Status: Needs review » Needs work

    This is an epic patch. Thanks for your hard work on this, @webflo et. al!

    1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
      @@ -0,0 +1,118 @@
      + *   core = {6},
      

      I'm pretty sure that Node Reference existed for D7 as well.

    2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
      @@ -0,0 +1,118 @@
      +  /**
      +   * @var string
      +   */
      

      Needs a description.

    3. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
      @@ -0,0 +1,118 @@
      +  protected $nodeTypeMigration = 'd6_node_type';
      

      Should this be a constant?

    4. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
      @@ -0,0 +1,118 @@
      +    $migration = \Drupal::service('plugin.manager.migration')->createStubMigration([]);
      

      Ideally this should be injected.

    5. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
      @@ -0,0 +1,71 @@
      +  /**
      +   * The migrate plugin manager, configured for lookups in d6_user_roles.
      +   *
      +   * @var \Drupal\migrate\Plugin\MigratePluginManager
      +   */
      +  protected $migratePluginManager;
      

      Not sure this doc comment is accurate. According to create(), this is the process plugin manager.

    6. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
      @@ -0,0 +1,71 @@
      +  /**
      +   * Gets the name of the field property which holds the entity ID.
      +   *
      +   * @return string
      +   */
      +  abstract protected function entityId();
      

      IMHO this should be part of the plugin definition, rather than a method. Not a big deal though.

    7. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
      @@ -0,0 +1,71 @@
      +
      +  /**
      +   * {@inheritdoc}
      +   */
      +  public function getFieldFormatterMap() {
      +    return array();
      +  }
      

      Isn't this already implemented in CckFieldPluginBase?

    8. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/TaxonomyTermReference.php
      @@ -0,0 +1,49 @@
      + *   core = {6,7},
      

      taxonomy_term_reference fields didn't exist in D6, to my knowledge.

    9. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/TaxonomyTermReference.php
      @@ -0,0 +1,49 @@
      +class TaxonomyTermReference extends ReferenceBase {
      

      This hasn't got any handler settings? (Valid vocabularies that can be referenced, etc.)

    10. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
      @@ -0,0 +1,125 @@
      + *   core = {6},
      

      Wasn't User Reference also available for D7?

    11. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
      @@ -0,0 +1,125 @@
      +  /**
      +   * @var string
      +   */
      +  protected $userRoleMigration = 'd6_user_role';
      

      Same as in NodeReference -- can/should this be a constant? Needs a description either way.

    12. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
      @@ -0,0 +1,125 @@
      +  protected function migrateUserRoles($source_roles) {
      

      This is very similar to NodeReference::migrateNodeTypes(). Can it be made into a generic method of ReferenceBase?

    13. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldInstanceSettings.php
      @@ -2,16 +2,56 @@
        *   id = "d6_field_field_settings"
        * )
        */
      -class FieldInstanceSettings extends ProcessPluginBase {
      +class FieldInstanceSettings extends ProcessPluginBase implements ContainerFactoryPluginInterface {
      

      Why is the plugin ID field_field_settings but the class name is FieldInstanceSettings?

    14. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldInstanceSettings.php
      @@ -19,6 +59,14 @@ class FieldInstanceSettings extends ProcessPluginBase {
      +    catch (PluginNotFoundException $e) {
      +
      +    }
      

      There's an unnecessary empty line here.

    15. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldSettings.php
      @@ -13,16 +17,57 @@
      +      return $this->cckPluginManager->createInstance($source_field_type, ['core' => 6])
      

      The plugin ID doesn't have a d6_ prefix, so why is the core version hard-coded to 6?

    16. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldWidgetType.php
      @@ -0,0 +1,96 @@
      +      return $this->cckPluginManager->createInstance($source_field_type, ['core' => 6])
      

      Why is the core version hard-coded to 6 if the plugin ID doesn't have the d6_ prefix?

    17. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldInstanceTest.php
      @@ -96,6 +96,55 @@ public function testFieldInstanceMigration() {
      +    $this->assertIdentical('Node reference', $field->label());
      +    $this->assertIdentical('default:node', $field->getSetting('handler'));
      +    $expected = [
      +      'target_bundles' => []
      +    ];
      +    $this->assertIdentical($expected, $field->getSetting('handler_settings'));
      

      Let's use assertSame() rather than the deprecated assertIdentical().

    18. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldInstanceTest.php
      @@ -96,6 +96,55 @@ public function testFieldInstanceMigration() {
      +    // Test node reference to entity reference migration.
      

      s/node/user

    19. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldInstanceTest.php
      @@ -96,6 +96,55 @@ public function testFieldInstanceMigration() {
      +    // Test node reference to entity reference migration.
      

      Same here.

    20. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldTest.php
      @@ -90,6 +90,16 @@ public function testFields() {
      +    $this->assertIdentical('entity_reference', $field_storage->getType());
      +    $this->assertIdentical('node', $field_storage->getSetting('target_type'));
      

      assertSame() instead of assertIdentical().

    21. +++ b/core/modules/field/tests/src/Unit/Plugin/migrate/process/d6/FieldSettingsTest.php
      @@ -19,16 +21,22 @@ class FieldSettingsTest extends UnitTestCase {
      +    $cck_plugin_manager = $this->getMockBuilder(MigratePluginManager::class)
      +      ->disableOriginalConstructor()
      +      ->getMock();
      

      Can we use Prophecy here?

    22. +++ b/core/modules/file/src/Plugin/migrate/cckfield/d6/FileField.php
      @@ -55,4 +55,61 @@ public function getFieldType(Row $row) {
      +  /**
      +   * {@inheritdoc}
      +   */
      +
      +  public function transformFieldInstanceSettings(Row $row) {
      

      There's an errant extra blank line after the doc comment. :)

    23. +++ b/core/modules/file/src/Plugin/migrate/cckfield/d6/FileField.php
      @@ -55,4 +55,61 @@ public function getFieldType(Row $row) {
      +    switch ($widget_type) {
      

      Let's add a default case, for completeness' sake.

    24. +++ b/core/modules/file/src/Plugin/migrate/cckfield/d6/FileField.php
      @@ -55,4 +55,61 @@ public function getFieldType(Row $row) {
      +  /**
      +   * Convert file size strings into their D8 format.
      +   *
      +   * D6 stores file size using a "K" for kilobytes and "M" for megabytes where
      +   * as D8 uses "KB" and "MB" respectively.
      +   *
      +   * @param string $size_string
      +   *   The size string, e.g. 10M
      +   *
      +   * @return string
      +   *   The D8 version of the size string.
      +   */
      +  protected function convertSizeUnit($size_string) {
      +    $size_unit = substr($size_string, strlen($size_string) - 1);
      +    if ($size_unit == "M" || $size_unit == "K") {
      +      return $size_string . "B";
      +    }
      +    return $size_string;
      +  }
      

      Not a big fan of the copy-paste with the D7 version of this class. Should this be moved into a base class?

    25. +++ b/core/modules/file/src/Plugin/migrate/cckfield/d7/FileField.php
      @@ -60,4 +60,57 @@ public function getFieldType(Row $row) {
      +    switch ($widget_type) {
      

      Missing default case.

    benstjohn’s picture

    Patching is failing using fresh drupal 8.1.8. This is rejected file:

    ***************
    *** 41,48 ****
            'comment_type' => 2,
            'contact_form' => 5,
            'editor' => 2,
    -      'field_config' => 62,
    -      'field_storage_config' => 43,
            'file' => 7,
            'filter_format' => 7,
            'image_style' => 5,
    --- 41,48 ----
            'comment_type' => 2,
            'contact_form' => 5,
            'editor' => 2,
    +      'field_config' => 66,
    +      'field_storage_config' => 47,
            'file' => 7,
            'filter_format' => 7,
            'image_style' => 5,
    

    It looks to my like the original file: MigrateUpgrade6Test.php has its field_config value set to 63, not 62, tweaking that in the patch fixes it although I'm not really sure what the implications of that are? Can someone weigh in?

    phenaproxima’s picture

    Version: 8.1.x-dev » 8.2.x-dev
    Issue tags: +Needs reroll

    Sounds like this needs a reroll against 8.2.x.

    The last submitted patch, 167: 2447727-167.patch, failed testing.

    hussainweb’s picture

    Status: Needs work » Needs review
    Issue tags: -Needs reroll
    FileSize
    63.06 KB

    Rerolled. Once tests pass, this should be back to needs work for #172.

    hussainweb’s picture

    Status: Needs review » Needs work

    Needs work as per #172.

    hussainweb’s picture

    Status: Needs work » Needs review
    FileSize
    14.53 KB
    61.85 KB

    #172,

    1, 10: I'd suggest to focus just on D6 here.
    2, 4, 5: Done
    3, 11: added descriptions
    7: No, this is not defined in the base class. It has to be implemented by concrete classes.
    8: This is copied from the already existing TaxonomyTermReference plugin. But you're right. I am thinking we should deal with that in a follow-up.
    12: Done, but I think we can improve the naming of the method.
    13: It's not changed here. I think the reason is that instances were just called fields in D6?
    14: Fixed
    15: Actually, this plugin is only for D6. There is a "d7_field_settings" for D7.
    16: See 15.
    17, 20: There are lots of other assertIdentical in the test. I think we should do this in a follow-up. I recall you raised this in another issue as well. We could combine this for all of them.
    18, 19: Fixed
    22: Fixed
    23, 25: What should the default case do?
    24: The only common base class here is CckFieldPluginBase and it doesn't make sense there. Could we inherit the D7 version from D6 like we do elsewhere?

    This patch adds a lot of features to ReferenceBase for field migrations. I think some of these would make sense for D7 as well. Follow-up?

    I also realize I skipped a couple of points I was not very clear on. There were lot of changes already, so we could look at them again after tests and review.

    phenaproxima’s picture

    8: This is copied from the already existing TaxonomyTermReference plugin. But you're right. I am thinking we should deal with that in a follow-up.

    I'm not sure I agree that it should be in a follow-up, because we would be committing something that is flat-out incorrect. At the very least, let's remove 6 from the plugin's core array.

    13: It's not changed here. I think the reason is that instances were just called fields in D6?

    Yeah, it's weird. I'm not TOO bent out of shape about it, but ideally I'd like to fix it ASAP.

    17, 20: There are lots of other assertIdentical in the test. I think we should do this in a follow-up. I recall you raised this in another issue as well. We could combine this for all of them.

    Okay, fair enough. I agree and will stop raising this issue until I open a follow-up :)

    23, 25: What should the default case do?

    Explicitly return NULL or FALSE so that migrations can skip on empty.

    24: The only common base class here is CckFieldPluginBase and it doesn't make sense there. Could we inherit the D7 version from D6 like we do elsewhere?

    If the D6 and D7 versions are very similar, I'm open to that.

    phenaproxima’s picture

    Status: Needs review » Needs work
    1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
      @@ -21,6 +17,8 @@
      +   * The plugin id for the node type migration.
      

      s/id/ID

    2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
      @@ -51,21 +63,57 @@ public static function create(ContainerInterface $container, array $configuratio
      +   * Look up migrated role IDs from the d6_user_role migration.
      

      The migration ID is not hard-coded.

    3. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
      @@ -51,21 +63,57 @@ public static function create(ContainerInterface $container, array $configuratio
      +   *   The migration id which migrated the source.
      

      s/id/ID

    4. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
      @@ -51,21 +63,57 @@ public static function create(ContainerInterface $container, array $configuratio
      +  protected function lookupMigrations($migration_id, $source_ids) {
      

      $source_ids should be type hinted.

    5. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
      @@ -51,21 +63,57 @@ public static function create(ContainerInterface $container, array $configuratio
      +    // the d6_user_role migration.
      

      Shouldn't say d6_user_role.

    6. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
      @@ -24,6 +18,8 @@
      +   * The plugin id for the user role migration.
      

      s/id/ID

    7. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
      @@ -14,18 +18,25 @@
      +   * The migration plugin manager.
      +   * @var \Drupal\migrate\Plugin\MigrationPluginManagerInterface
      

      Needs a blank line between the description and @var.

    The last submitted patch, 178: 2447727-178.patch, failed testing.

    hussainweb’s picture

    Status: Needs work » Needs review
    FileSize
    61.51 KB

    Rerolling first

    phenaproxima’s picture

    Status: Needs review » Needs work

    The reroll seems legit, but I'd like the stuff in #180 addressed before RTBC.

    keithm’s picture

    FileSize
    61.5 KB

    Edited #182 to address #180 open items.

    keithm’s picture

    Status: Needs work » Needs review
    phenaproxima’s picture

    Status: Needs review » Needs work

    I'm really sorry to do this.

    1. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/NodeReference.php
      @@ -0,0 +1,78 @@
      +    $node_types = array_filter($source_settings['referenceable_types']);
      

      We're assuming here that $source_settings['referenceable_types'] exists -- and it should, but you never know. The plugin should play defense here and simply return $settings as-is if 'referenceable_types' isn't set in $source_settings.

    2. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/ReferenceBase.php
      @@ -0,0 +1,120 @@
      +   * Look up migrated role IDs from a migration.
      

      It's not just role IDs were looking up here. I think this method should be renamed to more accurately reflect what it does -- perhaps, lookupMigratedBundles() or similar, and the doc comment must be fixed to match.

    3. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/TaxonomyTermReference.php
      @@ -0,0 +1,44 @@
      + *   core = {6,7},
      

      Taxonomy term reference fields did not exist in D6. I'd rather we just said {7} here, because this is the sort of thing that will fly under the radar and potentially cause weird bugs far down the line. Let's nip them in the bud.

    4. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
      @@ -0,0 +1,83 @@
      +    $roles = array_filter($source_settings['referenceable_roles']);
      

      Let's be defensive here as well, in case some weirdo source database doesn't give us $source_settings['referenceable_types'].

    5. +++ b/core/lib/Drupal/Core/Field/Plugin/migrate/cckfield/UserReference.php
      --- a/core/modules/field/migration_templates/d6_field.yml
      +++ b/core/modules/field/migration_templates/d6_field.yml
      
      +++ b/core/modules/field/migration_templates/d6_field.yml
      +++ b/core/modules/field/migration_templates/d6_field.yml
      @@ -126,6 +126,6 @@ process:
      
      @@ -126,6 +126,6 @@ process:
           source:
             - '@type'
             - global_settings
      -
      +      - type
      

      What is the nature of this change? What will this accomplish?

    6. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldInstanceSettings.php
      @@ -19,6 +59,13 @@ class FieldInstanceSettings extends ProcessPluginBase {
      +      return $this->cckPluginManager->createInstance($row->getSourceProperty('type'), ['core' => 6])
      

      I don't like the fact that ['core' => 6] is hardcoded here. Can this instead be a configuration value for this dispatch plugin?

      I'd also prefer if the value for type was passed in as $value, rather than pulled directly out of the row.

    7. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldWidgetType.php
      @@ -0,0 +1,96 @@
      +    $source_widget_type = $row->getSourceProperty('widget_type');
      +    $source_field_type = $row->getSourceProperty('type');
      

      These should not be pulled directly out of $row -- they should be passed in as $value.

    8. +++ b/core/modules/field/src/Plugin/migrate/process/d6/FieldWidgetType.php
      @@ -0,0 +1,96 @@
      +      return $this->cckPluginManager->createInstance($source_field_type, ['core' => 6])
      

      Again, I'd rather the core version were passed in with plugin configuration rather than hard-coded.

    9. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldInstanceTest.php
      @@ -96,6 +96,55 @@ public function testFieldInstanceMigration() {
      +    $this->assertIdentical('User reference', $field->label());
      +    $this->assertIdentical('default:user', $field->getSetting('handler'));
      

      assertIdentical() is deprecated -- PHPUnit uses assertSame(). I know the tests are still using the old method, but for the future's sake let's use assertSame() now and change all the old assertions in a follow-up.

    10. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldWidgetSettingsTest.php
      @@ -62,38 +73,54 @@ public function testWidgetSettings() {
      +    $expected['settings'] = array();
      

      Why array()? We should use consistent PHP 5.4+ syntax. Please change all of these to [].

    11. +++ b/core/modules/link/src/Plugin/migrate/cckfield/LinkField.php
      @@ -45,4 +46,16 @@ public function processCckFieldValues(MigrationInterface $migration, $field_name
      +    $map = ['disabled' => 0, 'optional' => 1, 'required' => 2];
      +    $settings['title'] = $map[$field_settings['title']];
      

      Let's do a check to ensure that $field_settings['title'] is actually set.

    iMiksu’s picture

    Assigned: webflo » iMiksu

    I'll work on some of the items now.

    iMiksu’s picture

    Assigned: iMiksu » Unassigned
    FileSize
    6.23 KB
    61.87 KB

    Finished working on comment #186 items 1, 3, 4, 9, 10 and 11.

    quietone’s picture

    Status: Needs work » Needs review

    Lets test patch #188

    phenaproxima’s picture

    +++ b/core/modules/link/src/Plugin/migrate/cckfield/LinkField.php
    @@ -51,10 +51,17 @@ public function processCckFieldValues(MigrationInterface $migration, $field_name
    +      $map = ['disabled' => 0, 'optional' => 1, 'required' => 2];
    +      $settings['title'] = $map[$field_settings['title']];
    +    }
    +    else {
    +      // In case we are missing title in field settings, use disabled value "0"
    +      // as a default value.
    +      $settings['title'] = 0;
    

    It's not obvious, but these are actually constants defined by core, so I'd prefer to use the actual constants rather than hard-code the integer equivalents. 0 = DRUPAL_DISABLED, 1 = DRUPAL_OPTIONAL, and 2 = DRUPAL_REQUIRED.

    dimaro’s picture

    Status: Needs review » Needs work

    The last submitted patch, 191: add_migrate_support_for-2447727-191.patch, failed testing.

    phenaproxima’s picture

    Status: Needs work » Closed (won't fix)

    I have read, re-read, reviewed, and even tried to refactor parts of this patch. This has led me to the conclusion that, for several reasons, we cannot continue with this patch as-is. I'm closing this issue so that we can pursue the noble goal of migrating Drupal 6 and 7 reference fields in a more sane, consistent, and compartmentalized way.

    I feel bad about effectively snubbing the hard and good work of so many people, but this patch has several problems that will make it hell to finish, much less get committed. It is littered with out-of-scope changes that, as far as I can tell, don't quite relate to the task at hand -- changes which haven't necessarily been reflected in changes to the test coverage. These changes confuse the issue and make it harder to review (and harder to find potential bugs). I think the reach of this patch exceeds its grasp.

    I intend to open follow-up issues which divide this big task (reference field support) into smaller parts. First we can support Drupal 6 node references, then user references, then ditto for Drupal 7, and so forth. I think this will make it a great deal easier (and faster) to get this completely done for 8.3.x.

    phenaproxima’s picture

    Title: Add migrate support for Drupal 6 node & user reference » Add base class for migrating reference fields
    Issue summary: View changes
    Status: Closed (won't fix) » Needs work
    Issue tags: +migrate-d7-d8, +blocker
    Related issues: +#2814949: Support Drupal 6 node reference fields

    Arise, Lazarus!

    After discussing with @benjy on IRC, I think we can reopen this issue in the name of not disrupting and discarding all the work that has been done here, with the proviso that we drastically reduce the scope of the patch.

    I think that the latest iteration of the patch contains a particularly excellent idea, which is the ReferenceBase base class upon which other reference-type field plugins can be built. They are generally very similar to each other, so I think it makes sense for ReferenceBase to be turned into its own, manageably small patch (in this issue) and committed first, followed closely by subclass implementations created in follow-up issues.

    Let's do this.

    P.S. To avoid reroll hell, I am postponing this issue on #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins.

    phenaproxima’s picture

    Status: Needs work » Postponed

    Apparently when I say "postponed", I mean "needs work".

    /me is an idiot.

    quietone’s picture

    Status: Postponed » Needs work
    Issue tags: -blocker
    heddn’s picture

    Status: Needs work » Postponed
    catch’s picture

    Priority: Normal » Major
    Issue tags: +Migrate critical

    Tagging and bumping status since this blocks several migrations like node reference and user reference.

    phenaproxima’s picture

    Version: 8.2.x-dev » 8.3.x-dev

    Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

    Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    quietone’s picture

    Version: 8.3.x-dev » 8.4.x-dev
    Issue tags: +Needs reroll

    #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins has been committed to 8.4.x.
    Changing this to 8.4.x, setting to NW and tagging for reroll.

    quietone’s picture

    Status: Postponed » Needs work

    Really set to NW

    Jo Fitzgerald’s picture

    Status: Needs work » Needs review
    Issue tags: -Needs reroll
    FileSize
    60.95 KB

    That was a tough re-roll!

    Status: Needs review » Needs work

    The last submitted patch, 204: add_migrate_support_for-2447727-204.patch, failed testing.

    heddn’s picture

    Issue tags: +Baltimore2017

    Tagging to sprint on in Baltimore.