Skip to content

Promote warnings to errors in array_push() #4585

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

Girgias
Copy link
Member

@Girgias Girgias commented Aug 21, 2019

Split from #4566

@@ -3168,8 +3168,8 @@ PHP_FUNCTION(array_push)

if (zend_hash_next_index_insert(Z_ARRVAL_P(stack), &new_var) == NULL) {
Z_TRY_DELREF(new_var);
php_error_docref(NULL, E_WARNING, "Cannot add element to the array as the next element is already occupied");
RETURN_FALSE;
zend_throw_error(NULL, "Cannot add element to the array as the next element is already occupied");
Copy link
Member

Choose a reason for hiding this comment

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

So, this is actually a pretty generic warning, in particular also used by $foo[] = $x, see

zend_error(E_WARNING, "Cannot add element to the array as the next element is already occupied");
. I think whatever we do here, we should have a consistent behavior between $foo[] = $x and array_push($foo, $x).

@Girgias Girgias force-pushed the push-array-warnings2error branch from 59b3ab9 to 8f780ab Compare August 26, 2019 15:53
@Girgias
Copy link
Member Author

Girgias commented Aug 26, 2019

Appveyor failure seems unrelated.

@Girgias Girgias force-pushed the push-array-warnings2error branch from 2a753b9 to 7680d5b Compare August 29, 2019 21:38
@Girgias
Copy link
Member Author

Girgias commented Aug 29, 2019

This probably needs to wait the result of the Reclassifying Engine Warnings RFC.

@cmb69
Copy link
Member

cmb69 commented Sep 29, 2019

That RFC has been accepted (this issue should trigger an Error exception), so we can proceed with this PR. :)

@nikic
Copy link
Member

nikic commented Oct 2, 2019

Merged as 1ca4ab0.

@nikic nikic closed this Oct 2, 2019
@Girgias Girgias deleted the push-array-warnings2error branch November 20, 2019 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants