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

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

Conversation

weierophinney
Copy link
Contributor

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

@moufmouf
Copy link
Contributor

moufmouf commented Mar 5, 2021

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:

  • Remove the "Throwable" interface
  • Tag a 2.0.1
  • Upgrade to PHP 7.4 and add back the "Throwable" interface
  • Tag a 2.0.2

What do you think?

@derrabus
Copy link

derrabus commented Mar 5, 2021

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
Copy link
Member

Jean85 commented Mar 7, 2021

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.

Copy link
Member

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

As for this PR, I agree with @moufmouf: we need to first revert the issue as we did in 1.1.1, so future users under PHP 7.2/7.3 will not incur in the same bug; afterwards, we could reintroduce the interface after bumping to 7.4.

@Jean85
Copy link
Member

Jean85 commented Mar 15, 2021

I've inadvertently pushed onto master this change: 19764b1

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
Copy link
Member

Jean85 commented Oct 14, 2021

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
Copy link
Member

mnapoli commented Oct 14, 2021

Sure!

Fixes php-fig#30 for the v2 series by bumping to a PHP version that does interface inheritance correctly.
@weierophinney weierophinney force-pushed the fix/bump-php-version-to-fix-dependency-issues branch from 4c05c98 to 6009776 Compare November 5, 2021 13:25
@weierophinney
Copy link
Contributor Author

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

Copy link
Member

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Nope, 7.4 is fine! 👍

@Jean85 Jean85 merged commit c71ecc5 into php-fig:master Nov 5, 2021
Jean85 pushed a commit that referenced this pull request Nov 5, 2021
@Jean85 Jean85 linked an issue Nov 5, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception interfaces should extend Throwable Fatal error with v1.1.0 and PHP 7.2 / 7.3
5 participants