Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use native array_replace_recursive #749

Merged
merged 2 commits into from
Oct 23, 2022

Conversation

Alkarex
Copy link
Contributor

@Alkarex Alkarex commented Oct 9, 2022

Since PHP 5.3+, there is a native equivalent to the SimplePie Misc function to merge arrays: https://php.net/array-replace-recursive
Improvement of #268

Since PHP 5.3+, there is a native equivalent to the SimplePie Misc function to merge arrays:
https://php.net/array-replace-recursive
Improvement of simplepie#268
@jtojnar
Copy link
Contributor

jtojnar commented Oct 9, 2022

Note that the functions are not equivalent. For example:

<?php

require_once __DIR__ . '/vendor/autoload.php';

use SimplePie\Misc;

var_dump(Misc::array_merge_recursive(['a' => 'b'], ['a' => ['b']]));
var_dump(array_replace_recursive(['a' => 'b'], ['a' => ['b']]));

prints

array(1) {
  ["a"]=>
  string(1) "b"
}
array(1) {
  ["a"]=>
  array(1) {
    [0]=>
    string(1) "b"
  }
}

Although the behaviour of array_replace_recursive makes more sense to me for a “replace” function and there probably won’t be arrays in the values we are merging.

@Alkarex
Copy link
Contributor Author

Alkarex commented Oct 9, 2022

Good catch :-) I do not think this was the intended behaviour of Misc::array_merge_recursive() and was probably an implementation bug (cf. #268 ). The native array_replace_recursive() indeed handles arrays better.

public static function array_merge_recursive($array1, $array2)
{
foreach ($array2 as $key => $value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For BC reasons I would leave the Misc::array_merge_recursive() method untouched. I think that it is enough if we mark it as deprecated and no longer use it internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@pull-request-size pull-request-size bot added size/XS and removed size/S labels Oct 9, 2022
@@ -199,6 +199,9 @@ public static function fix_protocol($url, $http = 1)
return $url;
}

/**
* @deprecated since SimplePie 1.7.1, use PHP native array_replace_recursive() instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also mention that the behaviour is slightly different in case someone really heeds this recommendation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's necessary. The developer must always check if and how one can use an offered alternative.

@mblaney mblaney merged commit 258a1bb into simplepie:master Oct 23, 2022
@Alkarex Alkarex deleted the array_replace_recursive branch October 23, 2022 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants