Fix prototype for trait methods #14016
Conversation
iluuu1994
force-pushed
the
gh-14009
branch
3 times, most recently
from
9d0c60d to
9e53a65
Compare
iluuu1994
force-pushed
the
gh-14009
branch
3 times, most recently
from
f33f80c to
0295615
Compare
There was a problem hiding this comment.
I think that the patch is still not good enough.
The logic is spread among the code with edge conditions...
Probably, it would be better to introduce yet another boolean argument that explicitly enables/disables child modification (may be replace all boolean arguments with flags).
force_mutable argument name makes me think about egg and chiсken question...
It would be also good to check the impact on performance. This function was especially optimized and has inlined and not-inlined versions (may be this not necessary anymore)
| /* Parent is a trait when its method is abstract. We only need to verify its signature then, | ||
| * don't actually use it as a prototype. See Zend/tests/gh14009_004.phpt. */ | ||
| && !(parent->common.scope->ce_flags & ZEND_ACC_TRAIT)) { |
There was a problem hiding this comment.
If this comment is true, then traits should use this function with check_only == true, but check_only also hides some error messages and makes test failures :(
There was a problem hiding this comment.
Right, that was my first thought. But I then realized that this completely disables the error messages, and adding yet another parameter to the already confusing ones is something I was trying to avoid.
|
Looking at this again, I think this is the best solution for now. We could pass a new parameter to prevent copying/modification, but it will be hard to name it well, and I feel it's not more correct. The function is not copied for abstract trait methods not because copying isn't possible. Rather, it's because setting the prototype is actually incorrect, as doing so will break the relationship with the actual parent class. Allowing multiple prototypes would fix the |
There was a problem hiding this comment.
I dived into the problem and found that this fix for 8.2 is wrong.
Zend/tests/gh14009_001.phpt works only because all the classes are in the single script. Actually you patch changes the prototype of the trait method and then copy it into "C". This won't work with immutable trait and may cause other problems.
I hope, I'll provide the updated fix by the evening.
|
Can you provide your test? In case of abstract trait methods, neither the trait not the original class should be affected, as both trait the method from the parent class can come from different files. I don't think copying is the right solution, because it breaks polymorphism through a common parent method. |
I was wrong. This works because the function entry is copied into a temporary buffer in Anyway please take a look into my patch: #14148 |
|
Closing in favor of #14148. |
Fixes GH-14009