Fix prototype for trait methods #14148
Conversation
Remove wierd checks and define the behavior by explicit set of flags
| /* Inherited members are overridden by members inserted by traits. | ||
| * Check whether the trait method fulfills the inheritance requirements. */ | ||
| do_inheritance_check_on_method( | ||
| fn, fixup_trait_scope(fn, ce), existing_fn, fixup_trait_scope(existing_fn, ce), | ||
| ce, NULL, /* check_visibility */ 1); | ||
| check_inheritance = true; |
There was a problem hiding this comment.
The movement of this call seems not necessary, because the function entry was duplicated into temporary buffer by zend_traits_copy_functions().
I'll verify if it was really necessary for master (see d344fe0)
There was a problem hiding this comment.
According to my check, this wasn't necessary. So I'm not sure if it's better to keep the call in the old place or move it down.
@TimWolla ^^
There was a problem hiding this comment.
Thank you for the ping. However I cannot comment meaningfully, due to my lack of expertise on the engine. You are the expert and I trust you here: If all tests pass, then the change is probably fine.
There was a problem hiding this comment.
@TimWolla the related code was changed so many times, that I spent hours trying to understand all the combinations of conditions... :)
Two questions:
-
Do you remember why did you move call to
do_inheritance_check_on_method_ex()inzend_add_trait_method()down (see commit d344fe0) -
I think
Zend/tests/attributes/override/016.phptis wrong. It should emit fatal error, because classCdon't have parent method at all.
There was a problem hiding this comment.
- Do you remember why did you move call to
do_inheritance_check_on_method_ex()inzend_add_trait_method()down (see commit d344fe0)
I don't remember exactly. Looking at the commit history of the PR, I made the change in:
The direct parent of it (66a7111) failed the ASAN CI. Unfortunately the logs are no longer available.
However based on that information, I assume that my initial attempt at the fix caused some memory unsafety that I was able to fix by moving the call down. Perhaps a future follow-up (e.g. Ilija's review suggestion) fixed it properly such that the moving of the call is no longer required, I cannot comment on that.
2. I think
Zend/tests/attributes/override/016.phptis wrong. It should emit fatal error, because classCdon't have parent method at all.
It does not have a parent method, but its signature is enforced by the abstract method in the trait. This has been voted on as part of the RFC.
There was a problem hiding this comment.
It does not have a parent method, but its signature is enforced by the
abstractmethod in the trait. This has been voted on as part of the RFC.
OK. I've committed the PRs keeping this behavior.
In other case I would also prefer to make clean up only in master, but you already found few weird related problems. |
|
Ok, then let's merge this for PHP 8.2. |
* PHP-8.2: Fix prototype for trait methods (#14148)
* PHP-8.3: Fix prototype for trait methods (#14148)
Fixes #14009