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 migrate field plugins that handle all reference-type fields, since they tend to be more similar than they are different. Implement the node and user reference migrations for Drupal 6.

Remaining tasks

Any necessary documentation changes? - None found.

User interface changes

None.

API changes

Deprecates \Drupal\migrate_drupal\Plugin\migrate\field\NodeReference.

New @MigrateField plugins: \Drupal\migrate_drupal\Plugin\migrate\field\d6\NodeReference and \Drupal\migrate_drupal\Plugin\migrate\field\d6\UserReference.

Release notes snippet

TBD

CommentFileSizeAuthor
#318 2447727-318.patch25.38 KBquietone
#318 diff-315-318.txt1.7 KBquietone
#315 2447727-315.patch25.38 KBquietone
#315 diff-311-315.txt764 bytesquietone
#313 diff-311-313.txt1002 bytesquietone
#313 2447727-313.patch25.15 KBquietone
#311 2447727-311.patch25.35 KBquietone
#311 interdiff-309-311.txt631 bytesquietone
#309 2447727-309.patch25.34 KBquietone
#309 interdiff-308-309.txt636 bytesquietone
#308 2447727-308.patch25.33 KBquietone
#308 interdiff-304-306.txt3.25 KBquietone
#306 2447727-306.patch25.17 KBquietone
#306 interdiff-303-306.txt2.77 KBquietone
#303 interdiff-2447727-293-303.txt1.3 KBmradcliffe
#303 2447727-303.patch22.42 KBmradcliffe
#293 2447727-293.patch22.44 KBquietone
#293 interdiff-290-293.txt905 bytesquietone
#290 2447727-290.patch22.45 KBquietone
#290 interdiff-286-290.txt536 bytesquietone
#286 interdiff_282-286.txt1.18 KBNeslee Canil Pinto
#286 2447727-286.patch22.45 KBNeslee Canil Pinto
#282 interdiff_279-282.txt1.19 KBNeslee Canil Pinto
#282 2447727-282.patch23.4 KBNeslee Canil Pinto
#279 interdiff_276-279.txt2.4 KBNeslee Canil Pinto
#279 2447727-279.patch23.43 KBNeslee Canil Pinto
#276 interdiff_274-276.txt2.18 KBNeslee Canil Pinto
#276 2447727-276.patch25.06 KBNeslee Canil Pinto
#274 interdiff_271-274.txt2.28 KBNeslee Canil Pinto
#274 2447727-274.patch25.15 KBNeslee Canil Pinto
#272 interdiff-2447727-270-271.txt4.6 KBmradcliffe
#272 2447727-271.patch25.16 KBmradcliffe
#270 2447727-270.patch25.72 KBmradcliffe
#270 interdiff-2447727-269-270.txt2.99 KBmradcliffe
#269 2447727-269.patch24.45 KBmradcliffe
#269 interdiff-2447727-267-269.txt3.59 KBmradcliffe
#267 2447727-267.patch22.44 KBmradcliffe
#259 2447727-259.patch25.2 KBquietone
#259 interdiff-257-259.txt1.75 KBquietone
#257 interdiff-256-257.txt13.78 KBquietone
#257 2447727-257.patch26.35 KBquietone
#256 2447727-256.patch29.64 KBquietone
#254 2447727-254.patch29.64 KBquietone
#254 interdiff-252-254.txt538 bytesquietone
#252 2447727-252.patch28.95 KBquietone
#252 interdiff-250-252.txt31.86 KBquietone
#250 2447727-250.patch64.61 KBquietone
#246 interdiff-2447727-244-246.txt667 bytesyogeshmpawar
#246 2447727-246.patch62.87 KByogeshmpawar
#244 interdiff-2447727-242-244.txt674 bytesyogeshmpawar
#244 2447727-244.patch62.87 KByogeshmpawar
#242 2447727-242.patch62.87 KByogeshmpawar
#240 2447727-240.patch62.84 KBjofitz
#240 interdiff-38-40.txt701 bytesjofitz
#238 2447727-238.patch62.83 KBjofitz
#236 2447727-236-D8.patch55.43 KBmohit1604
#233 interdiff.txt2.12 KBquietone
#233 2447727-233.patch62.86 KBquietone
#232 2447727-232.patch63.47 KBjofitz
#232 interdiff-230-232.txt668 bytesjofitz
#230 2447727-230.patch63.47 KBjofitz
#230 interdiff-228-230.txt15.44 KBjofitz
#228 2447727-228.patch66.86 KBjofitz
#226 2447727-226.patch66.81 KBjofitz
#226 interdiff-224-226.txt9.63 KBjofitz
#224 2447727-224.patch63.94 KBjofitz
#224 interdiff-222-224.txt21.19 KBjofitz
#222 add_migrate_support_for-2447727-222.patch60.18 KBquietone
#217 2447727-213-reroll-217.patch60.67 KBjoelpittet
#213 add_migrate_support_for-2447727-213.patch61.23 KBjofitz
#213 interdiff-211-213.txt1.73 KBjofitz
#211 add_migrate_support_for-2447727-211.patch61.25 KBjofitz
#211 interdiff-209-211.txt2 KBjofitz
#209 add_migrate_support_for-2447727-209.patch61.41 KBjofitz
#204 add_migrate_support_for-2447727-204.patch60.95 KBjofitz
#191 add_migrate_support_for-2447727-191.patch61.99 KBdimaro
#191 interdiff-2447727-188-191.txt1.43 KBdimaro
#188 2447727-188.patch61.87 KBiMiksu
#188 interdiff.txt6.23 KBiMiksu
#184 2447727-184.patch61.5 KBkeithm
#182 2447727-182.patch61.51 KBhussainweb
#178 2447727-178.patch61.85 KBhussainweb
#178 interdiff-176-178.txt14.53 KBhussainweb
#176 2447727-175.patch63.06 KBhussainweb
#167 2447727-167.patch63.05 KBwebflo
#165 2447727-164.patch63.97 KBwebflo
#164 2447727-164.patch3.44 KBwebflo
#162 2447727-162.patch63.85 KBwebflo
#160 2447727-160.patch63.19 KBwebflo
#158 2447727-155.patch63.15 KBwebflo
#158 2447727-158.interdiff.txt2.34 KBwebflo
#155 2447727-155.patch63.15 KBwebflo
#154 2447727-154.patch62.32 KBwebflo
#152 add_migrate_support_for-2447727-152.patch64.11 KBdimaro
#147 2447727-147.patch64.01 KBtlyngej
#136 2447727-135.patch64.01 KBwebflo
#136 2447727-135.interdiff.txt751 byteswebflo
#133 2447727-133.interdiff.txt2.98 KBwebflo
#133 2447727-133.patch64.01 KBwebflo
#132 2447727-132.interdiff.txt1.47 KBwebflo
#132 2447727-132.patch63.29 KBwebflo
#130 2447727-130.patch62.64 KBwebflo
#130 2447727-130.interdiff.txt927 byteswebflo
#124 2447727-124.patch62.51 KBwebflo
#124 2447727-124.interdiff.txt674 byteswebflo
#123 interdiff-123.txt672 byteskgoel
#123 2447727-123.patch61.85 KBkgoel
#122 2447727-109-122.interdiff.txt3.43 KBwebflo
#122 2447727-122.patch61.2 KBwebflo
#119 interdiff-2447727-117-119.txt2.68 KBmalcomio
#119 2447727-119.patch54.46 KBmalcomio
#117 add_migrate_support_for-2447727-117.patch57.43 KBvprocessor
#117 add_migrate_support_for-2447727-117.interdiff.txt1.18 KBvprocessor
#113 add_migrate_support_for-2447727-113.patch57.53 KBvprocessor
#112 interdiff.txt2.92 KBkgoel
#112 2447727-112.patch60.67 KBkgoel
#109 2447727-109.patch59.88 KBkgoel
#92 add_migrate_support_for-2447727-92.patch59.86 KBdimaro
#82 add_migrate_support_for-2447727-82.patch59.79 KBk4v
#69 interdiff-2447727-66-69.txt4.09 KBk4v
#69 add_migrate_support_for-2447727-69.patch60.06 KBk4v
#66 interdiff-2447727-62-66.txt2.48 KBk4v
#66 add_migrate_support_for-2447727-66.patch55.97 KBk4v
#62 interdiff-2447727-44-62.txt1.6 KBk4v
#62 add_migrate_support_for-2447727-62.patch53.5 KBk4v
#44 2447727-44.patch53.46 KBwebflo
#43 2447727-43.patch53.42 KBwebflo
#43 2447727-43.interdiff.txt6.55 KBwebflo
#42 2447727-42.interdiff.txt13.83 KBwebflo
#42 2447727-42.patch47.51 KBwebflo
#39 2447727-39.patch45.36 KBwebflo
#38 2447727-38.interdiff.txt9.89 KBwebflo
#38 2447727-38.patch45.38 KBwebflo
#36 2447727-36.patch37.08 KBwebflo
#36 2447727-36.interdiff.txt9.41 KBwebflo
#34 2447727-34.patch24.05 KBwebflo
#30 2447727-30.patch21.62 KBwebflo
#27 2447727-27.patch19.63 KBwebflo
#24 2447727-24.patch11.53 KBwebflo
#23 2447727-23.patch11.65 KBwebflo
#19 2447727-19.patch14.23 KBwebflo
#18 2447727-18.patch72.6 KBwebflo
#16 2447727-16.patch102.22 KBphenaproxima
#10 add_migrate_support_for-2447727-10.patch628.8 KBclaudiu.cristea
#8 add_migrate_support_for-2447727-8.patch73.16 KBclaudiu.cristea
#6 add_migrate_support_for-2447727-6.patch57.45 KBclaudiu.cristea
#5 add_migrate_support_for-2447727-5.patch31.91 KBclaudiu.cristea
#2 references.patch32.59 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

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: Migrate D6 user reference fields to D8 entity reference field storage config entities
FileSize
32.59 KB

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: Migrate D6 user reference fields to D8 entity reference field storage config entities 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.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
31.91 KB
claudiu.cristea’s picture

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.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
73.16 KB

