search

doctrine / instantiator

https://www.doctrine-project.org/projects/instantiator.html
MIT License
10.95k stars 62 forks source link

Extend `Throwable` in Exception marker interface #50

Closed BackEndTea closed 6 years ago

Majkl578 commented 6 years ago

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 commented 6 years ago

@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.

Majkl578 commented 6 years ago

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

BackEndTea commented 6 years ago

Thanks for the review and merge @Ocramius & @Majkl578

alcaeus commented 6 years ago

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 commented 6 years ago

But you need those method anyway :)

Majkl578 commented 6 years ago

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.