Problem/Motivation

Migrating user pictures has been notoriously buggy for people.

Proposed Resolution

User pictures should always be migrated the same way, regardless of Drupal version -- by looking up the fid of the migrated picture.

Remaining Tasks

We (the royal "we", man, the editorial) need to write a patch.

API Changes

The d6_user_picture plugin will be removed.

UI Changes

Nope.

CommentFileSizeAuthor
#21 2484405-21.patch3.46 KBphenaproxima
#18 interdiff-2484405-17-18.txt1.53 KBphenaproxima
#18 2484405-18.patch6.97 KBphenaproxima
#17 interdiff-2484405-15-17.txt784 bytesphenaproxima
#17 2484405-17.patch6.9 KBphenaproxima
#15 2484405-15.patch6.36 KBphenaproxima
#12 interdiff-1-12.txt5.11 KBquietone
#12 2484405-12.patch2.82 KBquietone
#8 interdiff-1-8.txt4.02 KBquietone
#8 2484405-8.patch1.73 KBquietone
#1 2484405-1.patch2.94 KBphenaproxima

Comments

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 2484405-1.patch2.94 KB

Hey, look what the cat dragged in!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: 2484405-1.patch, failed testing.

Status: Needs work » Needs review

phenaproxima queued 1: 2484405-1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2484405-1.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
commented

Having trouble following the code on this failure. Here is the trace for anyone investigating:

Fatal error: Call to a member function getFileUri() on null in /home/jgilliland/public_html/d8/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php on line 318

Call Stack:
    0.0024     430288   1. {main}() /home/jgilliland/public_html/d8/core/scripts/run-tests.sh:0
    0.4151   15851048   2. simpletest_script_run_one_test() /home/jgilliland/public_html/d8/core/scripts/run-tests.sh:43
    0.4220   17499184   3. Drupal\simpletest\TestBase->run() /home/jgilliland/public_html/d8/core/scripts/run-tests.sh:644
   50.1798   92962904   4. Drupal\migrate_drupal\Tests\MigrateFullDrupalTestBase->testDrupal() /home/jgilliland/public_html/d8/core/modules/simpletest/src/TestBase.php:998
   59.8735  114837960   5. Drupal\migrate\MigrateExecutable->import() /home/jgilliland/public_html/d8/core/modules/migrate_drupal/src/Tests/MigrateFullDrupalTestBase.php:65
   60.5152  116641088   6. Drupal\migrate\Plugin\migrate\destination\EntityUser->import() /home/jgilliland/public_html/d8/core/modules/migrate/src/MigrateExecutable.php:287
   60.5152  116641088   7. Drupal\migrate\Plugin\migrate\destination\EntityContentBase->import() /home/jgilliland/public_html/d8/core/modules/migrate/src/Plugin/migrate/destination/EntityUser.php:93
   60.5198  116678984   8. Drupal\migrate\Plugin\migrate\destination\EntityContentBase->save() /home/jgilliland/public_html/d8/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php:77
   60.5198  116678984   9. Drupal\Core\Entity\Entity->save() /home/jgilliland/public_html/d8/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php:92
   60.5198  116678984  10. Drupal\user\UserStorage->save() /home/jgilliland/public_html/d8/core/lib/Drupal/Core/Entity/Entity.php:350
   60.5199  116679360  11. Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() /home/jgilliland/public_html/d8/core/modules/user/src/UserStorage.php:82
   60.5205  116694976  12. Drupal\Core\Entity\EntityStorageBase->save() /home/jgilliland/public_html/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:920
   60.5205  116695440  13. Drupal\Core\Entity\ContentEntityStorageBase->invokeHook() /home/jgilliland/public_html/d8/core/lib/Drupal/Core/Entity/EntityStorageBase.php:409
   60.5205  116695520  14. Drupal\Core\Entity\ContentEntityStorageBase->invokeFieldMethod() /home/jgilliland/public_html/d8/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:187
   60.6223  116699256  15. Drupal\Core\Field\FieldItemList->preSave() /home/jgilliland/public_html/d8/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:204
   60.6223  116699344  16. Drupal\Core\Field\FieldItemList->delegateMethod() /home/jgilliland/public_html/d8/core/lib/Drupal/Core/Field/FieldItemList.php:209
   60.6223  116699344  17. Drupal\image\Plugin\Field\FieldType\ImageItem->preSave() /home/jgilliland/public_html/d8/core/lib/Drupal/Core/Field/FieldItemList.php:248

