Problem/Motivation

The D7 User migration plugin fails to migrate cck fields (e.g. file fields and taxonomy term reference fields) because in the field database table some of the D8 fields (e.g. field_FIELD_NAME_target_id) have different names to D7 (e.g. field_FIELD_NAME_tid) and so require a custom transform function.

Proposed resolution

Use the cck plugin manager in a similar manner to the D7 Node migration plugin because that is able to handle cck fields (e.g. file fields and taxonomy term reference fields).

Remaining tasks

Review and commit.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#73 migrate_d7_cck_fields-2673960-73.patch12.15 KBjofitz
#73 interdiff_70-73.txt1.35 KBjofitz
#70 migrate_d7_cck_fields-2673960-70.patch12.07 KBjofitz
#64 migrate_d7_cck_fields-2673960-64.patch12.07 KBjofitz
#64 interdiff_56-64.txt701 bytesjofitz
#56 migrate_d7_cck_fields-2673960-56.patch11.91 KBjofitz
#49 migrate_d7_cck_fields-2673960-49.patch11.91 KBjofitz
#49 interdiff_45-49.txt611 bytesjofitz
#47 migrate_d7_cck_fields-2673960-47.patch30.33 KBjofitz
#47 interdiff_45-47.txt611 bytesjofitz
#45 migrate_d7_cck_fields-2673960-45.patch11.32 KBjofitz
#45 interdiff_40-45.txt8.79 KBjofitz
#40 migrate_d7_cck_fields-2673960-40.patch5.68 KBjofitz
#34 migrate_d7_cck_fields-2673960-34.patch10.79 KBjofitz
#34 interdiff.txt1.43 KBjofitz
#31 71155908.jpg93.67 KBphenaproxima
#30 migrate_d7_cck_fields-2673960-30.patch10.78 KBjofitz
#30 interdiff.txt1.52 KBjofitz
#29 migrate_d7_cck_fields-2673960-29.patch10.78 KBjofitz
#25 migrate_d7_cck_fields-2673960-25.patch10.63 KBhussainweb
#25 interdiff-24-25.txt1.18 KBhussainweb
#24 migrate_d7_cck_fields-2673960-24.patch10.61 KBjofitz
#22 migrate_d7_cck_fields-2673960-22-complete.patch13.25 KBjofitz
#22 migrate_d7_cck_fields-2673960-22-tests.patch8.95 KBjofitz
#19 migrate_d7_cck_fields-2673960-19-complete.patch13.2 KBjofitz
#19 migrate_d7_cck_fields-2673960-19-tests.patch8.47 KBjofitz
#16 migrate_d7_cck_fields-2673960-16-complete.patch12.54 KBjofitz
#16 migrate_d7_cck_fields-2673960-16-tests.patch8.24 KBjofitz
#13 migrate_d7_cck_fields-2673960-13-complete.patch12.25 KBjofitz
#13 migrate_d7_cck_fields-2673960-13-tests.patch7.94 KBjofitz
#10 migrate_d7_user_image_fields-2673960-10.patch4.31 KBjofitz
#2 migrate_d7_user_image_fields-2673960-2.patch2.75 KBjofitz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jo Fitzgerald created an issue. See original summary.

jofitz’s picture

Use a field-type-specific migration process plugin if there is one available (similar to d7_node).

jofitz’s picture

Status: Active » Needs review
jofitz’s picture

Title: Unable to migrate D7 User image fields » Unable to migrate D7 User taxonomy term reference fields
Issue summary: View changes

