Fix GH-15731: Prevent #[AllowDynamicProperties] on enums
#15733
Conversation
DanielEScherzer
force-pushed
the
allowdynamicproperties-enum
branch
from
c8de323 to
34a598c
Compare
|
lgtm but prefer to see other people opinion, no rush it s not really a terrible bug :-) |
| @@ -84,6 +84,11 @@ static void validate_allow_dynamic_properties( | |||
| ZSTR_VAL(scope->name) | |||
| ); | |||
| } | |||
| if (scope->ce_flags & ZEND_ACC_ENUM) { | |||
There was a problem hiding this comment.
The change looks good on its own but I'm wondering whether it isn't an oversight of the enum RFC that attributes targeting classes are allowed for enums? It looks like to me that we should(n't have) allow(ed) this, so the best course of action would be to add a new target for enums, and only allow the usage if it's set.
cc @iluuu1994
There was a problem hiding this comment.
It's not an oversight. From the RFC:
https://wiki.php.net/rfc/enumerations
Enums and cases may have attributes attached to them, like any other language construct. The TARGET_CLASS target filter will include Enums themselves.
Enums are special classes, just like interfaces, traits, etc.
There was a problem hiding this comment.
@kocsismate Do you have a special use-case for TARGET_ENUM? We considered it back then but it never came up again, so I assume use-cases are limited.
There was a problem hiding this comment.
Do you have a special use-case for TARGET_ENUM? We considered it back then but it never came up again, so I assume use-cases are limited.
No, I don't have any... I just proposed this in order to be able to retain the original behavior if we prevented the usage of attributes targeting classes for enums. But I realized that it would already be a possibly serious BC break since userland attributes have already been relying on the current behavior. Sorry for the noise!
@DanielEScherzer it might be best if you just update your other PR instead. |
Since the other PR has already merged, I have retargeted this to |
|
Thanks ! |
No description provided.