GH-22354: support interfaces added by attribute validators in zend_compile_implements()#22355
GH-22354: support interfaces added by attribute validators in zend_compile_implements()#22355DanielEScherzer wants to merge 2 commits into
Conversation
DanielEScherzer
force-pushed
the
attribute-validator-interfaces
branch
from
a911024 to
094b72f
Compare
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Logic-wise this seems correct. But I feel this is a “master-only” bugfix, since the situation for this to appear seems to be quite rare.
| // there are errors about trying to implement an interface that was already | ||
| // implemented. | ||
| if (EXPECTED(ce->num_interfaces == 0)) { | ||
| interface_names = emalloc(sizeof(zend_class_name) * list->children); |
There was a problem hiding this comment.
This line is unchanged from earlier and I'd prefer not to mess with it - I just moved it into a conditional block
| efree(skipped_names); | ||
|
|
||
| const uint32_t initial_count = ce->num_interfaces; | ||
| interface_names = safe_erealloc(ce->interface_names, (initial_count + to_add_count), sizeof(*interface_names), 0); |
There was a problem hiding this comment.
You are repeating initial_count + to_add_count a lot. I suggest a local variable.
There was a problem hiding this comment.
This is precisely the only place that that expression occurs? Below I use added_count which also changes each time through the loop so shouldn't be saved
There was a problem hiding this comment.
Ah, yes. In that case using pointer arithmetic might reduce the amount of copy and paste required.
DanielEScherzer
force-pushed
the
attribute-validator-interfaces
branch
from
094b72f to
910c09b
Compare
DanielEScherzer
left a comment
DanielEScherzer
left a comment
There was a problem hiding this comment.
Logic-wise this seems correct. But I feel this is a “master-only” bugfix, since the situation for this to appear seems to be quite rare.
I encountered this in an actual extension, https://github.com/DanielEScherzer/CustomCast - I'm not sure why I didn't notice or report it earlier, but it does indeed affect me
| // there are errors about trying to implement an interface that was already | ||
| // implemented. | ||
| if (EXPECTED(ce->num_interfaces == 0)) { | ||
| interface_names = emalloc(sizeof(zend_class_name) * list->children); |
There was a problem hiding this comment.
This line is unchanged from earlier and I'd prefer not to mess with it - I just moved it into a conditional block
| efree(skipped_names); | ||
|
|
||
| const uint32_t initial_count = ce->num_interfaces; | ||
| interface_names = safe_erealloc(ce->interface_names, (initial_count + to_add_count), sizeof(*interface_names), 0); |
There was a problem hiding this comment.
This is precisely the only place that that expression occurs? Below I use added_count which also changes each time through the loop so shouldn't be saved
DanielEScherzer
force-pushed
the
attribute-validator-interfaces
branch
from
910c09b to
a13a680
Compare
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Also requested another review from someone more experienced with engine code to make a target-branch call.
|
I agree with Tim that this should target master, this is borderline unintended. I also don't love this approach, because if mutation of classes is allowed, adding an interface is just one thing the attribute could do. It might be better to add a new attribute hook that triggers after the class is fully compiled, after which the attribute can do it's thing, and in this case handle the duplicate interface logic itself. |
In `zend_compile_implements()`, when there were no interfaces already added, keep the existing code of just adding the newly declared interfaces. But, when some interfaces are already present, don't overwrite them. Instead, add to the existing list. In case the developer tries to add an interface that was already added by an attribute, skip the manual interface addition to avoid errors about trying to apply the same interface multiple times. But, only skip the *first* manual interface addition of the same interface, if there are multiple such additions then there really was an attempt to apply the same interface multiple times. Fixes php#22354
DanielEScherzer
force-pushed
the
attribute-validator-interfaces
branch
from
a13a680 to
3d1840c
Compare
3 participants
No description provided.