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

[DependencyInjection] Add conflict rules for incompatible ext-psr versions #40876

Merged
merged 1 commit into from Apr 20, 2021

Conversation

derrabus
Copy link
Member

Q A
Branch? 5.x
Bug fix? no
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

This PR adds conflict rules for outdated versions of the PECL extension psr which are known to cause compatibility issues with DependencyInjection 5.3 and ServiceContracts 2.4.

Copy link
Member

@jderusse jderusse left a comment

Choose a reason for hiding this comment

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

What's about psr/cache

@derrabus
Copy link
Member Author

What's about psr/cache

Nothing to do here. We haven't implemented the property type declarations of psr/cache 2 because that would have forced all users of symfony/cache to bump to psr/cache 2 which requires PHP 8.

In other words: ext-psr does not break our cache implementation (yet).

@wouterj
Copy link
Member

wouterj commented Apr 19, 2021

Shouldn't this target 5.2 (or 4.4), as it's a bugfix? (the previous bug reports fixed by this change were also about 5.2)

@derrabus
Copy link
Member Author

@wouterj No, because the change on our side that causes the compatibility problems with ext-psr was introduced with #40384. DependencyInjection < 5.3 and ServiceContracts < 2.4 are unaffected.

composer.json Outdated
@@ -151,6 +151,7 @@
"twig/markdown-extra": "^2.12|^3"
},
"conflict": {
"ext-psr": "<1.1",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should also conflict with ext-psr >=2, as we are not compatible with psr/container 2.0 either.

note that the versioning of ext-psr will probably be a mess anyway, as the various psr packages don't have the same version anymore...

Copy link
Member

Choose a reason for hiding this comment

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

* would work for me. That'd be my preference actually...

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should also conflict with ext-psr >=2, as we are not compatible with psr/container 2.0 either.

True, but we don't know if and when ext-psr will implement psr/container 2.0. When they do, I don't think that they will match the FIG's versioning strategy. Simply because psr/container is not the only package that this extension provides.

psr

Version => 1.1.0
Released => 2021-04-10
Authors => John Boehr <jbboehr@gmail.com> (lead)
PSR-3 Log Version => 1.0.0
PSR-6 Cache Version => 1.0.0
PSR-7 Http Message Version => 1.0.0
PSR-11 Container Version => 1.1.1
PSR-13 Link Version => 1.0.0
PSR-14 Event Dispatcher => 1.0.0
PSR-15 HTTP Handlers (Server Handler) => 1.0.0
PSR-15 HTTP Handlers (Middleware) => 1.0.0
PSR-16 Simple Cache Version => 1.0.0
PSR-17 HTTP Factories => 1.0.0
PSR-18 HTTP Client => 1.0.0

Excluding any future version would just be a wild guess, I'm afraid.

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to guess about the future to know that this extension is doomed and the any future version might only be a waste of time.

Copy link
Member Author

Choose a reason for hiding this comment

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

This extension is a dependency of Phalcon. If we disallow it completely, we basically disallow using Symfony components in Phalcon apps as well as deploying Symfony and Phalcon apps on the same shared hosting environment.

Copy link
Member

Choose a reason for hiding this comment

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

@derrabus we require psr/container 1.1. If ext-psr bundles the a different one, things won't work (be it 1.0 or 2.0). The difference is that right now, composer will not report any issue, and users will then get a fatal error saying that the Symfony code implements the interface in the wrong way.
Also, we are not disallowing the 1.1 version of the extension for now. so that's still possible for phalcon today if the project uses the 1.1 version of ext-psr.

But IMO, this should also be reported to phalcon so that they try to solve that issue too.

Copy link
Member

Choose a reason for hiding this comment

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

I'm with @nicolas-grekas on this one.
This extension is by design broken.

Merging severals package (with different individual versions and behavior) in a single package can not work.

Packages that depends on it will follow the same path. Phalcon should reconsider its dependencies (and probably stops depending on extensions)

Copy link
Member Author

Choose a reason for hiding this comment

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

@derrabus we require psr/container 1.1. If ext-psr bundles the a different one, things won't work (be it 1.0 or 2.0). The difference is that right now, composer will not report any issue, and users will then get a fatal error saying that the Symfony code implements the interface in the wrong way.

Correct. This is why I propose to add a conflict rule for versions of ext-psr that we know will break our code. But thinking about it, the assumption that a version 2 of that package will break our DI component again, is probably safe.

Also, we are not disallowing the 1.1 version of the extension for now.

Right, but @nicolas-grekas suggested to do that.

Packages that depends on it will follow the same path. Phalcon should reconsider its dependencies (and probably stops depending on extensions)

@jderusse Phalcon itself is an extension. It's a full-stack framework implemented as a PHP extension. Phalcon provides implementations of the PSR interfaces. That's why they need those interfaces to be provided as a PHP extension as well: a class provided by an extension cannot implement a userland interface.

Copy link
Member

Choose a reason for hiding this comment

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

and that framework implemented as an extension is basically incompatible with running other projects with the same PHP runtime on the system (as it would still load the phalcon framework.

@derrabus derrabus force-pushed the improvement/conflict-ext-psr branch from 442399a to a02f062 Compare April 19, 2021 20:21
@derrabus
Copy link
Member Author

I have implemented @stof's suggestion. This means we're basically only allowing ^1.1 of the extension.

@fabpot
Copy link
Member

fabpot commented Apr 20, 2021

Thank you @derrabus.

@fabpot fabpot merged commit ec62a5a into symfony:5.x Apr 20, 2021
@derrabus derrabus deleted the improvement/conflict-ext-psr branch April 20, 2021 07:20
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.

None yet

7 participants