I confused a couple of issues (see also #2674088: Unable to migrate D7 image fields) - I have made the adjustments to the Summary, but the patch filenames will look a little odd because they reference image fields rather than taxonomy term reference fields. Oops!

benjy’s picture

Is this patch missing the actual process plugin itself?

jofitz’s picture

@benjy No, the Taxonomy Term Reference process plugin already exists presumably for use by the D7 Node migration.

benjy’s picture

Issue tags: +Needs tests

OK, great we just need some tests then.

quietone’s picture

Issue tags: +migrate-d7-d8

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.

jofitz’s picture

Updated for D8.1.1, i.e. edits to User migration plugin rather than builder.

vasi’s picture

We already have Drupal\migrate_drupal\Plugin\migrate\CckMigration that does this sort of processing. Can you build on top of that, instead of copying the code?

vasi’s picture

Status: Needs review » Needs work
jofitz’s picture

Added tests.
(integration of CckMigration to follow)

The last submitted patch, 13: migrate_d7_cck_fields-2673960-13-tests.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: migrate_d7_cck_fields-2673960-13-complete.patch, failed testing.

jofitz’s picture

Added tests.
(integration of CckMigration to follow)

N.B. Test user language changed from '' (empty string) to 'en' because fields (with a language) cannot be associated with users without a language (see #2671312: No default value for User langcode when migrating D7 users with no language).

The last submitted patch, 16: migrate_d7_cck_fields-2673960-16-tests.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: migrate_d7_cck_fields-2673960-16-complete.patch, failed testing.

jofitz’s picture

The last submitted patch, 19: migrate_d7_cck_fields-2673960-19-tests.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 19: migrate_d7_cck_fields-2673960-19-complete.patch, failed testing.

jofitz’s picture

The last submitted patch, 22: migrate_d7_cck_fields-2673960-22-tests.patch, failed testing.

jofitz’s picture

hussainweb’s picture

Fixing two really small nitpicks.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Assigning to myself for review.

jofitz’s picture

How's the testing going, @phenaproxima? After 13 days I assume it's been a very thorough test :p

phenaproxima’s picture

Status: Needs review » Needs work

Dang, this got lost in the flow. Thanks for the reminder, @Jo Fitzgerald. Anyway, this is very close to RTBC. I have one question and a few nitpicks.

  1. +++ b/core/modules/user/migration_templates/d7_user.yml
    @@ -37,6 +37,7 @@ migration_dependencies:
    +    - d7_field_instance
    

    I'm not sure about this. What was the reasoning behind adding this dependency?

  2. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -27,15 +40,42 @@ protected function setUp() {
    +  protected function createType($id) {
    

    Let's call this createNodeType(), for clarity's sake.

  3. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -27,15 +40,42 @@ protected function setUp() {
    +      'label' => $this->randomString(),
    

    I think we might want $this->randomMachineName() or similar; IIRC, randomString() can include punctuation and other gibberish, which can lead to breaks and other weirdness in some circumstances.

  4. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -60,8 +100,12 @@ protected function setUp() {
    +   *   The target id of the file field.
    

    s/id/ID

  5. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -80,6 +124,14 @@ protected function assertEntity($id, $label, $mail, $password, $access, $login,
    +      $this->assertEquals($field_integer, $user->field_integer->value);
    

    Let's use assertSame() here.

  6. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -80,6 +124,14 @@ protected function assertEntity($id, $label, $mail, $password, $access, $login,
    +      $this->assertEquals($field_file_target_id, $user->field_file->target_id);
    

    And here.

  7. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -87,7 +139,7 @@ protected function assertEntity($id, $label, $mail, $password, $access, $login,
    +    $this->assertEntity(2, 'Odo', 'odo@local.host', $password, '0', '0', FALSE, 'en', 'odo@local.host', [RoleInterface::AUTHENTICATED_ID], FALSE, 99, 2);
    

    So glad to see that my Deep Space Nine reference is still intact. =P

jofitz’s picture

Status: Needs work » Needs review
FileSize
10.78 KB

Re-roll patch prior to code review changes.

jofitz’s picture

Thanks for the code review.

  1. d7_field_instance has been added as a dependency because the field instance must exist before it can added to a user. Similar to adding a field to a node where it is a dependency of d7_node.yml. To prove the requirement of this dependency to myself (it's been a while since I last worked on this) I removed the migration from MigrateUserTest.php and the test promptly failed.
  2. I 'borrowed' the concept of createType() from MigrateFieldInstanceTest.php so have kept the function name the same for consistency. Also it is creating CommentTypes too, not just NodeTypes. I haven't made the change, but am happy to do so if required.
  3. label is only the human-readable name so randomString() should be perfectly adequate. I haven't made the change, but am happy to do so if required.
  4. Changed "id" to "ID".
  5. Changed assertEquals() to assertSame().
  6. Changed assertEquals() to assertSame().
  7. How could I delete Odo!?! Your DS9 references raised a smile in my office.
phenaproxima’s picture

FileSize
93.67 KB
  1. Seems legit to me!
  2. I'm not too bent out of shape over the name of the method, it's OK to leave it as-is.
  3. I think I'd prefer randomMachineName(), because it might not be immediately obvious to developers that randomString() includes non-alphanumeric characters. This is really a DX issue with the random generator, but for sanity's sake let's go with randomMachineName().

For you, my fellow DS9 fan:

The Sisko

EDIT: Punches it in the face. Stupid typos.

Status: Needs review » Needs work

The last submitted patch, 30: migrate_d7_cck_fields-2673960-30.patch, failed testing.

The last submitted patch, 30: migrate_d7_cck_fields-2673960-30.patch, failed testing.

jofitz’s picture

Made change requested by phenaproxima (randomString() -> randomMachineName()) and corrected embarrassingly simple test fail that should've been caught in local testing.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

Status: Needs review » Needs work

The last submitted patch, 34: migrate_d7_cck_fields-2673960-34.patch, failed testing.

jofitz’s picture

Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review

Test now passes - appears to have been a testbot glitch.

jofitz’s picture

@phenaproxima would you mind taking a look at this, please? Previously you had said it was close to RTBC, I'm sure it's even closer now - let's push it over the line. Thanks.

mitrpaka’s picture

Status: Needs review » Needs work
Issue tags: -migrate-d7-d8 +migrate-d7-d8 Needs reroll

#34 needs reroll

curl https://www.drupal.org/files/issues/migrate_d7_cck_fields-2673960-34.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 11053  100 11053    0     0  31239      0 --:--:-- --:--:-- --:--:-- 31311
patching file core/modules/migrate_drupal/tests/fixtures/drupal7.php
Reversed (or previously applied) patch detected!  Assume -R? [n] y
Hunk #2 FAILED at 4374.
Hunk #4 FAILED at 6160.
Hunk #5 succeeded at 6582 (offset 42 lines).
Hunk #6 FAILED at 40865.
3 out of 6 hunks FAILED -- saving rejects to file core/modules/migrate_drupal/tests/fixtures/drupal7.php.rej
patching file core/modules/migrate_drupal_ui/src/Tests/d7/MigrateUpgrade7Test.php
jofitz’s picture

Status: Needs work » Needs review
Issue tags: -migrate-d7-d8 Needs reroll +migrate-d7-d8
FileSize
5.68 KB

Re-roll

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Assigning to myself for review and shepherding to commit.

phenaproxima’s picture

Looking again at this patch, I am wondering why it doesn't even mention taxonomy term references. I guess the IS is outdated?

jofitz’s picture

Title: Unable to migrate D7 User taxonomy term reference fields » Unable to migrate D7 User cck fields
Issue summary: View changes

Updated the IS coz it was waaay out of date.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate_drupal/tests/fixtures/drupal7.php
    @@ -4395,6 +4395,18 @@
    +->values(array(
    +  'entity_type' => 'user',
    +  'bundle' => 'user',
    +  'deleted' => '0',
    +  'entity_id' => '2',
    +  'revision_id' => '2',
    +  'language' => 'und',
    +  'delta' => '0',
    +  'field_file_fid' => '2',
    +  'field_file_display' => '1',
    +  'field_file_description' => 'file desc',
    +))
    

    Does this file actually exist in the DB? According to my grepping in drupal7.php (as it currently exists in the 8.3.x branch) it does not. It seems like that might break things down the line, since no file field should be referencing a non-existent file if Drupal is working correctly. Also, does

  2. +++ b/core/modules/user/src/Plugin/migrate/User.php
    @@ -33,7 +26,18 @@ public function getProcess() {
    +          $field_type = $row->getSourceProperty('type');
    

    Let's add a continue here if $field_type is empty, just for the sake of sanity checking. Or throw an exception.

  3. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -109,10 +109,12 @@ protected function createType($id) {
    +  protected function assertEntity($id, $label, $mail, $password, $created, $access, $login, $blocked, $langcode, $timezone, $init, $roles, $field_integer, $field_file_target_id, $has_picture = FALSE) {
    

    $field_file_target_id should probably be defaulted to NULL, and the doc comment updated accordingly.

  4. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -155,6 +157,10 @@ protected function assertEntity($id, $label, $mail, $password, $created, $access
    +    if (!is_null($field_file_target_id)) {
    

    Should be !empty() to account for falsy values.

  5. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -155,6 +157,10 @@ protected function assertEntity($id, $label, $mail, $password, $created, $access
    +      $this->assertSame($field_file_target_id[0], $user->field_file->target_id);
    

    I would rather this was not an array. Just NULL or an int, as in the doc comment.

  6. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -190,6 +196,14 @@ public function testUser() {
    +      $field_file = Database::getConnection('default', 'migrate')
    +        ->select('field_data_field_file', 'ff')
    +        ->fields('ff', array('field_file_fid'))
    +        ->condition('ff.entity_id', $source->uid)
    +        ->execute()
    +        ->fetchCol();
    

    Use fetchField() to retrieve the scalar file ID, rather than an array.

  7. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -190,6 +196,14 @@ public function testUser() {
    +      $field_file = !empty($field_file) ? $field_file : NULL;
    

    Once you change to fetchField(), no need for this.

I think I now understand the problem this is fixing, so once these are fixed and tests are passing, I'd say this is RTBC.

jofitz’s picture

  1. Added file to be referenced by fid 2.
  2. continue added.
  3. Set default value to FALSE rather than NULL because that is the 'empty' value given by fetchField() (see 6.)
  4. Corrected conditional.
  5. Integer passed instead of array.
  6. fetchField() used instead of fetchCol().
  7. Removed now-redundant line.

Status: Needs review » Needs work

The last submitted patch, 45: migrate_d7_cck_fields-2673960-45.patch, failed testing.

jofitz’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Needs work » Needs review
FileSize
611 bytes
30.33 KB

I shoulda seen that coming.

Status: Needs review » Needs work

The last submitted patch, 47: migrate_d7_cck_fields-2673960-47.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
611 bytes
11.91 KB

Wrong patch

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Omigod, a DS9 ASCII art in the tests? @Jo Fitzgerald, you are my new best friend.

Other than that, nothing about the patch strikes me as objectionable. I think this is good to go.

heddn’s picture

Status: Reviewed & tested by the community » Needs work

Nit: can we convert the ascii art into using vfsStream instead of a static file?

jofitz’s picture

Assigned: phenaproxima » Unassigned

@heddn can you explain further, please? The static file was added in response to point 1 in #44 - the file is not actually referenced within this patch it exists because otherwise it "...might break things down the line, since no file field should be referencing a non-existent file if Drupal is working correctly."

heddn’s picture

Status: Needs work » Reviewed & tested by the community

https://github.com/mikey179/vfsStream is what I'm referring to. It is a virtual filestream for use in phpunit. I used it in http://cgit.drupalcode.org/migrate_source_csv/tree/tests/src/Unit based on recommendations from @phenaproxima.

I see your point though. This is a modification of a fixture. So I think my comment in #51 was misguided. Back to RTBC.

jofitz’s picture

Aha, I see. Thanks for explaining (and so promptly too)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: migrate_d7_cck_fields-2673960-49.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
11.91 KB

Re-roll.

jofitz’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC in (hope and) expectation of a test pass after a straightforward re-roll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 56: migrate_d7_cck_fields-2673960-56.patch, failed testing.

xjm’s picture

I love the DS9 ASCII art, but is it GPL?

quietone’s picture

Issue tags: +Needs reroll

Found the ascii art at https://startrekasciiart.blogspot.co.nz/2011/05/deep-space-nine.html, with this comment about use.

All pictures are by Joshua Bell, unless otherwise noted. Any corrections to the attributions would be welcomed. Permission to re-use any ASCII art on this blog is granted, assuming attribution (if any) is retained.

jofitz’s picture

Is there a standard method for attributing such things, e.g.:

Used with permission from:
Orbital Space Station (Terok Nor - Deep Space 9) - Joe Reiss
https://startrekasciiart.blogspot.co.uk/2011/05/deep-space-nine.html

jofitz’s picture

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

The patch still applies - must've been a testbot issue.

Unless attribution is needed in the ascii art file then this is ready to go.

mikeryan’s picture

Status: Needs review » Needs work

Attribution is a good community thing, even if not strictly required.

jofitz’s picture

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

Attributed ascii art.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: migrate_d7_cck_fields-2673960-64.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review

Ran re-test - all fine.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: migrate_d7_cck_fields-2673960-64.patch, failed testing.

jofitz’s picture

Simple re-roll.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

phenaproxima’s picture

Status: Needs review » Needs work

I think I finally grok this, and I think I see why the test proves that it works. I have only nitpicks, then this is RTBC.

Although...I'm not sure if we should postpone this on the renaming of the cckfield plugins, since that will require this to be rerolled (or vice-versa, depending on which one gets in first).

  1. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -109,10 +109,12 @@ protected function createType($id) {
    +   * @param int|false $field_file_target_id
    +   *   The target ID of the file field.
    

    Should just be @param int, and the description should begin with (optional).

  2. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserTest.php
    @@ -190,6 +196,13 @@ public function testUser() {
    +        ->fields('ff', array('field_file_fid'))
    

    Nit: Should use [] instead of array().

jofitz’s picture

Changes made:

  1. Added (optional) at the beginning of the description.
  2. Used [] instead of array().

I have not changed @param int|false because more often than not the value will be false (as returned by fetchField() when there is no record).

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Gorgeous.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed eb271e5 to 8.3.x and 453f74e to 8.2.x. Thanks!

  • alexpott committed b844712 on 8.3.x
    Issue #2673960 by Jo Fitzgerald, hussainweb, phenaproxima, mikeryan,...

  • alexpott committed 453f74e on 8.2.x
    Issue #2673960 by Jo Fitzgerald, hussainweb, phenaproxima, mikeryan,...

  • alexpott committed b844712 on 8.4.x
    Issue #2673960 by Jo Fitzgerald, hussainweb, phenaproxima, mikeryan,...

  • alexpott committed b844712 on 8.4.x
    Issue #2673960 by Jo Fitzgerald, hussainweb, phenaproxima, mikeryan,...

Status: Fixed » Closed (fixed)

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