Why are SourceMigrations skipped with "If any source keys are empty, skip this set"?

In migration.inc there is code that will skip a source migration mapping for multiple keys array, if a single value is empty. I guess this is not intended.

So the source igration can not be retrieved, when I got a set, like:

  • street: My Street
  • street2: ""
  • zip: 30159
  • city: Hannover

I guess the intended behaviour is, to skip an element, when ALL values are empty - and not, like it is currently, to skip it if any is empty.

CommentFileSizeAuthor
#4 empty_key_value-1831940-4.patch676 bytesmikeryan
#1 multi_source_key_migration-11831940-1.patch786 bytesderhasi

Comments

derhasi’s picture

derhasi commented
Status: Active » Needs review
StatusFileSize
new multi_source_key_migration-11831940-1.patch786 bytes

I wrote a small patch to fix that.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Postponed (maintainer needs more info)

No, it is intended - the key here is serving as a primary key on the source, and all elements of the key must not be null. An address, as in your example, doesn't seem like a reliable way to uniquely identify a row - is there no combination of non-NULL values in your source data that can serve as a proper key?

derhasi’s picture

derhasi commented
Status: Postponed (maintainer needs more info) » Needs review

@mikeryan, "" is NOT NULL. I'ts just an empty text

If you want to skip the step where any key is NULL, then we should use !isset() instead of !empty($value) || $value === 0 || $value === '0'. This would make sense to me.

To get back to your address question. No there unfortunately is no other way. In some cases even an empty value (not NULL) should be a valid (part of a) key.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
StatusFileSize
new empty_key_value-1831940-4.patch676 bytes

This seems a bit simpler, does it work for you?

Thanks.

derhasi’s picture

derhasi commented

Cannot test it right now, but seems fine to me.

pwolanin’s picture

pwolanin commented

I think !isset() is preferred, from the comments on php.net

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Needs review » Fixed

Committed (with !isset) to the wizard_api branch, thanks.

Status: Fixed » Closed (fixed)

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

imiksu’s picture

imiksu
Location Finland
commented
Issue summary: View changes
Status: Closed (fixed) » Active

Hi! I've been struggling with one of my migrations after updating from 2.5 to 2.8 and turns out that this patch has created an regression.

I have an migration for hierarchial taxonomy terms and I use parent as one of my field mappings together with ->sourceMigration($this->getMachineName()).

Unluckily my source has an an foreign ID "0" which is the ID of a root term. So, after my upgrade all my second level terms are now considered as root terms. Is there way to improve this issue's patch by covering that case too without breaking the original fix?