Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#35058 closed defect (bug) (fixed)

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

Reported by: jeffpyebrookcom's profile jeff@… Owned by: swissspidy's profile swissspidy
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Formatting Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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

Attachments (5)

Remove_for_loop_iteration__by_reference__to_avoid_fatal_php_error.patch (638 bytes) - added by jeff@… 9 years ago.
Patch to avoid PHP fatal error when recursing through map_deep
Remove_for_loop_iteration__by_reference__to_avoid_fatal_php_error1.patch (4.1 KB) - added by jeff@… 9 years ago.
Patch that handles objects as well as arrays
avoid_fatal_php_error.patch (3.9 KB) - added by jeff@… 9 years ago.
All pass by reference removed from recursive function
Clean_map_deep_function_without_pass_by_reference__without_object_casts.patch (4.3 KB) - added by jeff@… 9 years ago.
Patch without pass by reference, and without object casts
35058.diff (2.2 KB) - added by swissspidy 9 years ago.

Download all attachments as: .zip

Change History (23)

#1 @jeff@…
9 years ago

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.

Last edited 9 years ago by jeff@… (previous) (diff)

#2 @jeff@…
9 years ago

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 );
	}
}

#3 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.4.1

@jeff@…
9 years ago

Patch to avoid PHP fatal error when recursing through map_deep

#4 @jeff@…
9 years ago

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

#5 @jeff@…
9 years ago

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@…
9 years ago

Patch that handles objects as well as arrays

#6 @swissspidy
9 years ago

  • Keywords needs-patch added

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@…
9 years ago

All pass by reference removed from recursive function

#7 @jeff@…
9 years ago

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

#8 @swissspidy
9 years ago

@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.

#9 @jeff@…
9 years ago

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.

Last edited 9 years ago by jeff@… (previous) (diff)

#10 @jeff@…
9 years ago

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@…
9 years ago

Patch without pass by reference, and without object casts

#11 @jeff@…
9 years ago

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
9 years ago

#12 @swissspidy
9 years ago

  • Keywords has-patch has-unit-tests added; needs-patch removed
  • Owner set to swissspidy
  • Status changed from new to assigned

Thanks, the latest patch looks great!

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

#13 @jorbin
9 years ago

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

#14 @dd32
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#15 @dd32
9 years ago

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.

#16 @dd32
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#17 @dd32
9 years ago

In 36117:

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

See #35058.

#18 @dd32
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.