-
Notifications
You must be signed in to change notification settings - Fork 7.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfix for #73987 #2342
Bugfix for #73987 #2342
Conversation
|
||
?> | ||
--EXPECTF-- | ||
Fatal error: Declaration of B::example($a, $b, $c = NULL) must be compatible with A::example($a, $b = NULL, $c = NULL) in %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC the rule is that an incompatibility with an interface throws a fatal error, while an incompatibility with a (non-abstract) class signature only generates a warning. As such, shouldn't this be a warning? (Same for the other cases as well, of course).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, just saw that we always fatal for return types. In that case it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'd recommend landing this in 7.1+ only for now, as there is a potential of fallout from code that accidentally specified wrong signatures due to this bug.
Thanks for the review! Agreed |
Merged b67eb34 Thanks. |
See the reason: https://3v4l.org/VDT0P The behavior was changed with this PR: php/php-src#2342 but somehow got left undocumented in upgrade file.
Inheritance checks should not ignore parents if these implement an interface or abstract.
Link for bugsnet: https://bugs.php.net/bug.php?id=73987