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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (23)
#2
@
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 ); } }
#4
@
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
@
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 );
}
}
#6
@
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)
#7
@
9 years ago
Makes sense. All of the pass by reference optimization attempts have been removed in the most recent patch.
#8
@
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
@
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.
#10
@
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', ), )
#11
@
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; }
#12
@
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.
#16
@
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.
Changing
to
Avoids the fatal error. But I am not sure I grasp the original intent of changing the $item to be by reference.