Skip to content
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

Extend Throwable in Exception marker interface #50

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

BackEndTea
Copy link
Contributor

No description provided.

@Majkl578
Copy link
Contributor

Majkl578 commented Sep 5, 2018

Tricky.
One one hand this is ok as Exceptions already implement all methods from Throwable.
On the other if anyone implemented only this interface, it's a BC break.

@Ocramius Thoughts?

@Ocramius
Copy link
Member

Ocramius commented Sep 5, 2018

@Majkl578 not a BC break IMO, since implementing this interface not on an exception would be extremely unwise and already unsupported by us anyway, since the interface explicitly states:

  * Base exception marker interface for the instantiator component 

Yes, we don't have a BC policy for this, but we can't have a BC policy for everything.

@Ocramius Ocramius self-assigned this Sep 5, 2018
@Ocramius Ocramius removed the BC break label Sep 5, 2018
@Ocramius Ocramius added this to the 1.2.0 milestone Sep 5, 2018
@Ocramius Ocramius merged commit 8788c31 into doctrine:master Sep 5, 2018
@Ocramius Ocramius changed the title Extend throwable in Exception marker interface Extend Throwable in Exception marker interface Sep 5, 2018
@Majkl578
Copy link
Contributor

Majkl578 commented Sep 5, 2018

I basically quoted the reason why Symfony reverted this change, but I'm generally 👍 for these. Also plays well with phpstan/phpstan#1001.

@BackEndTea BackEndTea deleted the update/throwable branch September 5, 2018 19:44
@BackEndTea
Copy link
Contributor Author

Thanks for the review and merge @Ocramius & @Majkl578

@alcaeus
Copy link
Member

alcaeus commented Sep 6, 2018

Strictly speaking, this is a BC break: we're adding new methods to an interface that previously weren't there. I don't think this should be included in 1.x.

@malarzm
Copy link
Member

malarzm commented Sep 6, 2018

But you need those method anyway :)

@Majkl578
Copy link
Contributor

Majkl578 commented Sep 6, 2018

Technically you don't, I'd you mocked only the marker interface, you are now required to implement these new methods while there were none previously.

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.

5 participants