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

Make array parsing parameters error message consistency with ZPP failure #3429

Closed

Conversation

4 participants
@carusogabriel
Copy link
Member

carusogabriel commented Aug 5, 2018

As suggested in #76674, we could improve our parsing parameters error messages for array_ functions, by exposing what was passed, instead of an array.

I'm opening the PR for review before merging, as well asking if this should go for 7.3 or wait for 7.4 :)

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Aug 5, 2018

In my opinion, this can go into PHP-7.3. (Minor issue: I'd like more consistency with ZPP failure messages, but wouldn't know how to accomplish this.)

@carusogabriel

This comment has been minimized.

Copy link
Member Author

carusogabriel commented Aug 5, 2018

@cmb69 Good idea. Maybe:

-Argument #2 should be an array, int given
+array_map() expects parameter 2 to be array, int given

Would be tricky to catch the function name for shared internal functions, but there must be a way.

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Aug 5, 2018

Hmm, maybe array_map(): Expected parameter 2 to be array, int given is good enough?

@carusogabriel

This comment has been minimized.

Copy link
Member Author

carusogabriel commented Aug 5, 2018

@cmb69 Also, should we throw a php_error_docref or a zend_internal_type_error, relying on strict_types?

@carusogabriel carusogabriel force-pushed the carusogabriel:improve-array-error-messages branch from 458147d to a9d8af1 Aug 5, 2018

@carusogabriel carusogabriel changed the title Improve error messages when parsing parameters in array functions Make array parsing parameters error message consistency with ZPP failure Aug 5, 2018

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Aug 6, 2018

@carusogabriel Hmm, not sure about the BC break – some may not like that.

@carusogabriel

This comment has been minimized.

Copy link
Member Author

carusogabriel commented Aug 6, 2018

@cmb69 I've kept the php_error_docref. Should be ready for the review.

@@ -6152,7 +6152,7 @@ PHP_FUNCTION(array_map)

for (i = 0; i < n_arrays; i++) {
if (Z_TYPE(arrays[i]) != IS_ARRAY) {
php_error_docref(NULL, E_WARNING, "Argument #%d should be an array", i + 2);
php_error_docref(NULL, E_WARNING, "Expected parameter #%d to be an array, %s given", i + 2, zend_get_type_by_const(Z_TYPE(arrays[0])));

This comment has been minimized.

Copy link
@staabm

staabm Aug 6, 2018

Contributor

# should be stripped here as well, or the other wayarround.

I liked the # personally

This comment has been minimized.

Copy link
@carusogabriel

carusogabriel Aug 6, 2018

Author Member

I missed this one, thanks.

@carusogabriel carusogabriel force-pushed the carusogabriel:improve-array-error-messages branch from a9d8af1 to fd8abc9 Aug 6, 2018

carusogabriel referenced this pull request Aug 8, 2018

@carusogabriel carusogabriel force-pushed the carusogabriel:improve-array-error-messages branch from fd8abc9 to 0866695 Aug 8, 2018

@@ -3,7 +3,7 @@ Test array_diff_key() function : usage variation - Passing unexpected values to
--FILE--
<?php
/* Prototype : array array_diff_key(array arr1, array arr2 [, array ...])
* Description: Returns the entries of arr1 that have keys which are not present in any of the others arguments.
* Description: Returns the entries of arr1 that have keys which are not present in any of the others arguments.

This comment has been minimized.

Copy link
@cmb69

cmb69 Aug 8, 2018

Contributor

Please do not submit unrelated whitespace changes! I fully agree that there shouldn't be any trailing whitespace, but automatically removing these may make diffs harder to read, and may cause merge conflicts. If at all, such whitespace changes should go to a separate commit/PR.

This comment has been minimized.

Copy link
@carusogabriel

carusogabriel Aug 9, 2018

Author Member

@cmb69 Gonna revert these changes, sorry. We do need to find a way to trim all those whitespaces in tests, btw

@carusogabriel carusogabriel force-pushed the carusogabriel:improve-array-error-messages branch from 0866695 to d99991a Aug 9, 2018

@@ -4639,7 +4639,7 @@ static void php_array_intersect_key(INTERNAL_FUNCTION_PARAMETERS, int data_compa

for (i = 0; i < argc; i++) {
if (Z_TYPE(args[i]) != IS_ARRAY) {
php_error_docref(NULL, E_WARNING, "Argument #%d is not an array", i + 1);
php_error_docref(NULL, E_WARNING, "Expected parameter %d to be an array, %s given", i + 1, zend_get_type_by_const(Z_TYPE(args[i])));

This comment has been minimized.

Copy link
@nikic

nikic Aug 9, 2018

Member

Any reason to use zend_zval_type_name in some places and zend_get_type_by_const in others?

This comment has been minimized.

Copy link
@carusogabriel

carusogabriel Aug 9, 2018

Author Member

I used zend_zval_type_name when possible as was the function I found for such a task. But, as it needs a const zval *arg I got an error type when using here, for e.g.:

expected ‘const zval * {aka const struct _zval_struct *}’ but argument is of type ‘zval {aka struct _zval_struct}’

So I search a way to retrieve the type, and come up with zend_get_type_by_const + Z_TYPE. Is there a better way? :)

This comment has been minimized.

Copy link
@nikic

nikic Aug 9, 2018

Member

You can get a zval* from a zval by writing &args[i] instead of args[i].

This comment has been minimized.

Copy link
@carusogabriel

carusogabriel Aug 9, 2018

Author Member

Me and my C deficit 🤦‍♂️

@carusogabriel carusogabriel force-pushed the carusogabriel:improve-array-error-messages branch from a97f4f4 to 2fc004e Aug 9, 2018

@carusogabriel

This comment has been minimized.

Copy link
Member Author

carusogabriel commented Aug 9, 2018

@cmb69 May we merge this one?

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Aug 9, 2018

Maybe wait for a few days, so others are able to object. If nobody objects, it's fine to merge.

@carusogabriel carusogabriel force-pushed the carusogabriel:improve-array-error-messages branch from 2fc004e to 03a84b0 Aug 10, 2018

@carusogabriel carusogabriel force-pushed the carusogabriel:improve-array-error-messages branch from 03a84b0 to 55f9c59 Aug 11, 2018

@carusogabriel

This comment has been minimized.

Copy link
Member Author

carusogabriel commented Aug 19, 2018

If there's no objection on this one, I'll merge on August 27th, so we can include it in PHP 7.3 beta 3.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Aug 19, 2018

@carusogabriel Feel free to merge right away, I think enough time has passed for anyone to object.

@carusogabriel

This comment has been minimized.

Copy link
Member Author

carusogabriel commented Aug 20, 2018

Merged as efbf846, thanks!

@carusogabriel carusogabriel deleted the carusogabriel:improve-array-error-messages branch Aug 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.