str_replace throws php error when source contains null

We had this issue pop up again in a different area. This time it was a field that isn't always populated. It comes in with a value of NULL when it isn't populated. Here is a patch. I added a check to verify whether the incoming value is a string or an array of strings, and if it isn't it will throw a MigrateSkipProcessException. I also added a unit test for it.

Slight change in approach; this just returns the none string value so that the next process in the chain can continue on. I misunderstood and thought this was what the MigrateSkipProcessException did. This also iterates through an array and acts aon all strings in the array, returning all non-string items as is.

This is looking good. Nice work adding the test.

We have a
+use Drupal\migrate\MigrateSkipProcessException;

that snuck in that does not seem to be used.

It might be good to add some comments in there to show reason for the checks. In a year it would be hard to infer why those are there.

+    if (!$this->multiple && !is_string($value)) {
         // This can't be processed by str_replace so pass it on through.
+      return $value;
+    }

+      foreach($value as $key => $item) {
+        if (is_string($item)) {
             // Replace individual item.
+          $value[$key] = $function($this->configuration['search'], $this->configuration['replace'], $item);
+        } else {
             // Can't replace so, so pass it on through.
+          $value[$key] = $item;
+        }
+      }

Using this patch is showing failures on migrations where there is some other process plugin that also expects/requires a string. So the patch is only a partial fix, but is allows a breaking change.

Looking at this a bit closer. php str_replace() handles TRUE and FALSE just fine. It is just the NULL that it chokes on. So I removed the part about bool in the issue description.

In testing, old php str_replace() would return a empty string if passed a NULL or a FALSE.
So to keep this running without breaking existing migrations, we would need this to do the following conversions.

incoming -> outgoing
NULL -> empty string ''
FALSE -> empty string ''
TRUE -> '1'
integers -> string equivalent

Ok, I will rework it to do the above.

Updated patch to allow for the above situations.

This is looking good overall.

This bit seems a little loose though

+        $item = $this->makeStringIfPossible($item);
+        if (is_string($item)) {
+          //Replace the item.
+          $value[$key] = $function($this->configuration['search'], $this->configuration['replace'], $item);
+        } else {
+          //Cannot replace so pass it through.
+          $value[$key] = $item;
+        }

What items are we expecting to pass through that would not be a string? The original version of php str_replace returned a string, so if we are passing through something that is not a string, that is going to lead to regressions.
I don't think we need all the specific cases for TRUE, NULL , FALSE, ... when simply casting them to a string would do it. Here are some quick tests of the various values in php:

php > print_r(is_string((string) TRUE));
1
php > print_r(is_string((string) FALSE));
1
php > print_r(is_string((string) NULL));
1
php > print_r(is_string((string) 27));
1
php > print_r(is_string((string) 27.556));
1
php > print_r(is_string((string) "hello"));
1

All the individual cases become strings like we want them to.

Also it would be helpful if we follow Drupal patch guidelines → on including the comment number in the patch? It makes it much harder to reference patches when they all have the same file name.

Good point @swirt! Made the relevant changes as this simplifies things.

#10 is looking great @edmund.dunn, thank you for tackling this so quickly.

This should fix the tests.

I have run multiple migrations successfully with this patch in my local development environment.

The records with a null field that had been failing previously are passing once again during migration.

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