Extend deprecation notices to is_callable($foo) and callable $foo #8823
Conversation
| @@ -3500,7 +3500,7 @@ ZEND_API void zend_release_fcall_info_cache(zend_fcall_info_cache *fcc) { | |||
| } | |||
| } | |||
|
|
|||
| static zend_always_inline bool zend_is_callable_check_func(zval *callable, zend_execute_data *frame, zend_fcall_info_cache *fcc, bool strict_class, char **error) /* {{{ */ | |||
| static zend_always_inline bool zend_is_callable_check_func(zval *callable, zend_execute_data *frame, zend_fcall_info_cache *fcc, bool strict_class, char **error, bool suppress_deprecation) /* {{{ */ | |||
There was a problem hiding this comment.
Maybe put the suppress_deprecation parameter before error, since the latter is an out-parameter.
There was a problem hiding this comment.
But that would be inconsistent with existing zend_is_callable_check_class (ref: ee510ee#diff-210c47b94a106559d6926f51c60d8c0d39f996738af3fe79e983a409d87a6b45L3294-R3294)?
There was a problem hiding this comment.
Indeed, that's why I initially put it where I did; however, I think both functions are entirely internal, so I'm tempted to rearrange both, as out params on the end does feel sensible.
There was a problem hiding this comment.
Then note: the parameter currently before error, strict_class, is "in" here but "out" there
There was a problem hiding this comment.
Well, maybe then leave this as is now.
58f4b87 to
10cce40
Compare
There was a problem hiding this comment.
Thank you for the PR! This looks generally good to me, although I'm somewhat concerned regarding the "necessity" to pass IS_CALLABLE_SUPPRESS_DEPRECTATIONS to zend_is_callable_ex() what might require changes to many extensions. At the very least that would need a note in UPGRADING.INTERNALS, but I wonder whether we shouldn't invert the flag, i.e. have something like IS_CALLABLE_RAISE_DEPRECATIONS. Would that make sense?
|
@cmb69 My hunch - without a lot of deep thought - is that most extensions would want to trigger the deprecation, for the same reason that The only case where you don't want it is if you're calling zend_is_callable_ex (or one of its callers, like zend_fcall_info_init) on the same variable more than once, and want to avoid duplicating the message; but that feels like it ought to be rare. If you're just accepting a single callable, you'll probably use arginfo (which doesn't raise the deprecation) then ZPP macros (which do) at which point you've got a variable you know is callable and don't need to call it again. |
|
@IMSoP, ah, thanks for the explanation. Makes perfect sense. So we should just have a short note in UPGRADING.INTERNALS about that. |
95a5044 to
8ce7450
Compare
UPGRADING.INTERNALS
Outdated
| zend_fcall_info_init, will issue deprecation notices if passed values which | ||
| are deprecated (see main UPGRADING notes). To suppress the notice, e.g. to | ||
| avoid duplicates when processing the same value multiple times, pass or add | ||
| IS_CALLABLE_SUPPRESS_DEPRECTATIONS to the check_flags parameter. |
There was a problem hiding this comment.
typo "DEPRECTATIONS"
| IS_CALLABLE_SUPPRESS_DEPRECTATIONS to the check_flags parameter. | |
| IS_CALLABLE_SUPPRESS_DEPRECATIONS to the check_flags parameter. |
There was a problem hiding this comment.
Oops, well spotted! Fixed.
Implements https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices so that uses of "self" and "parent" in is_callable() and callable type constraints now raise a deprecation notice, independent of the one raised when and if the callable is actually invoked. A new flag is added to the existing check_flags parameter of zend_is_callable / zend_is_callable_ex, for use in internal calls that would otherwise repeat the notice multiple times. In particular, arguments to internal function calls are checked first based on arginfo, and then again during ZPP, so the former suppresses the deprecation notice. Some existing tests which raised this deprecation have been updated to avoid the syntax, but the existing version retained for maximum regression coverage until it is made an error. With thanks to Juliette Reinders Folmer for the RFC and initial investigation.
8ce7450 to
1ef6e28
Compare
Implements https://wiki.php.net/rfc/partially-supported-callables-expand-deprecation-notices
so that uses of "self" and "parent" in is_callable() and callable
type constraints now raise a deprecation notice, independent of the
one raised when and if the callable is actually invoked.
A new flag is added to the existing check_flags parameter of
zend_is_callable / zend_is_callable_ex, for use in internal calls
that would otherwise repeat the notice multiple times. In particular,
arguments to internal function calls are checked first based on
arginfo, and then again during ZPP, so the former suppresses the
deprecation notice.
Some existing tests which raised this deprecation have been updated
to avoid the syntax, but the existing version retained for maximum
regression coverage until it is made an error.
With thanks to Juliette Reinders Folmer for the RFC and initial
investigation.