Skip to content

Conversation

@pmmaga
Copy link
Contributor

@pmmaga pmmaga commented Jul 3, 2016

When an implementation of a child method has the same name as a private parent method, their signatures are checked for compatibility.

This shouldn't be the case as if the parent's method is private, the child's method is not overriding it.

Link for the bug: https://bugs.php.net/bug.php?id=72496

This PR replaces #1971 which was made against 5.5

@pmmaga pmmaga changed the title private prototype method should be enough to not enforce the signature Fix for bug #72496 - Child method with different signature from parent private method Jul 3, 2016
@cmb69
Copy link
Member

cmb69 commented Jul 3, 2016

Thanks for basing on 5.6. The PR looks good to me.

@nikic As you've commented positively on the bug report: can this be merged, or does it need further discussion on internals?

@nikic
Copy link
Member

nikic commented Jul 3, 2016

@cmb69 It can be merged. Don't think there's anything controversial here.

@cmb69
Copy link
Member

cmb69 commented Jul 4, 2016

@nikic Fine. I can't merge, however, due to insufficient karma. FWIW, the respective changes for PHP 7.0 would be:

Zend/zend_inheritance.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c
index 845500c..cca0b97 100644
--- a/Zend/zend_inheritance.c
+++ b/Zend/zend_inheritance.c
@@ -268,8 +268,8 @@ static zend_bool zend_do_perform_implementation_check(const zend_function *fe, c
        return 1;
    }

-   /* If both methods are private do not enforce a signature */
-    if ((fe->common.fn_flags & ZEND_ACC_PRIVATE) && (proto->common.fn_flags & ZEND_ACC_PRIVATE)) {
+   /* If the prototype method is private do not enforce a signature */
+    if (proto->common.fn_flags & ZEND_ACC_PRIVATE) {
        return 1;
    }

@pmmaga
Copy link
Contributor Author

pmmaga commented Jul 4, 2016

@cmb69, I actually started by fixing it on master. Besides the change being now on inheritance there is also another test that needs a fix. You can check my changes: master...pmmaga:bug-72496

@cmb69
Copy link
Member

cmb69 commented Jul 4, 2016

Ah, thanks, @pmmaga! I had missed that.

@nikic
Copy link
Member

nikic commented Jul 5, 2016

Merged as 08777e9 into 5.6 and upwards. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants