In my migration process I have something like this:

  sortiment:
    plugin: static_map
    source: 'Sortiment 1'
    default_value: null
    map:
      order: 1
      collect: 2

I want this to set my field to N/A if no value is set in the "Sortiment 1" source field.

What happens is it skips the row.

I have located this in core/modules/migrate/src/Plugin/migrate/process/StaticMap.php, ln 43

      if (isset($this->configuration['default_value'])) {
isset() will return FALSE if testing a variable that has been set to NULL.

Is this intended behavior (to have a hidden skip row functionality (trap)? I'm happy to provide a patch ortherwise.

CommentFileSizeAuthor
#21 interdiff.txt763 bytesmikeryan
#21 default_value_null_in-2613178-21.patch1.77 KBmikeryan
#9 default_value_null_in-2613178-9.patch1.82 KBmikeryan
#9 default_value_null_in-2613178-9-FAIL.patch997 bytesmikeryan
#3 default_value_null_in-2613178-3.patch818 bytesmikeryan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miiimooo created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented
Version: 8.0.0-rc3 » 8.1.x-dev

Just ran into this myself, still there in 8.1.x.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented
Status: Active » Needs review
FileSize
default_value_null_in-2613178-3.patch818 bytes

Seems simple enough.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented
Issue tags: +Needs tests
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

It this a duplicate of #2692373: Value should be null when is produced skip process? That one has tests.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented

Nope, this one is purely a static_map issue, which that issue doesn't touch.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented 
+++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -35,7 +35,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+      if (array_key_exists('default_value', $this->configuration)) {

It is best to do this with both isset() || array_key_exists(). See http://www.zomeoff.com/php-fast-way-to-determine-a-key-elements-existanc...

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Needs work
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
default_value_null_in-2613178-9-FAIL.patch997 bytes
default_value_null_in-2613178-9.patch1.82 KB

Now with testy goodness!

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

Assuming this comes back green, this is RTBC.

The last submitted patch, 9: default_value_null_in-2613178-9-FAIL.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: default_value_null_in-2613178-9.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented

The failures don't appear to have anything to do with this change, requeueing.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Reviewed & tested by the community

@heddn, thanks for the link in #7. And to mikeryan for doing all the work.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

I never did see a clean failure. But assuming that the reque-ed test fails cleanly, +1 on RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/migrate/src/Plugin/migrate/process/StaticMap.php
@@ -35,7 +35,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+      if (isset($this->configuration['default_value']) || array_key_exists('default_value', $this->configuration)) {

I think this should just be https://www.drupal.org/files/issues/default_value_null_in-2613178-9.patch the OR is unnecessary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented

I think this should just be https://www.drupal.org/files/issues/default_value_null_in-2613178-9.patch the OR is unnecessary.

A mispaste? You mean we should just do the array_key_exists()?

Thanks.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

Hmm, isset is always a little tough in php. But me thinks that it is possible to provide a static value of NULL. I've actually done that in at least one case when I wanted to null out the destination of an entity reference. I think I need more description on why #9 seems incomplete.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@mikeryan yeah 

if (isset($this->configuration['default_value']) || array_key_exists('default_value', $this->configuration)) {

is the same as 

if (array_key_exists('default_value', $this->configuration)) {
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@heddn in every case that isset() returns TRUE array_key_exists() will also return TRUE. However array_key_exists() will also return TRUE if the value is NULL. Which is what we want here. By removing the || we make it simpler for a reviewer or ourselves in 6 months time to look at this code and work out what we were thinking.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Virtuoso Performance for North Studio commented
Status: Needs work » Needs review
FileSize
default_value_null_in-2613178-21.patch1.77 KB
interdiff.txt763 bytes

OK, back to the original version.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

@alexpott, the both together is a micro-optimization I use all the time. But it probably won't increase speed all *that* much. No blocker from me. Back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Yeah it is a super super micro-optimisation which shouldn't matter at all in a migration - there is going to be much slower things going on. Also in PHP7 the optimisation completely disappears. See https://ilia.ws/archives/247-Performance-Analysis-of-isset-vs-array_key_... - I've run that script on PHP7 and there's no difference between isset and array_key_exists.

@mikeryan - sorry I didn't notice the earlier discussion. I would have just committed the first patch.

Committed 4149ee4 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed ef39eed on 8.2.x 
    Issue #2613178 by mikeryan, heddn: default_value: null in static map...

  • alexpott committed 4149ee4 on 8.1.x 
    Issue #2613178 by mikeryan, heddn: default_value: null in static map...

Status: Fixed » Closed (fixed)

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