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.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

miiimooo created an issue. See original summary.

mikeryan’s picture

Version: 8.0.0-rc3 » 8.1.x-dev

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

mikeryan’s picture

Status: Active » Needs review
FileSize
818 bytes

Seems simple enough.

mikeryan’s picture

Issue tags: +Needs tests
heddn’s picture

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

mikeryan’s picture

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

heddn’s picture

+++ 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

Status: Needs review » Needs work
mikeryan’s picture

heddn’s picture

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

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

quietone’s picture

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

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

alexpott’s picture

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

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

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

@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

@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

Status: Needs work » Needs review
FileSize
1.77 KB
763 bytes

OK, back to the original version.

heddn’s picture

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

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.