[RFC] Add #[\DelayedTargetValidation] attribute
#18817
Conversation
037775b to
26ad0f2
Compare
1978ce1 to
82f556a
Compare
#[\DelayedTargetValidation] attribute#[\DelayedTargetValidation] attribute
|
Currently on vacation, so can't review, but I trust that you are capable of correctly moving error messages 😄 |
82f556a to
abbef91
Compare
abbef91 to
6b11fd2
Compare
| /** | ||
| * @strict-properties | ||
| */ | ||
| #[Attribute] |
There was a problem hiding this comment.
| #[Attribute] | |
| #[Attribute(Attribute::TARGET_ALL)] |
Spelling it out explicitly is probably a good idea for documentation purposes.
| internal_attr->flags = Z_LVAL(attr->args[0].value); | ||
| if (attr->argc == 0) { | ||
| // Apply default of Attribute::TARGET_ALL | ||
| internal_attr->flags = ZEND_ATTRIBUTE_TARGET_ALL; | ||
| } else { | ||
| internal_attr->flags = Z_LVAL(attr->args[0].value); | ||
| } |
There was a problem hiding this comment.
This would then become obsolete. I would suggest shipping this as an independent bugfix PR (or alternatively an assertion?).
| - Core: | ||
| . DelayedTargetValidation is an attribute that, when added, delays any errors | ||
| from *other* internal attributes about being applied to invalid targets from | ||
| compile-time to runtime. | ||
| RFC: https://wiki.php.net/rfc/delayedtargetvalidation_attribute |
There was a problem hiding this comment.
Not sure if we ever added attributes here. Makes sense, but I would just mention the class name + RFC and not the full explanation. Can you just commit the related change for NoDiscard to master?
There was a problem hiding this comment.
After sleeping on this, my preferred approach would be:
- Stop using
zend_error_noreturn()invalidatorin favor ofzend_throw_error()for target validation, instruct from UPGRADING.INTERNALS to do the same for extensions. - Use
EG(exception)fromzend_compile_attributes()to understand whether the validator succeeded. - Without
#[DelayedTargetValidation], promote to a fatal error withzend_exception_uncaught_error(). - With
#[DelayedTargetValidation], record whether the validator has succeeded and store it inzend_attribute.EG(exception)would have to be removed/freed. - When it failed, repeat the validation on
ReflectionAttribute::newInstance(). Skipping for successful validators prevents writing to locked shm. Validators should be idempotent, i.e. we should be able to rely on the fact that calling it again will also throw.
The benefit is that we don't need to handle this for every validator, and we don't need the ZEND_ATTRIBUTE_NO_TARGET_VALIDATION/ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION flags. Then, this feature should work mostly automatically.
WDYT?
| * attribute is applied to, attr->scope is just the overall class entry | ||
| * that the method is a part of. */ | ||
| if (ce == zend_ce_nodiscard && attr->on_property_hook) { | ||
| zend_throw_error(NULL, "#[\\NoDiscard] is not supported for property hooks");; | ||
| RETURN_THROWS(); |
There was a problem hiding this comment.
This hack should become obsolete with #19465.
| @@ -8422,6 +8446,10 @@ static zend_op_array *zend_compile_func_decl_ex( | |||
|
|
|||
| CG(active_op_array) = op_array; | |||
|
|
|||
| zend_oparray_context_begin(&orig_oparray_context, op_array); | |||
| CG(context).active_property_info_name = property_info_name; | |||
| CG(context).active_property_hook_kind = hook_kind; | |||
zend_throw_error() will just use zend_error_noreturn() if I'm reading correctly, Lines 1831 to 1836 in 222f751 when validators are run,
No validators should be writing anything at runtime, e.g. for allow dynamic properties the validator just |
https://wiki.php.net/rfc/delayedtargetvalidation_attribute