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.
| * Technically this shouldn't be needed since attribute validators run before | ||
| * interfaces are added and classes are linked, but better safe than sorry |
There was a problem hiding this comment.
This is zend_test, taking a short-cut here is IMO okay.
There was a problem hiding this comment.
Except that another attribute might have added an interface (also I realized that I neglected to update the reference to "custom casting") so I think this is still useful
| // 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
| } | ||
| } | ||
| if (already_skipped) { | ||
| to_add_names[i] = name; |
There was a problem hiding this comment.
This looks wrong. It's in already_added and you are adding it to to_add_names again.
There was a problem hiding this comment.
This is not wrong - it was already added by an attribute validator, but we already skipped the first time that the developer manually tried to add the interface, here the interface is being added a second or subsequent time manually in addition to what the attribute did, added a comment to document this
See the ext/zend_test/tests/attribute-adds-interface-04.phpt test
| 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
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
| * Technically this shouldn't be needed since attribute validators run before | ||
| * interfaces are added and classes are linked, but better safe than sorry |
There was a problem hiding this comment.
Except that another attribute might have added an interface (also I realized that I neglected to update the reference to "custom casting") so I think this is still useful
| // 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
| } | ||
| } | ||
| if (already_skipped) { | ||
| to_add_names[i] = name; |
There was a problem hiding this comment.
This is not wrong - it was already added by an attribute validator, but we already skipped the first time that the developer manually tried to add the interface, here the interface is being added a second or subsequent time manually in addition to what the attribute did, added a comment to document this
See the ext/zend_test/tests/attribute-adds-interface-04.phpt test
| 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
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
910c09b to
a13a680
Compare
2 participants
No description provided.