Skip to content

Fix #74607 - Don't check for bi-directional compatibility in traits #2530

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

Closed
wants to merge 1 commit into from

Conversation

pmmaga
Copy link
Contributor

@pmmaga pmmaga commented May 21, 2017

Link for bugsnet: https://bugs.php.net/bug.php?id=74607

Bi-directional checks are kept in case two abstract methods are being added. Additionally, some error messages now show the proper direction of the compatibility mismatch.

Given that this lifts a current restriction, no BC breaks should be introduced.

@@ -1154,8 +1154,8 @@ static void zend_add_trait_method(zend_class_entry *ce, const char *name, zend_s
/* Make sure the abstract declaration is compatible with previous declaration */
Copy link
Member

@nikic nikic May 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to this else if, are we not going to miss a check in a case like this?

abstract class A {
    abstract function foo($x);
}
trait T {
    abstract function foo($x = 0);
}
abstract class C extends A {
    use T;
}

Assuming I'm understanding the desired behavior correctly (that is, abstract methods in traits impose an additional constraint, they do not simply override), this should fail, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But no, that doesn't really make sense to me. Allowing that code should be fine.

@krakjoe krakjoe added the Bug label May 25, 2017
@krakjoe
Copy link
Member

krakjoe commented May 25, 2017

Can we get an update on the status of this patch please ?

@nikic
Copy link
Member

nikic commented Jun 2, 2017

Merged as c6c1e75 into master. Not applying this to release branches, as this is a larger change to inheritance semantics.

@nikic nikic closed this Jun 2, 2017
@pmmaga
Copy link
Contributor Author

pmmaga commented Jun 4, 2017

Hi guys! Sorry for the long silence. Thanks for picking it up! Cheers

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

Successfully merging this pull request may close these issues.

3 participants