search

php-fig / container

MIT License
9.93k stars 52 forks source link

Make ContainerExceptionInterface extend Throwable where it is available #23

Closed ErikBooij closed 3 years ago

ErikBooij commented 5 years ago

I've run into some static analysis issues because the ContainerExceptionInterface provides no guarantee that it's actually throwable (or catchable more specifically).

I understand making it extend Throwable would break BC, so I think this would be the ideal middle ground, providing the benefit of stricter typing only where it's available.

mnapoli commented 5 years ago

👍 I've faced the same problem.

Is it a BC break to extend Throwable?

ErikBooij commented 5 years ago

Current minimum compatibility constraint is 

"require": {
    "php": ">=5.3.0"
}

Throwable is only available from 7.0 upwards.

mnapoli commented 5 years ago

OK I see, tagging a release that targets 7.0 wouldn't really be a BC break though since Composer will not install the new release unless you run PHP 7.

ErikBooij commented 5 years ago

Agreed, and I'm not aware of the FIG's BC policy, but to me it seems unnecessary to limit BC to PHP 7+ going forward, while with this solution, we can still maintain compatibility with 5.3+.

Although I also don't see many changes/upgrades coming to this package in the future, since I guess it does what it's supposed to ;-)

If it's preferred to just extend Throwable without the existence check for the interface, I'm happy to change it.

mnapoli commented 5 years ago

You're right, this solution works fine!

I'm :+1: with both solution. Does anyone know what is needed to merge this?

Jean85 commented 5 years ago

Unfortunately we do not have any kind of process defined for this kind of stuff. In any case, the editor(s) of this PSR (which are you @mnapoli along with @moufmouf) have the final say on this.

ErikBooij commented 5 years ago

@mnapoli @moufmouf Could you please take another look at this and if it's acceptable, get it merged and released? Would be much appreciated.

ErikBooij commented 5 years ago

@mnapoli @moufmouf @Jean85 Gentle reminder that this PR is still here and open 😉 Any chance we can get this merged and released?

Jean85 commented 4 years ago

We've been discussing this kind of issues a lot lately, and we've condensed our proposed solution to updating PSR interfaces in a blog post. Please read it and provide your feedback!

https://www.php-fig.org/blog/2019/10/upgrading-psr-interfaces/

ErikBooij commented 4 years ago

@Jean85 I've read the post and it makes total sense, I've filled in the feedback form.

At the same time, to be honest, I don't think it really applies to this PR, as this has specifically been created to not be a BC break, therefore I'd still love to just get this merged and released as 1.0.1

ChristophWurst commented 4 years ago

Hey, I'm in the process of adapting more PSR interfaces in Nextcloud and I came across this missing base class as I added the psr/conatiner package. Would it be possible to include this change with the v2.0 release together with https://github.com/php-fig/container/pull/28?

Jean85 commented 4 years ago

This could made even 1.1, and with no if shenanigans too!

ping @weierophinney

weierophinney commented 4 years ago

@Jean85 Would this require an errata vote?

mnapoli commented 4 years ago

I hope not ^^

Jean85 commented 4 years ago

@weierophinney IMHO it could be bundled up with everything else in the 1.1-2.0 evolution vote. As far as I'm concerned, this interface only enforces at the code level what the spec requires.

weierophinney commented 3 years ago

@Jean85 This patch actually duplicates work already in #20, which does target 1.1, and doesn't require a conditional as it requires PHP 7.2. As such, I'm closing it.