Didn't apply

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
628.8 KB

c'mon

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.

Anonymous’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

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!

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

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?

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.

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

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).

webflo’s picture

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.

Anonymous’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 K’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?
  9. 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.

Anonymous’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...

Anonymous’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 migration of node reference field values from Drupal 6

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

jofitz’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.

heddn’s picture

Priority: Major » Normal

With most of the reference from d6 mostly done, the priority here can be dropped. I've confirmed this with @phenaproxima in IRC as well.

mikeryan’s picture

Issue tags: -Migrate critical
jofitz’s picture

Status: Needs work » Needs review
FileSize
61.41 KB

Patch no longer applied so re-rolled.

Status: Needs review » Needs work

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

jofitz’s picture

Transfer functions out of the deprecated class CckFieldPluginBase.

Status: Needs review » Needs work

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

jofitz’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
61.23 KB

Correcting mistakes in the D6 fixture.

Status: Needs review » Needs work

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

kubrt’s picture

Hi,
great work on this so far. Is there any way to use this patch with the current 8.3.3 core to migrate nodereferences from D6 to D8 ?
Any pointers greatly appreciated.

phenaproxima’s picture

Adding #2886801: Migrate D6 user reference values to D8 as a related issue, since all reference field issues seem to be coalescing around this one.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
60.67 KB

