Skip to content

Remove redundant warning in array_push() and array_unshift() #3011

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

Closed
wants to merge 1 commit into from

Conversation

timurib
Copy link
Contributor

@timurib timurib commented Jan 7, 2018

The array_push and array_unshift functions require at least 2 arguments. Requirement about second argument was relevant until version 5.6, because the code array_push($array) really does not make sense. But with argument unpacking syntax we can use spread operator for array union:

array_push($array, ...$values);
array_unshift($array, ...$values);

If the addition list length is not predefined then we should explicitly check the case when this list is emptiness to avoid the warning message, or prepend the expression with @-operator. Actually the functions work well with single argument, they just do not change it. The warning is completely redundant and explicit checking negates the benefit (code size reducing) gained with the spread operator.

For example, relevant array methods push and unshift in the JavaScript work this way:

"use strict";
var array = [1,2,3]
var values = []
array.push(...values)
array.unshift(...values)
// no error messages, both methods just return 3, array is still [1,2,3]

Adding multiple elements to an array is a very common operation. This patch just will make it a little more convenient.

Unfortunately, it may cause the minor BC break for return value. Currently both functions returns NULL when second argument is not passed. With this patch in this case they will return the length of array passed as first argument. Do I need to create RFC for such a small change?

@php-pulls
Copy link

Comment on behalf of cmb at php.net:

Thanks! Applied via f7f4864 (also added a note in UPGRADING and fixed the respective arginfos).

@php-pulls php-pulls closed this Mar 25, 2018
salathe pushed a commit to salathe/phpdoc-en that referenced this pull request Mar 25, 2018
Cf. <php/php-src#3011>

git-svn-id: https://svn.php.net/repository/phpdoc/en/trunk@344569 c90b9560-bf6c-de11-be94-00142212c4b1
LawnGnome pushed a commit to LawnGnome/phpdoc-en that referenced this pull request Mar 25, 2018
Cf. <php/php-src#3011>

git-svn-id: https://svn.php.net/repository/phpdoc/en/trunk@344569 c90b9560-bf6c-de11-be94-00142212c4b1
svn2github pushed a commit to svn2github/phpdoc_en that referenced this pull request Mar 29, 2018
Cf. <php/php-src#3011>

git-svn-id: http://svn.php.net/repository/phpdoc/en@344569 c90b9560-bf6c-de11-be94-00142212c4b1
@bolknote
Copy link
Contributor

Very strange. Why did not you use other function instead of these? array_merge for example?

@timurib
Copy link
Contributor Author

timurib commented Apr 27, 2018

@bolknote, yes, of course, array_merge can be used. But when you don't create a new array, but mutate the original one, the array_push is more straightforward way to do this (imho).

Anyway, this commit don't change the actual behavior - the array_push already worked in this manner, it's just removes the warning message for this case.

@bolknote
Copy link
Contributor

bolknote commented Apr 27, 2018

@timurib

this commit don't change the actual behavior

Of course I got it. :) But I still think that you can do it without this change.

@nikic
Copy link
Member

nikic commented Apr 27, 2018

Speaking of array_merge, we should also allow zero arguments there as well. I've seen people write array_merge([], ...$arrays) quite a few times...

salathe pushed a commit to salathe/phpdoc-en that referenced this pull request Sep 3, 2020
Cf. <php/php-src#3011>

git-svn-id: https://svn.php.net/repository/phpdoc/en/trunk@344569 c90b9560-bf6c-de11-be94-00142212c4b1
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.

5 participants