Problem/Motivation

It's an edge case, but occasionally a user picture file ID will be 0 in the source database. This wreaks havoc with the d7_user migration, because it's not defending against invalid data coming from the source database.

Proposed Resolution

d7_user should ignore the user_picture field if it's empty in the database. This can be done with the skip_on_empty process plugin running before the migration plugin.

Remaining Tasks

Fix the migration and add test coverage. Then review and commit the patch.

API/UI Changes

None.

CommentFileSizeAuthor
#7 2562083-7.patch2.57 KBphenaproxima
#6 2562083-6-FAIL.patch877 bytesphenaproxima
#4 2562083-4.patch620 bytesphenaproxima
#2 2562083-2.patch1.23 KBphenaproxima

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
StatusFileSize
new 2562083-2.patch1.23 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: +Needs tests
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Title: User migrations should skip over user picture if empty » D7 user migration should skip over user picture if empty
Issue summary: View changes
StatusFileSize
new 2562083-4.patch620 bytes

Due to quirks in the way properties are processed through their individual pipelines, it looks like d6_user needs no changes, but d7_user does.

Why?

In Drupal 6, {users}.picture is either going to be a string (file path) or NULL. But the value entering the process pipeline is never empty, because for Drupal 6 the user picture is looked up by user ID, and all migrated users will have non-empty IDs. If the Migration process plugin fails the lookup, it implicitly returns NULL, and that is the value the destination property receives. This causes no problems for the field system because NULL field values are harmlessly ignored.

In Drupal 7, however, {users}.picture is either a number (file ID) or NULL, and the file is looked up by its fid. If the fid is 0 -- which shouldn't happen under normal circumstances, but it does happen in the data I've been testing with -- the Migration process plugin throws a MigrateSkipProcessException, and the destination property is assigned its initial value of 0. This gets passed into the field system and causes fatal errors.

The last submitted patch, 2: 2562083-2.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 2562083-6-FAIL.patch877 bytes

Here's a fail patch proving the problem -- the D7 MigrateUserTest dies with a fatal error.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: -Needs tests
StatusFileSize
new 2562083-7.patch2.57 KB

...and this passes :)

The last submitted patch, 6: 2562083-6-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2562083-7.patch, failed testing.

Status: Needs work » Needs review

phenaproxima queued 7: 2562083-7.patch for re-testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/user/migration_templates/d7_user.yml
@@ -25,9 +25,13 @@ process:
+    -
+      plugin: default_value
+      source: picture
+      default_value: null

Stumbled a bit here on how 0 caused the default_value of NULL to set, since in D7 migrate defaultValue() was only applied if the incoming value was NULL. But, D8 default_value is a little more flexible - by default (hah!) a 0 or empty string counts as "empty" and triggers the default_value, you can set strict: true to get the old behavior and only default when receiving NULL.

Looks good!

webchick’s picture

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

Committed and pushed to 8.0.x. Thanks!

  • webchick committed d111904 on 
    Issue #2562083 by phenaproxima, mikeryan: D7 user migration should skip...

Status: Fixed » Closed (fixed)

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