An attempt at a re-roll. Probably messed up in FieldSettings::transform

Status: Needs review » Needs work

The last submitted patch, 217: 2447727-213-reroll-217.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joelpittet’s picture

Status: Needs work » Needs review

The errors are related to Undefined variable: source_field_type mostly, but still think FieldSettings::transform I did wrong, as it's unclear what it's doing and how the BC works.

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

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

joelpittet’s picture

Status: Needs review » Needs work
quietone’s picture

Status: Needs work » Needs review
FileSize
60.18 KB

Had a go at the reroll. I started with the patch in #213.

Status: Needs review » Needs work

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

jofitz’s picture

Status: Needs work » Needs review
FileSize
21.19 KB
63.94 KB
  • Replace references to "cckfield" with "field".
  • Replace assertIdentical() with assertSame() in new/edited assertions.

Hopefully this will correct more test failures than it creates...

Status: Needs review » Needs work

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

jofitz’s picture

Status: Needs work » Needs review
FileSize
9.63 KB
66.81 KB
  • Correct test expectations.
  • Now there is a user_reference field instance in the fixture the d6_user_role migration must run before the d6_field_instance migration.
  • Correct coding standards errors.

Status: Needs review » Needs work

The last submitted patch, 226: 2447727-226.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
66.86 KB

Patch in #226 no longer applies. Re-rolled (prior to addressing test failures).

Status: Needs review » Needs work

The last submitted patch, 228: 2447727-228.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
15.44 KB
63.47 KB

Having spent hours pouring over this code, I've made a few discoveries/improvements:

  1. Removed some commented out code and return a variable to it's previous name in d6/FieldSettings.
  2. Moved code from cckfield/FileField and MigrateCckFieldInterface to where it should be in field/FileField and MigrateFieldInterface.
  3. Removed references to cckfield.
  4. Most importantly: in field_node_reference do not reference nodes that have not yet been migrated.

Clearly that 4th change is a flimsy band-aid until a robust solution is found, but I just wanted to get this moving. I recommend we try and solve that in a follow-up so that this code can get committed. I also wonder if this would be an issue with other similar fields, e.g. term/user reference fields?

