PHP Fatal when map_deep tries to work on an object that has a property by reference

[jeff@…]

This is the error that happens when the thing being operated on has a property that is a "by reference" to something.

error type [1] An iterator cannot be used with foreach by reference file: \wp-includes\formatting.php line: 3897

Issue is new as of 4.4

[jeff@…]

Changing

foreach ( $value as &$item ) {

to

foreach ( $value as $item ) {

Avoids the fatal error. But I am not sure I grasp the original intent of changing the $item to be by reference.

[jeff@…]

This is my adjusted function...

/**
 * Maps a function to all non-iterable elements of an array or an object.
 *
 * This is similar to `array_walk_recursive()` but acts upon objects too.
 *
 * @since 4.4.0
 *
 * @param mixed    $value    The array, object, or scalar.
 * @param callable $callback The function to map onto $value.
 * @return The value with the callback applied to all non-arrays and non-objects inside it.
 */
function map_deep( $value, $callback ) {
	if ( is_array( $value ) || is_object( $value ) ) {
		foreach ( $value as $index => $item ) {
			$value[$index] = map_deep( $item, $callback );
		}
		return $value;
	} else {
		return call_user_func( $callback, $value );
	}
}

[jeff@…]

Patch to avoid PHP fatal error when recursing through map_deep

[jeff@…]

The patch offered above has failed my testing. If the foreach operates on an object the assignment using the array operator will fail with fatal php error error type: [1] Cannot use object of type [object name]as array file: wp-includes\formatting.php line: 3897

I will work through the alternatives

[jeff@…]

This replacement function seems much better ...

function map_deep( &$value, $callback ) {
        if ( is_array( $value ) || is_object( $value ) ) {
                foreach ( $value as $index => $item ) {
                        if ( is_object( $value ) ) {
                                $value->$index = map_deep( $item, $callback );
                        } else {
                                $value[$index] = map_deep( $item, $callback );
                        }
                }
                return $value;
        } else {
                return call_user_func( $callback, $value );
        }
}

[jeff@…]

Patch that handles objects as well as arrays

[swissspidy]

Both patches don't pass the unit tests.

The first one because it doesn't consider objects, the second one because only variables can be passed by reference (&$value triggers a fatal error)

[jeff@…]

All pass by reference removed from recursive function

[jeff@…]

Makes sense. All of the pass by reference optimization attempts have been removed in the most recent patch.

[swissspidy]

@jeff@… Thanks, that was quick!

Just two failing tests locally:

There were 2 failures:

1) Tests_Formatting_MapDeep::test_map_deep_should_map_each_property_of_an_object
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 stdClass Object (
-    'var0' => 'ababa'
+    'var0' => 'ababababa'
     'var1' => 'xbaba'
 )

2) Tests_Formatting_MapDeep::test_map_deep_should_map_each_array_property_of_an_object
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 stdClass Object (
-    'var0' => 'ababa'
+    'var0' => 'ababababa'
     'var1' => Array (...)
 )

At first glance I have no idea why the first var gets processed twice.

[jeff@…]

I'll dig into it a little bit.

We probably need to get an additional unit test into the mix that tests objects & arrays that refer to an object directly and by reference. These were the original data structures that brought the defect (PHP fatal error) to light.

I am also a little concerned that fixing the defect opens up an infinite recursion issue, although that was probably already present in the 4.3 and earlier implementations.

[jeff@…]

We have something that I have tested as follows:

$test_object = (object)array( 'var0' => 'ali', 'var1' => array( 'gaga' ) );
$test_array = array( 'var0' => 'varvar', 'var1' => array( 'googoo' ) );

function append_baba( $value ) {
	return $value . 'baba';
}

error_log( 'BEFORE ' . var_export( $test_object, true ) );

$output = map_deep( $test_object, 'append_baba'  );

error_log(  'AFTER ' . var_export( $output, true ) );



error_log( 'BEFORE ' . var_export( $test_array, true ) );

$output = map_deep( $test_array, 'append_baba'  );

error_log( 'AFTER ' . var_export( $output, true ) );

This is the test output:

[16-Dec-2015 17:07:20 America/New_York] BEFORE stdClass::__set_state(array(
   'var0' => 'ali',
   'var1' => 
  array (
    0 => 'gaga',
  ),
))

[16-Dec-2015 17:07:20 America/New_York] AFTER stdClass::__set_state(array(
   'var0' => 'alibaba',
   'var1' => 
  array (
    0 => 'gagababa',
  ),
))

[16-Dec-2015 17:07:20 America/New_York] BEFORE array (
  'var0' => 'varvar',
  'var1' => 
  array (
    0 => 'googoo',
  ),
)

[16-Dec-2015 17:07:20 America/New_York] AFTER array (
  'var0' => 'varvarbaba',
  'var1' => 
  array (
    0 => 'googoobaba',
  ),
)

[jeff@…]

Patch without pass by reference, and without object casts

[jeff@…]

This is the revised implementation:

function map_deep( $value, $callback ) {
	if ( is_array( $value ) ) {
		foreach ( $value as $index => $item ) {
			$value[$index] = map_deep( $item, $callback );
		}
	} elseif ( is_object( $value ) ) {
		$object_vars = get_object_vars( $value );
		foreach ( $object_vars as $property_name => $property_value ) {
			$value->$property_name = map_deep( $property_value, $callback );
		}
	} else {
		$value = call_user_func( $callback, $value );
	}

	return $value;
}

[swissspidy]

Thanks, the latest patch looks great!

I added two unit tests for values passed by reference in 35058.diff​.

[jorbin]

@swissspidy what do you need here to feel this will be committable?

[dd32]

In 36100:

Allow map_deep() to work with object properties containing a reference. Restores the previous behaviour of stripslashes_deep().

Props jeff@…, swissspidy.
See #22300, [35252].
Fixes #35058.

[dd32]

In 36101:

Allow map_deep() to work with object properties containing a reference. Restores the previous behaviour of stripslashes_deep().

Merges [36100] to the 4.4 branch.
Props jeff@…, swissspidy.
See #22300, [35252].
Fixes #35058.

[dd32]

Turns out I missed the test changes required after this.

(object) array( 'x' ) creates an "invalid" object in PHP5, objects with numeric keys are not returned by get_object_vars() but are if you cast it to an array. Of course under PHP4 & 7 those numeric keys are returned.

I'm going to switch out the tests to use (object) array( 'var0' => 'x' ) instead so it's compatible across the board.

[dd32]

In 36117:

Tests: After [36100] use an object style which is compatible with PHP5 get_object_vars().

See #35058.

[dd32]

In 36118:

Tests: After [36100] use an object style which is compatible with PHP5 get_object_vars().

Merges [36117] to the 4.4 branch.
See #35249.
Fixes #35058.