Conversation
Girgias
force-pushed
the
inherit-interfaces-early
branch
from
cb173f5 to
91c5a7c
Compare
arnaud-lb
left a comment
arnaud-lb
left a comment
There was a problem hiding this comment.
I don't see anything blocking from moving zend_do_inherit_interfaces() at the beginning of do_interface_implementation(), but maybe wait for other reviews.
One potential BC would be that interface_gets_implemented is now called early, but this doesn't appear to be an issue in practice.
The only thing I could see happening is a different compile time error being emitted, but I didn't manage to find such a test case. |
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Can't meaningfully comment on the change itself, as I am not familiar with the code.
Zend/zend_inheritance.c
Outdated
| static inline void do_implement_interface(zend_class_entry *ce, zend_class_entry *iface) | ||
| { | ||
| do_implement_interface_ex(ce, iface, iface); | ||
| } |
There was a problem hiding this comment.
Do we really need the _ex? distinction? The function is static and only used 2 times from what I see.
Zend/zend_inheritance.c
Outdated
| { | ||
| if (!(ce->ce_flags & ZEND_ACC_INTERFACE) && iface->interface_gets_implemented && iface->interface_gets_implemented(iface, ce) == FAILURE) { | ||
| zend_error_noreturn(E_CORE_ERROR, "%s %s could not implement interface %s", zend_get_object_type_uc(ce), ZSTR_VAL(ce->name), ZSTR_VAL(iface->name)); | ||
| if (!(ce->ce_flags & ZEND_ACC_INTERFACE) && inherited_face->interface_gets_implemented && inherited_face->interface_gets_implemented(base_iface, ce) == FAILURE) { |
There was a problem hiding this comment.
Is this correct? interface_gets_implemented() should be able to validate if an inteface can be implemented, but if we only pass the base_iface, we'll miss validation for the actual inherited interface. E.g. will this break if UnitEnum is added to a class implicitly through an inherited interface?
Is this change needed at all?
There was a problem hiding this comment.
The first parameter of the interface_gets_implemented() object hook is basically never used. And if it is (as in the case for the Enum and Throwable ones) it's just for the name of it, which frankly we already know. So we probably could just get rid of it.
However passing the base_iface can clarify what explicitly implemented interface is causing the issue (as the new test I'm pushing showcases), but overall no it seems like it is not needed.
Girgias
force-pushed
the
inherit-interfaces-early
branch
from
91c5a7c to
7864a62
Compare
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
Output of Zend/tests/enum/no-class-implements-backed-enum.phpt changed and looks a bit weird now, but probably not worth fixing (and is technically correct still).
4 participants
Needed for #18260 so that different bound types for implicit and explicitly implemented interfaces are checked correctly due to how the implementation stores the types in the HashTable.
I don't see any particular reason as to why the inheritance of interfaces was delayed, thus moving it early doesn't seem like an issue to me.