-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Allow calling array_merge / array_merge_recursive without arguments #4175
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
Conversation
This needs a discussion on internals. |
I will send a message to internals, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest retargeting this at 7.4. The UPGRADING entry can be moved to new features instead of BC changes. This is not really a BC break, as the function now accepts strictly more inputs.
@dtakken please link back to the internals discussion, and try to respond to comments left on the patch ... just so we know you're still working on it ;) |
0e57f2d
to
cef9608
Compare
Sorry, I left to discuss this on internals and did not notice the comments here until someone @mentioned me. :) The discussion on internals can be found here: https://externals.io/message/105736 |
Semantically this doesn't make any sense, concatenating nothing shouldn't work. |
There's a bunch of phar test failures on the opcache-enabled builds with:
I don't immediately see what's going wrong here, but something clearly is :/ |
The num_args does not include variadics, so a "zero-arg" function may accept additional arguments through that. No functions seem to be affected right now, but they will be after #4175.
The num_args does not include variadics, so a "zero-arg" function may accept additional arguments through that. No functions seem to be affected right now, but they will be after #4175.
Fixed the opcache bug in 31ce1cb, let's see if the build passes now... |
…al array_merge() parameters > array_merge() and array_merge_recursive() may now be called without any > arguments, in which case they will return an empty array. This is useful > in conjunction with the spread operator, e.g. array_merge(...$arrays). Refs: * https://github.com/php/php-src/blob/42cc58ff7b2fee1c17a00dc77a4873552ffb577f/UPGRADING#L276 * https://www.php.net/manual/en/function.array-merge.php * https://www.php.net/manual/en/function.array-merge-recursive.php * php/php-src#4175 * php/php-src@77cf3d7
The array_merge and array_merge_recursive functions require at least one argument. That requirement was relevant until version 5.6, because calling it without any arguments made no sense. But with argument unpacking syntax we can use the spread operator for merging multiple arrays:
Using array_merge or array_merge_recursive in this way requires checking that the list of arrays is not empty. Failing to do so may give you null in stead of an array, as well as a warning. This introduces a corner case that gives rise to bugs. A work-around like this appears to be popular:
The proposed change makes both array_merge and array_merge_recursive return an empty array when called without arguments, making the use of the spread operator a lot more convenient. A similar issue was fixed by ##3011 for the array_push and array_unshift functions in PHP 7.3. Back then, @nikic already hinted at fixing array_merge and array_merge_recursive in the same way, which is what this PR intends to do.
Just like with the change of array_push, changing array_merge causes a minor BC break for the return value. Using the spread operator to merge an empty list of arrays will now return an empty list in stead of returning null. The BC break for array_merge has slightly more impact though, because the result of the merge is provided by means of the return value while array_push modifies an argument.