Status: Needs review » Needs work

The last submitted patch, 230: 2447727-230.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
668 bytes
63.47 KB

Corrected D6 field_storage_config entity count.

quietone’s picture

@Jo Fitzgerald, thanks for continuing to work on this.

I looked at this a bit too late in the day, my tired brain isn't up to a review. I did fix a few missing commas, fixed in the attached patch.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

maxocub’s picture

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

Needs reroll.

mohit1604’s picture

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

Thanks everyone for working on the issue. Here is the rerolled patch :)

Status: Needs review » Needs work

The last submitted patch, 236: 2447727-236-D8.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
62.83 KB

Reroll of patch in #233.

Status: Needs review » Needs work

The last submitted patch, 238: 2447727-238.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Needs review
FileSize
701 bytes
62.84 KB

Handle the mapping between nodereference_autocomplete and entity_reference_autocomplete_tags as removed from d6_field_instance_widget_settings.yml.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Re-rolled the #240 patch against 8.6.x

Status: Needs review » Needs work

The last submitted patch, 242: 2447727-242.patch, failed testing. View results

yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
62.87 KB
674 bytes

Updated patch will solve the test fails, also added an interdiff.

Status: Needs review » Needs work

The last submitted patch, 244: 2447727-244.patch, failed testing. View results

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
62.87 KB
667 bytes

One more try to resolve test fail issue with an interdiff.

phenaproxima’s picture

Status: Needs review » Needs work

I'm sorry to say that I think this patch needs significant refactoring.

The thing is, the current patch makes a lot of changes to field plugins that have nothing to do with reference fields. While these changes are a very welcome DX improvement, they are way out of scope here and need to be dealt with in follow-up issues. As it is, they creates a lot of patch noise that makes review and commit close to impossible in a timely manner. Let's revert as much of this patch as possible and change only the things that affect reference fields.

Once that's done, we may need to clear this with a framework manager because adding methods to MigrateFieldInterface may constitute a BC break. Which, if we can even get away with it, will need a change record as well. But that's putting the cart before the horse; first, let's clean things up so that we're making the fixes we actually need, rather than all the ones we ant.

Version: 8.6.x-dev » 8.7.x-dev

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

edysmp’s picture

I have a issue with drupal 6 to drupal 8 migration with nodereference fields, they are not migrated with their target_bundles. This issue seems fixing this too, do it?

quietone’s picture

Status: Needs work » Needs review
FileSize
64.61 KB

Just a reroll for 8.7.x

Status: Needs review » Needs work

The last submitted patch, 250: 2447727-250.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
31.86 KB
28.95 KB

Trying to address #247 and remove all changes that are not related to reference fields. The removed changes need to go to a new issue.

Status: Needs review » Needs work

The last submitted patch, 252: 2447727-252.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
538 bytes
29.64 KB

Adjust the entity counts for the addition of 4 fields.

heddn’s picture

Status: Needs review » Needs work

