Add test for NULL source value to test of static_map process plugin

Here is my patch.
Was updated test and added fix for pass all tests.

My next try

I hope that tests now will be passed.
But I'm not sure in 100% that it the best way. So need review and other opinion.

Sorry, I had attached the patch without test in my previous comment.

Assigning to myself to review this coming weekend.

Lets not just plow forward with an empty string here. Let's throw the same exception on NULL that we do on an empty array. Can we just replace

    $new_value = $value;
    if (is_array($value)) {
      if (!$value) {
        throw new MigrateException('Can not lookup without a value.');
      }
    }
    else {
      $new_value = [$value];
    }
    $new_value = NestedArray::getValue($this->configuration['map'], $new_value, $key_exists);

with

    if (($value ?? []) === []) {
      throw new MigrateException('Can not lookup without a value.');
    }
    $new_value = NestedArray::getValue($this->configuration['map'], is_array($value) ? $value : [$value], $key_exists);

And update the tests to check for the exception being thrown on NULL?

@mikelutz, thanks for your review!
You are right, that's way is much better.
In this patch implemented this approach.
Please review

In the docs we have this relating to the case where a float(-ish thing) is passed in as $value. Should we also throw an exception then?

Mapping from a string which contains a period is not supported. A custom
process plugin can be written to handle this kind of a transformation.
Another option which may be feasible in certain use cases is to first pass
the value through the machine_name process plugin.

1.1 (or 500.00) can be a valid array key if it's a string, but will trigger array_key_exists() warning if it's received as a float, and 500.00 will look like '500' at this point, making you drupalwtf at why array_key_exists() is complaining that an apparent int is not an int (it's a double!)

Even if we don't include that behaviour in this change, we could possibly document not using null / empty string inputs also.

It took a bit to see that the work here is attempting to fix what is thought to be a problem with the processing of NULL in the static map process plugin. That is not the error reported in the IS but the title of this issue suggests it is. xurizaemon, #16, was on the right track. There are potentially two problems

1. Does the static map process NULL source values correctly
2. Preventing the error reported in the IS, "array_key_exists(): The first argument should be either a string or an integer NestedArray.php:72".

For now, let's defer the issue about the processing of the NULL, whatever static map is doing it is working and folks are expected the behavior to remain the same. Proof of that is patch #14 which alters the processing of NULL and then we have test failures.

That leaves the error from array_key_exists. For that, we need a fail patch, one that generates the error reported in the IS, which is not is not generated with NULL as the source value. And then, I think a try/catch around NestedArray.

Add tag.

Now back to the processing of NULL. When the source value is NULL it is changed to [NULL] before being passed to NestedArray::getValue() and will not cause an error or a warning. On return the array_key will not exist and then if the default_value is set that is returned. If there is no default value and bypass is empty then a MigrateSkipRowException is thrown, other wise NULL is returned.

Does anyone think that logic needs to change?

So first, let me say I was wrong about NULL, I did some testing, and NULL seems to be converted into an empty string, which is a valid array key and is not throwing the warning. Passing a float, array or object to array_key_exists does trigger the warning, and there are various inputs that can cause that to happen, all of them wrong.

I don't like the float test here. The reason that it works if the value is found, but not if it isn't is because the array key is converted to an integer on assignment and lookup. $configuration['map'][1.2] = 'float'; actually sets $configuration['map'][1] = 'float', and nested array checks for $configuration['map'][1.2] and php returns $configuration['map'][1], which exists, so array_key_exists is never called. in the second case, [1] doesn't exist so array_key_exists is called throwing the warning. This is a whole mess of potentially buggy things that can happen, and php is not going to help us find them. I think we should explicitly throw an exception if we get a float value.

Finally, the try/catch is actually not helpful here. The MigrateException only gets thrown in the test because the errorhandler in phpunit converts the warning to an \Exception. in the real world, It's a php warning that is thrown, not an \Exception, so the warning wouldn't be caught anyway.
You could, of course catch a \Throwable instead of an \Exception, but it's a bad smell to me to try to catch errors/warnings after they are thrown. It's much better to detect the situations that can trigger them in advance and throw an \Exception before it ever gets to the point of throwing php error or warning.

So, what is to be done here? It looks like we go back and add a test for NULL as a source and add documentation. There is already an issue for the documentation, maybe we can combine the two?

I started from the patch in #11, added a new test for a valid NULL source and added the documentation from the related issue. There is no interdiff because this is starting over.

Well, that was a complete mess. Ignore #26

Based on the comment in #24, this patch starts from the patch in #11, added a new test for a valid NULL source. No interdiff because this is starting over.

Since this no longer changes any existing behaviour and only adds extra test coverage, I'm gonna change this to a task.

This looks good and the tested behaviour matches the the doc block example added in #2941323: StaticMap should document how/whether it handles source values of NULL, TRUE, FALSE

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!