FATAL Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test: test runner returned a non-zero error code (255).
Simpletest database and files kept and test exited immediately on fail so should be reproducible if you change settings.php to use the database prefix simpletest329981 and config directories in sites/simpletest/329981

Fatal on this code:

 $image = \Drupal::service('image.factory')->get($this->entity->getFileUri());
quietone’s picture

quietone commented

Spent some time debugging this and I agree with neclimdul that it is a bit funky to follow. But what I see is that the value returned from the user picture process plugin is the uid not the fid. And that the test, MigrateUserTest always has uid=fid (when there is a picture file), so the test will pass.

I can't get the process plugin to return the correct value. It feels like I am looking right at the solution and not seeing it.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2484405-8.patch1.73 KB
new interdiff-1-8.txt4.02 KB

To get the correct fid, I added a query to the process plugin. I don't know if it the best/preferred way to fix this but it does work locally. Is there a better way?

Status: Needs review » Needs work

The last submitted patch, 8: 2484405-8.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

@quietone: To get the correct fid, we use the migration process plugin, which looks up the destination ID created by a previous migration. Documentation here: https://www.drupal.org/node/2149801

quietone’s picture

quietone commented

@phenaproxima. Hmm. Not sure how the migration process plugin helps when the user picture file doesn't have an fid destination.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2484405-12.patch2.82 KB
new interdiff-1-12.txt5.11 KB

Right. Start over. There was an error in my test.

Fresh patch to start again.

Status: Needs review » Needs work

The last submitted patch, 12: 2484405-12.patch, failed testing.

quietone’s picture

quietone commented

Did some debugging.

The skip row process behaves differently than the user picture processor when there is no user picture file. Skip row returns an empty string whereas the user picture processor returns a NULL. When the empty string is returned the error that neclimdul reported occurs. But when running from HEAD with the user picture process that method (preSave) isn't executed.

Not sure where to fix this.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 2484405-15.patch6.36 KB

I've refactored this to be, like, sane. This patch introduces a single process plugin for migrating user pictures into an image field, retrieving the fid using the migration process plugin. My hope is that this will stabilize and standardize Migrate Drupal's approach to the notoriously finicky user picture migration.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: d6_user_picture processor is redundant » User pictures are not correctly migrated into image fields
Category: Task » Bug report
Issue summary: View changes
Priority: Normal » Major
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 2484405-17.patch6.9 KB
new interdiff-2484405-15-17.txt784 bytes

Deleted a bit of extraneous junk.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 2484405-18.patch6.97 KB
new interdiff-2484405-17-18.txt1.53 KB

@mikeryan pointed out that the plugin doesn't need to package the returned value as an image field -- the field system is smart enough to know what to do. But we do need to catch the MigrateSkipRowException that the migration process plugin may throw.

We can remove the plugin when #2487568: Process plugins should not be allowed to skip rows is fixed and use the migration process plugin directly.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Postponed

Postponed on #2560671: The Migration process plugin should not skip rows. Once that lands, we will not need a dedicated process plugin to wrap the migration plugin.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Postponed » Active

Unblocked. With #2560671: The Migration process plugin should not skip rows in, we don't need the user_picture plugin at all. Let's axe it -- patch forthcoming.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
StatusFileSize
new 2484405-21.patch3.46 KB

Ah. I sure do like shedding unnecessary LOC.

quietone’s picture

quietone commented

I can verify this works. Code looks good (but still getting to grips with the standards). Best of all, it is much easier to understand what is happening than before.

+1 RTBC !

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

As agreed on IRC

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: User pictures are not correctly migrated into image fields » User pictures do not need a dedicated process plugin
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Less code FTW!

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 887850e on 8.0.x 
    Issue #2484405 by phenaproxima, quietone, neclimdul: User pictures do...

Status: Fixed » Closed (fixed)

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