Really great work here on a long-standing issue. I think we're pretty close.

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

    Could we also add test coverage for target_bundles to address concerns in #249?

  • +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/NodeReference.php
    @@ -15,7 +16,40 @@
    +class NodeReference extends ReferenceBase {
    ...
    +  protected $nodeTypeMigration = 'd6_node_type';
    
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/ReferenceBase.php
    @@ -0,0 +1,128 @@
    +abstract class ReferenceBase extends FieldPluginBase implements ContainerFactoryPluginInterface {
    
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/UserReference.php
    @@ -15,7 +16,40 @@
    +class UserReference extends ReferenceBase {
    ...
    +  protected $userRoleMigration = 'd6_user_role';
    

    I wonder if these should move into a d6 namespace? Since they are obviously only for D6.

quietone’s picture

Status: Needs work » Needs review
FileSize
29.64 KB

Needs a reroll

quietone’s picture

255.1 Adding a test will only show it doesn't work. That is an existing bug and, in my opinion, should be in a followup. where the removed code here can be used to fix that. The target_bundles are migrated in a new method transformFieldInstanceSettings which is being removed here according to #247.

255.2 This patch moves the plugins to a d6 subdirectory.

And removes the new method lookupMigrations and transformFieldInstanceSettings which does not need to be done to make a base class. Add doc comments and other cleanup.

If this passes, the next piece is to look at any other reference migrations that can use the new base class, such as taxonomytermreference.

quietone’s picture

I've skimmed through the comments and found that phenaproxima said in #172 that user and node reference exist in Drupal 7, which is different that what heddn says in #255.

Can someone clarify that?

quietone’s picture

mikelutz answered on slack that it should be d6 and d7 so moving back.

quietone’s picture

Re #249 and target bundles. My comment about that in #257 is wrong. The work to migrate target_bundles and the tests are, or rather were, in this issue. They are now moved to #3033522: Add methods to customize field to MigrateFieldInterface so that this patch is only making the ReferenceBase class.

mikelutz’s picture

I was talking with Damien McKenna and John Ouellet in slack, and figured out the confusion with the drupal versions. Currently in code, we seem to support d6 nodereferece and userreference fields, but we don't actually have field plugins for D7 reference fields (node and user). D7 term reference fields were part of core, but node and user references were in the contrib references module, which we don't but should support.

I don't know, but I suspect they might need new field plugins separate from the d6 ones, so we might want the plugins in a d6 folder and a followup to write new ones for D7. I need to do a bit more research.

heddn’s picture

Status: Needs review » Needs work

So, for a base field maybe we want to provide a getter for $nodeTypeMigration and $userRoleMigration that is abstract? Then leave these in the global namespace and make a d6 version that implements those methods.

mradcliffe’s picture

I'm debating whether or not it's worthwhile to also include the test fixture and test I wrote in #3008202: Migrating more than 1 entity reference field instance fails to this issue and closing it. That test confirms that an entityreference entity reference used on multiple bundles of an entity do not get migrated to entity_reference fields correctly and field definitions are missing.

It would probably be confusing to users if this issue were completed, but they would still end up with a broken web site if they re-used entityreference fields in Drupal 7 when saving an entity reference field on one of those nodes with broken field definitions.

On the other hand the patch is rather large already.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Wim Leers’s picture

Priority: Normal » Major
Issue tags: +blocker

This blocks #2814953: Migrate Drupal 7 node/user reference fields.

I increased the priority of that issue to Major based on:

Note that https://git.drupalcode.org/project/references is used on 100K Drupal 7 sites (see https://www.drupal.org/project/usage/references), or about 1 in 7 (see https://www.drupal.org/project/usage/drupal).

Matching priority.

mradcliffe’s picture

This an attempt at a re-roll of the patch in #259 and incremented the counts in Upgrade6Test from what they are in 8.9.x by 4, which is what the patch in #259 is doing.

This is still Needs work based on #261 + #262, and I think that means reworking the changes in #257.

mradcliffe’s picture

I'm going to work on re-rolling and fixing the migration count changes that I introduced in #267.

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
3.59 KB
24.45 KB

I went back and counted up the field config manually. I think I thought there were 4 more added to core, but that wasn't the case. I changed the assertion in FieldDiscoveryTest to assert the contents of the array rather than the explicit order.

The interdiff is a little strange because the hunks changed.

I have some more time so I'm going to try to move NodeReference and UserReference back into a d6 folder like @quietone did in #257 and @mikelutz suggests in #261.

mradcliffe’s picture

This patch moves NodeReference and UserReference back underneath a d6 directory and updates the namespaces in those files.

It also addresses a test failure in #269.

This doesn't attempt to implement any getters mentioned by @heddn in #262, which I am confused about what the purpose of splitting the protected property is for.

Edit: I'm finished working on this for the day.

The last submitted patch, 269: 2447727-269.patch, failed testing. View results

mradcliffe’s picture

I think I understand what @heddn was suggesting. Here's a new patch that creates an abstract protected method on ReferenceBase that takes a MigrationInterface as a parameter. This should allow some amount of flexibility for contrib, custom, or core plugins that implement it.

I renamed the protected properties on d6/UserReference and d6/NodeReference to the ones that @heddn suggested. I still don't understand the comment about the "global namespace", but hopefully this meets expectations. This also removes the referenceTypeMigration property from ReferenceBase.

I marked the plugins as @internal because core plugins are internal.

Previous patches also made this change:

+++ b/core/modules/taxonomy/tests/src/Unit/Plugin/migrate/field/TaxonomyTermReferenceFieldTest.php
@@ -27,7 +29,10 @@ class TaxonomyTermReferenceFieldTest extends UnitTestCase {
+    $this->plugin = new TaxonomyTermReference([], 'taxonomy', [], $migration_plugin_manager, $migrate_process_plugin_manager);

This is giving a warning in phpstorm because the plugin class doesn't implement a __construct method that takes 5 arguments. It's using \Drupal\Component\Plugin\PluginBase::__construct(array $configuration, $plugin_id, $plugin_definition);

I'm not sure about the intent of the change here? Are $migration_plugin_manager and $migrate_process_plugin_manager necessary? It does not affect the test run of the unit test. I've reverted this change in this patch.

heddn’s picture

Status: Needs review » Needs work

Very nice work! You dissected my fairly obtuse comments quite nicely. A few small comments.

  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/NodeReference.php
    @@ -0,0 +1,46 @@
    + * @internal
    
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/UserReference.php
    @@ -14,8 +17,31 @@
    + * @internal
    

    :thumbsup:

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/NodeReference.php
    @@ -0,0 +1,46 @@
    +class NodeReference extends ReferenceBase {
    
    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/UserReference.php
    @@ -14,8 +17,31 @@
    +class UserReference extends ReferenceBase {
    

    Would it make sense to mark these as final? It would, but not sure what that means for BC concerns. Ideally we would. But :shrug: I guess we can't at this point.

  3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/NodeReference.php
    @@ -0,0 +1,46 @@
    +  protected function getReferenceTypeMigration(MigrationInterface $migration) {
    

    Naming is hard. What if we called this getEntityTypeMigrationId? Would that make more sense to anyone else?

Neslee Canil Pinto’s picture

Status: Needs work » Needs review
FileSize
25.15 KB
2.28 KB
heddn’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/ReferenceBase.php
@@ -20,7 +20,7 @@
+  abstract protected function getEntityTypeMigrationId(MigrationInterface $migration);

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/NodeReference.php
@@ -32,7 +32,7 @@
+  protected function getEntityTypeMigrationId(MigrationInterface $migration) {

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/UserReference.php
@@ -32,7 +32,7 @@
+  protected function getEntityTypeMigrationId(MigrationInterface $migration) {

I just noticed this, why do we need to provide an argument to this method? We could be fine without it, right?

Neslee Canil Pinto’s picture

Status: Needs work » Needs review
FileSize
25.06 KB
2.18 KB
heddn’s picture

Status: Needs review » Reviewed & tested by the community

If this returns green. Then I like it!

quietone’s picture

Status: Reviewed & tested by the community » Needs work

Really glad to see the progress here. I found a few nits.

  1. +++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
    @@ -39,6 +39,8 @@ protected function setUp() {
    +      'd6_filter_format',
    +      'd6_user_role',
    

    Why are these migrations needed for taxonomy translation? I applied the patch and ran the test without them and it passed.

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/ReferenceBase.php
    @@ -0,0 +1,69 @@
    +   * @param \Drupal\migrate\Plugin\MigrationInterface $migration
    +   *   The migration entity.
    

    Unused parameter, can be removed.

  3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/NodeReference.php
    @@ -0,0 +1,46 @@
    +use Drupal\migrate\Plugin\MigrationInterface;
    

    Not used, can be removed.

  4. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
    @@ -47691,10 +47983,10 @@
    +  'status' => '1',
    ...
    +  'schema_version' => '6002',
    

    I don't think a theme needs to be enabled. This can be removed.

  5. +++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6TestBase.php
    @@ -82,6 +82,8 @@ protected function migrateContentTypes() {
    +      'd6_filter_format',
    +      'd6_user_role',
    

    Including the filter_format and user_role migration in the field migrations is unexpected. I'd rather see this removed and those migrations only added to the tests that need it.

Neslee Canil Pinto’s picture

Status: Needs work » Needs review
FileSize
23.43 KB
2.4 KB
mradcliffe’s picture

@heddn, I added the parameter because the method seemed sort of basic without one. I thought it would make sense for a contrib, custom, or other core plugin might need to access the migration entity to calculate the correct entity id. Otherwise the method seems fairly limiting.

It could be an API addition later, but harder to do later.

quietone’s picture

Status: Needs review » Needs work

@Neslee Canil Pinto, thanks for the changes. It would be easier for me to review if you added a comment stating what you changed.

And one more nit.

+++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
@@ -47694,7 +47986,7 @@
-  'schema_version' => '-1',
+  'schema_version' => '6002',

This changed can be removed.

Neslee Canil Pinto’s picture

Status: Needs work » Needs review
FileSize
23.4 KB
1.19 KB

Hi @quietone,

+++ b/core/modules/content_translation/tests/src/Kernel/Migrate/d6/MigrateTaxonomyTermTranslationTest.php
@@ -39,6 +39,8 @@ protected function setUp() {
+      'd6_filter_format',
+      'd6_user_role',

Removed d6_filter_format and d6_user_role.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/ReferenceBase.php
@@ -0,0 +1,69 @@
+   * @param \Drupal\migrate\Plugin\MigrationInterface $migration
+   *   The migration entity.

Removed the @param.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/NodeReference.php
@@ -0,0 +1,46 @@
+use Drupal\migrate\Plugin\MigrationInterface;

Removed the unused use statement.

+++ b/core/modules/migrate_drupal/tests/fixtures/drupal6.php
@@ -47691,10 +47983,10 @@
+  'status' => '1',
...
+  'schema_version' => '6002',

Removed schema_version key.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6TestBase.php
@@ -82,6 +82,8 @@ protected function migrateContentTypes() {
+      'd6_filter_format',
+      'd6_user_role',

Removed d6_filter_format and d6_user_role.

Status: Needs review » Needs work

The last submitted patch, 282: 2447727-282.patch, failed testing. View results

Neslee Canil Pinto’s picture

Status: Needs work » Needs review

We need schema_version key, #279 Works Fine

mradcliffe’s picture

Status: Needs review » Needs work

I think @quietone wanted to schema_version to be set, to -1, not removed.

Flipping back to Needs work because the latest patch is broken.

Neslee Canil Pinto’s picture

quietone’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the fixes, sorry about the confusion on the 'schema' line in the Drupal6 test fixture.

I have applied the patch and checked the changes since #278 and they all are fixed. This can go back to RTBC.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Issue tags: +Needs reroll
quietone’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
536 bytes
22.45 KB

A simple reroll

mradcliffe’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Assertions look the same between #286 and #291. Going to flip to RTBC based on the review in #287.

quietone’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for restoring the RTBC.

Onoe small change to make.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/FieldDiscoveryTest.php
@@ -269,9 +273,11 @@ public function testAddFields() {
+    $this->assertSame(25, count($actual_fields['node']['story']));

I think this should use assertCount()

quietone’s picture

Status: Needs work » Needs review
FileSize
905 bytes
22.44 KB

Fix for #292.

Status: Needs review » Needs work

The last submitted patch, 293: 2447727-293.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review

Unrelated failure in Drupal\Tests\media_library\FunctionalJavascript\EntityReferenceWidgetTest

mradcliffe’s picture

Hmm, there haven't been any changes that would seem to effect this, but it's still failing randomly (?). I don't know why this patch would cause that to fail, and HEAD isn't broken on 9.1.x.

steinmb’s picture

Status: Needs review » Reviewed & tested by the community

Looks like RTBC to me. assertCount() seems like the right change.

quietone’s picture

Nuuou’s picture

My team has been working on some Drupal 7 to Drupal 8 (or 9) migrations, and this is something we obviously encountered.

I'm fairly confused by the state of this though, as the initial request seemed to be about both Drupal 6 and Drupal 7. However, unless I'm misunderstanding this, it appears the only included migration path in this update is for Drupal 6.

Applying this patch does not seem to migrate Drupal 7 References module (not entityreference module) to Drupal 8/9. Is that correct, or am I encountering something else possibly?

mradcliffe’s picture

Yes, that's correct, @Nuuou. This issue is about adding a base class that can then be used by the follow-up issues for migrations for D7 references, D7 entity reference, etc...

quietone’s picture

There is a patch at 2814953-#33 for Drupal7. I have not tested it.

mradcliffe’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

#293 is failing to apply to 9.1.x, but status didn't update. Flipping the status to Needs work and and adding the Needs reroll tag.

I'll try re-rolling it again.

mradcliffe’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
22.42 KB
1.3 KB

The patch applied cleanly when I used a 3-way merge (git apply -3). I noticed that the current patch used assertIdentical test assertion method that was deprecated in Drupal 9. The only difference in the patches after the re-roll was this.

Uploading the patch for testing. Hopefully it still passes.

I also updated the issue summary removing the outdated part of remaining tasks.

I realize at 300+ comments, we're now at the second page, which makes it significantly harder for a core committer to review. What can we do to make it easier review the next time the issue is RTBC?

quietone’s picture

@mradcliffe, thanks for the reroll. It look good to me.

+1 for RTBC

sime’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

The API changes in the IS are out of date, and seem to be related to #272 patch. Have updated.

Code reads logically. I ran the affected tests files successfully. I tweaked the fixture data and caused the relevant tests to fail as expected.

Since @quietone has voted RTBC, and this was RTBC before the patch got stale, and the interdiff in #303 adds no logic, I'm RTBC.

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
FileSize
2.77 KB
25.17 KB

@sime, thank you very much! Your review and testing will help the next reviewer. You see, in the meantime, jibran pointed out that deleting NodeReference was a BC break, so more work to do.

Which I have attempted to to do now. The IS is updated to include the deprecation, a change record has been added, deprecated \Drupal\migrate_drupal\Plugin\migrate\field\NodeReference and a test has been added.

Status: Needs review » Needs work

The last submitted patch, 306: 2447727-306.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
3.25 KB
25.33 KB

There were some coding standards to fix. I've not figured out why the new test is failing. It passes with phpunit but then I found that run-tests.sh isn't working on any of my dev environments so that is no help to me right now.

quietone’s picture

I am sure I checked the namespace several times.

mradcliffe’s picture

Issue summary: View changes

+1 on adding the backwards compatibility class.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/NodeReference.php
@@ -2,36 +2,15 @@
+ * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use
+ * \Drupal\image\Plugin\migrate\field\d7\ImageField instead.

Is this supposed to be ImageField or \Drupal\migrate_drupal\Plugin\migrate\field\d6\NodeReference?

quietone’s picture

@mradcliffe, thanks. It's a copy/paste error. This should fix it.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the update.

1) I reviewed from #305 and onward as there are a LOT of comments. :)

2) Issue was RTBC in #305 but patch was updated in #306 to #309 to address a backwards compatibility issue for NodeReference.

3) The only interdiffs I reviewed were the ones from #306, #308, #309, and #311.

a) #306 adds a deprecation for NodeReference and a test.

b) #308 addresses code standard issues from #306.

c) #309 addresses name space issue from #308.

d) #311 addresses #310 for a wrong class.

4) Patch applies cleanly and tests pass.

5) Read the change record and made some minor formatting changes.

6) Title and issue summary are clear though there is a question on whether documentation will need to be updated based on this so there may need to be a follow-up issue.

7) Note that the release notes snippet does need to be filled in.

Back to RTBC based on the above.

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
FileSize
25.15 KB
1002 bytes

And now a reroll. The only change was to UserReference.php

There is no documentation to update, at least none that I could find.

Kristen Pol’s picture

Status: Needs review » Needs work

Moving back to needs work as it looks like a bit of the patch from #311 was not preserved.

Diff:

< -namespace Drupal\migrate_drupal\Plugin\migrate\field;
< +namespace Drupal\migrate_drupal\Plugin\migrate\field\d6;
<  
<  use Drupal\migrate\Plugin\MigrationInterface;
< +use Drupal\migrate_drupal\Plugin\migrate\field\ReferenceBase;
---
> @@ -7,6 +7,8 @@
>  // cspell:ignore userreference

Previous patch:

diff --git a/core/modules/migrate_drupal/src/Plugin/migrate/field/UserReference.php b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/
similarity index 55%
rename from core/modules/migrate_drupal/src/Plugin/migrate/field/UserReference.php
rename to core/modules/migrate_drupal/src/Plugin/migrate/field/d6/UserReference.php
index 4a60f450de..b389f8b0b1 100644
--- a/core/modules/migrate_drupal/src/Plugin/migrate/field/UserReference.php
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/field/d6/UserReference.php
@@ -1,10 +1,13 @@
 <?php

-namespace Drupal\migrate_drupal\Plugin\migrate\field;
+namespace Drupal\migrate_drupal\Plugin\migrate\field\d6;

 use Drupal\migrate\Plugin\MigrationInterface;
+use Drupal\migrate_drupal\Plugin\migrate\field\ReferenceBase;

quietone’s picture

Status: Needs work » Needs review
FileSize
764 bytes
25.38 KB

Arg! I got interrupted and didn't check my work. Rerolled again against #311.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the update. Back to RTBC since:

1) The missing code noted in #314 was added back.

2) Tests are green again.

3) See #312 for more details on the review.

catch’s picture

Status: Reviewed & tested by the community » Needs work

#315 looks good but unfortunately needs another re-roll.

quietone’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
25.38 KB

OK. Another reroll. The changes are to the drupal6 test fixture.

Status: Needs review » Needs work

The last submitted patch, 318: 2447727-318.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review

There was a random test failure and then I set this to retest.

Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DCCO2020

Thanks for the update. Back to RTBC based on:

1) Patch still applies cleanly (just checked).

2) Tests pass.

3) Was RTBC based on #316 and just needed a reroll.

  • catch committed 2e80fc2 on 9.1.x
    Issue #2447727 by webflo, quietone, jofitz, Neslee Canil Pinto,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2e80fc2 and pushed to 9.1.x. Thanks!

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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

quietone’s picture

Published change record.