php-fig / container

MIT License
9.93k stars 52 forks source link

Bump minimum supported PHP version to 7.4 on v2 releases #32

Closed weierophinney closed 2 years ago

weierophinney commented 3 years ago

Fixes #30 for the v2 series by bumping to a PHP version that does interface inheritance correctly.

moufmouf commented 3 years ago

I think we need to think twice before merging this.

It is possible to implement v1 and v2 at the same time. The only thing that changes is the added return type. It the implementation is adding a return type but is still targetting v1, this is not going to cause issues. See https://3v4l.org/MlNRT

Let's take the example of Symfony. By introducing a return type in their container, they are indeed introducing a breaking change in their own library. So Symfony can only upgrade to psr/container in a new major release (Symfony 6)

But Symfony 6 could target "psr/container ^1|^2" rather than only targetting "psr/container ^1". This makes a lot of sense (a lot of third party libraries are targetting "psr/container ^1" only and you don't want to make Symfony 6 incompatible with those libraries)

But if we merge this PR and tag a v2.0.1, any user using PHP 7.2 or PHP 7.3 could install Symfony 6 using "composer update". This would automatically install "psr/container 2.0.0" which has the issue described in #30.

Maybe we could instead:

What do you think?

derrabus commented 3 years ago

Let's take the example of Symfony. By introducing a return type in their container, they are indeed introducing a breaking change in their own library. So Symfony can only upgrade to psr/container in a new major release (Symfony 6)

Correct, it won't happen in 5.x.

But if we merge this PR and tag a v2.0.1, any user using PHP 7.2 or PHP 7.3 could install Symfony 6 using "composer update". This would automatically install "psr/container 2.0.0" which has the issue described in #30.

Only if you assume that Symfony 6 maintains compatibility with PHP 7.2/7.3. If Symfony 6 requires PHP 7.4 at least, we're good. And I think it's very likely that Symfony 6 will do that PHP bump (My personal prediction, no official announcement).

Then again, Symfony is not the only container implementation out there, so your plan sounds reasonable.

Jean85 commented 3 years ago

But Symfony 6 could target "psr/container ^1|^2" rather than only targetting "psr/container ^1". This makes a lot of sense (a lot of third party libraries are targetting "psr/container ^1" only and you don't want to make Symfony 6 incompatible with those libraries)

Not only they could, it's suggested by our bylaw. But they (or anyone else) can target only ^1.1|^2, plain 1.0 can't coexist with 2.0.

Or maybe even better ^1.1.1|2.0.1, skipping the bugged releases for 7.2-7.3.

Jean85 commented 3 years ago

I've inadvertently pushed onto master this change: https://github.com/php-fig/container/commit/19764b1677626ba3db35c033c329f9d603e87f26

Sorry for the mistake; it does follow @moufmouf suggestion, so if we tag it as 2.0.1 we can then revert it and require PHP 7.4, tagging it as 2.1.0. Do you agree?

Jean85 commented 2 years ago

We definitely forgot about this.. IMHO we should re-add extends \Throwable to this PR and proceed to release this as a new patch for both 2.x and 3.x, as said in #33 and elsewhere.

@weierophinney @mnapoli @moufmouf WDYT?

mnapoli commented 2 years ago

Sure!

weierophinney commented 2 years ago

@Jean85 I've re-instated the extends Throwable declaration in the ContainerExceptionInterface. Should I also bump the PHP version constraint? Or leave as-is?