Unable to migrate D7 User cck fields

Is this patch missing the actual process plugin itself?

OK, great we just need some tests then.

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?

Fixing two really small nitpicks.

Assigning to myself for review.

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

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:

EDIT: Punches it in the face. Stupid typos.

#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

Assigning to myself for review and shepherding to commit.

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

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.

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.

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

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.

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

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.

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

Thanks!

Self-assigning for review.

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

Gorgeous.

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