Problem/Motivation

Value should be null when is produced skip process.
In this example the migration plugin throw a MigrateSkipProcessException keeping the value empty so set to destination property in:

      // No plugins or no value means do not set.
      if ($plugins && !is_null($value)) {
        $row->setDestinationProperty($destination, $value);
      }

This produce: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'field_project_name_target_id' because is a EntityReference.

Given the process configuration in the migration is:

process:
  field_project_name:
    -
      plugin: migration
      migration: project_name
      no_stub: true
      source: project_name

and project_name = ''

Proposed resolution

set a value = NULL in processRow when is produce skip process.
This avoid additional code:

      plugin: static_map
      bypass: true
      map:
        '': null

Remaining tasks

Write a patch.

Comments

edysmp created an issue. See original summary.

edysmp’s picture

edysmp’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

heddn’s picture

FileSize
2.17 KB
1.08 KB

I've reviewed the failing testing. In this case, it seems to return a '0' back from the migration process plugin because the pid for the parent comment is 0 in D6 and is therefore empty. I agree that returning NULL is a more acceptable approach. There aren't entity_ids in D6/D7 that allow the usage of '0' as a valid entity_id. To add to that argument, we already set the precedent to return NULL in the case of 'no_stub'. So I've fixed the tests.

benjy’s picture

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -383,6 +383,7 @@ public function processRow(Row $row, array $process = NULL, $value = NULL) {
+              $value = NULL;

This one doesn't make sense because I see this, right afterwards.

$value = $new_value;

We could set the value to NULL in the Migration process plugin if we're sure that's the right behaviour in all cases? Otherwise, I've seen this handled from the migration side of things in the yml with a default value, which is a little more granular.

heddn’s picture

Re #6: I see that now too. That should probably be a $new_value[] = NULL;

A default value or a static_map both fix this. But also changing the result to NULL does too. And it is one less thing to need to troubleshoot when your migration fails. How many times does the referenced migration (it's gotta be through an ER field) ever return a valid value of 0 for a remote entity_id? Never. Which is why 'no_stub' already sets NULL.

heddn’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
627 bytes

Status: Needs review » Needs work

The last submitted patch, 8: drupal-MigrateSkipProcessException_null-2692373-8.patch, failed testing.

heddn’s picture

Version: 8.0.x-dev » 8.2.x-dev
Status: Needs work » Needs review
heddn’s picture

mikeryan’s picture

cmanalansan’s picture

Working with @willwh on this

rakesh.gectcr’s picture

I have rerolled the patch.

Status: Needs review » Needs work

The last submitted patch, 14: 2692373-14.patch, failed testing.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
2.51 KB
mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

catch’s picture

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

Patch looks fine but doesn't this need a change record?

mikeryan’s picture

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

Change record added (not sure I should set to RTBC if I wrote it, but let's see...)

The last submitted patch, 14: 2692373-14.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 17b2166 and pushed to 8.1.x and 8.2.x. Thanks!

Added to both branches becuase migrate is experimental and keeping the behaviour the same makes things simpler.

  • alexpott committed 31e613b on 8.2.x
    Issue #2692373 by heddn, rakesh.gectcr, edysmp, mikeryan, benjy: Value...

  • alexpott committed 17b2166 on 8.1.x
    Issue #2692373 by heddn, rakesh.gectcr, edysmp, mikeryan, benjy: Value...

Status: Fixed » Closed (fixed)

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

boromino’s picture

Status: Closed (fixed) » Needs work

Reopening it because the taxonomy term hierarchy in 8.1.2 and 8.2.x-dev doesn't get migrated correctly. Hence the taxonomy reference fields are displayed empty on node edit forms, because the corresponding database query makes an inner join on the taxonomy_term_hierarchy table. And the vocabulary term lists are empty, too.

heddn’s picture

Status: Needs work » Closed (fixed)

re #25: Please open a follow-up issue to discuss. And link it back to here. It is best to leave closed issues in a closed status.