Improve __debugInfo return type error message #5496
Conversation
Improvements: - include name of the class - align the camelCase with documentation - the method implementation does allow a null return, so that is included in the message I've also refactored the tests, improving feature contributions.
| @@ -0,0 +1,18 @@ | |||
| --TEST-- | |||
| Testing __debugInfo() magic method with invalid returns inside anon-class | |||
There was a problem hiding this comment.
Note for reviewers: I've added this test to ensure that the error message is Method C::__debugInfo() ... instead of Method class@anonymous::__debugInfo() ..., as the "first" error is in the C class, by returning an anon-class.
|
Question: should we drop all the variant tests, leaving |
| @@ -169,7 +169,7 @@ ZEND_API HashTable *zend_std_get_debug_info(zend_object *object, int *is_temp) / | |||
| return ht; | |||
| } | |||
|
|
|||
| zend_error_noreturn(E_ERROR, ZEND_DEBUGINFO_FUNC_NAME "() must return an array"); | |||
| zend_error_noreturn(E_ERROR, "Method %s::__debugInfo() must return an ?array", ZSTR_VAL(ce->name)); | |||
There was a problem hiding this comment.
Return value of %s::__debugInfo() must be of type ?array, %s returned would be more in line with other return type error messages
There was a problem hiding this comment.
I don't remember having seen an error message with a nullable value represented like the syntax, is that something we do in other places? If not then I would prefer ... of type array or null... (or whichever we error text we use elsewhere to be consistent)
There was a problem hiding this comment.
@KalleZ It is something we changed for PHP 8. We are now displaying nullable types everywhere like this.
There was a problem hiding this comment.
@kocsismate awesome then, no objections on my end then! Thanks
There was a problem hiding this comment.
@kocsismate Align with what messages, ZPP?
Because all the "magical" ones do not mention it:
- __serialize(): https://3v4l.org/HLiTj
- __toString(): https://3v4l.org/Dbe6G
- __debugInfo(): https://3v4l.org/0EEPh
- __sleep(): https://3v4l.org/dH96A
Also, I'd need to figure out how to populate %s returned. Maybe Z_TYPE(retval) does the job?
There was a problem hiding this comment.
@carusogabriel Yes, it's ZPP. I think the other messages should (but at least could) be converted to that format later.
You can use zend_zval_type_name(retval) to get the string representation of the type of a zval. :)
|
@sgolemon Why does |
I'd have to dig through list emails to recall. I want to say that I wanted arrays only, but someone disagreed and I couldn't be bothered to fight for it. I would say that it's certainly been around long enough that disallowing it now needs a deprecation cycle leading up to 9.0. I'm also not bothered by having |
|
@sgolemon Thanks for the reply. I'll send an email to internals this week proposing this minor change. Maybe we don't need an RFC for that, but let's see. |
Improvements:
Following d906eb2, founded by @kocsismate on #4177.
I've also refactored the tests, removing unnecessary duplication.