Skip to content

[DependencyInjection][FrameworkBundle][SecurityBundle][TwigBundle] Require Composer's runtime API to be present #43788

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

Merged

Conversation

derrabus
Copy link
Member

Q A
Branch? 6.0
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

Recently, I was asked to help debugging a strange behavior of my client's app that surfaced only on some developer machines while others were fine. Turns out, a particular package was installed as a dev dependency (which was fine because we used it for dev tooling only at that time) and the difference between the environments was that the broken ones used Composer 2.

With ContainerBuilder::willBeAvailable(), we have introduced logic into the very heart of the framework that exposes significantly different behavior for Composer 1 and 2.

With this PR, I'd like to propose to make Composer's runtime API a requirement, essentially making the use of Composer 2 a requirement. Composer 2 has been released over a year ago and by now every developer should have been able to upgrade to version 2. I don't think that this constraint would push the ecosystem too hard.

Let's make everyone's lives easier by moving on to Composer 2.

@derrabus derrabus added DependencyInjection Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) ErrorHandler labels Oct 27, 2021
@derrabus derrabus requested a review from yceruto as a code owner October 27, 2021 16:10
@carsonbot carsonbot added this to the 6.0 milestone Oct 27, 2021
@carsonbot carsonbot changed the title Require Composer's runtime API to be present [DependencyInjection][ErrorHandler] Require Composer's runtime API to be present Oct 27, 2021
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 27, 2021

I'm personally fine doing that but we should throw a deprecation somewhere in 5.4.
Maybe at compile time in the DI component would be appropriate?

@derrabus
Copy link
Member Author

I'm personally fine doing that but we should throw a deprecation somewhere in 5.4. Maybe at compile time in the DI component would be appropriate?

In ContainerBuilder::compile() directly?

@nicolas-grekas
Copy link
Member

In ContainerBuilder::compile() directly?

Possibly yes. In a place that allows the deprecation to be collected for the profiler (with other compile-time deprecations).

👍 for Composer 2.1

but let's remove the change on ErrorHandler to allow using the component with less constraints if that doesn't matter much

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 27, 2021

Oh btw, if Symfony 6.0 is Composer 2.1 only, we could release Flex 2, which would bump to the same, with a cleaned up code base (eg the http downloader which is now in C2)

@derrabus derrabus force-pushed the improvement/require-composer-runtime-api branch from 04472b4 to 5993128 Compare October 28, 2021 09:41
@derrabus
Copy link
Member Author

I've bumped to 2.1 and reverted the ErrorHandler changes.

@derrabus derrabus force-pushed the improvement/require-composer-runtime-api branch 2 times, most recently from 6783d6a to 12ae615 Compare October 28, 2021 10:42
@nicolas-grekas
Copy link
Member

What about moving this to frameworkbundle actually? That would give better guarantees for what we consider "Symfony 6 apps", without imposing a restriction on standalone DI users.

@carsonbot carsonbot changed the title [DependencyInjection][ErrorHandler] Require Composer's runtime API to be present [DependencyInjection] Require Composer's runtime API to be present Oct 28, 2021
@derrabus
Copy link
Member Author

What about moving this to frameworkbundle actually?

What do you mean by "this"? The willBeAvailable() method?

@nicolas-grekas
Copy link
Member

I mean the composer constraint

@stof
Copy link
Member

stof commented Oct 28, 2021

The usage of the composer runtime api is in the DI component, not in FrameworkBundle. So that's where the dependency is.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 28, 2021

If we keep the "class_exists" check that is in place, DI doesn't have a dependency on C2.1.
This change is motivated by the will to provide a predictable behavior accross the board.
Adding the constraint to FWB found achieve this goal.

My fear is that by adding the constraint to DI, ppl that try to upgrade to SF6 with C1 will get dependencies resolved just fine... with DI 5.4...

@derrabus
Copy link
Member Author

If we keep the "class_exists" check that is in place, DI doesn't have a dependency on C2.1.

But that class_exists() check is precisely what bothers me. It's causes the unpredictable behavior. Alternatively, we could let willBeAvailable() throw on the 6.0 branch if the Runtime API is not available.

This change is motivated by the will to provide a predictable behavior accross the board. Adding the constraint to FWB found achieve this goal.

… for full-stack Symfony apps. The component standalone would still behave undpredictably if willBeAvailable() is used.

My fear is that by adding the constraint to DI, ppl that try to upgrade to SF6 with C1 will get dependencies resolved just fine... with DI 5.4...

We could solve that by either adding the Runtime API dependency to FWB as well or by letting FWB conflict with DI 5.4.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 28, 2021

The component standalone would still behave undpredictably if willBeAvailable() is used.

if willBeAvailable() is used yes.

this might not be enough a reason to force requiring C2.1 in DI, nor to make FWB 6 conflict with DI 5.4

We could solve that by either adding the Runtime API dependency to FWB as well or by letting FWB conflict with DI 5.4.

I'm all for adding the bump to fwb. Not for conflicting. And then I wonder why we should add it to DI. We like optional deps everywhere else.

@derrabus
Copy link
Member Author

We like optional deps everywhere else.

Okay, what about my proposal of letting willBeAvailable() throw on the 6.0 branch if the Runtime API is not available? The unpredictable behavior would be replaced with a clear error message and by adding the runtime API to FWB's dependencies we make sure, a Symfony 6 app won't run into the exception.

@nicolas-grekas
Copy link
Member

Throwing works for me, with a deprecation on 5.4 :)

@derrabus
Copy link
Member Author

Here you go: #43804

derrabus added a commit that referenced this pull request Oct 28, 2021
…[TwigBundle] Deprecate Composer 1 (derrabus)

This PR was merged into the 5.4 branch.

Discussion
----------

[DependencyInjection][FrameworkBundle][SecurityBundle][TwigBundle] Deprecate Composer 1

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Prepares #43788
| License       | MIT
| Doc PR        | N/A

This PR deprecates configuring one of those core bundles that make use of `ContainerBuilder::willBeAvailable()` if dependencies have been installed with Composer 1. `ContainerBuilder::willBeAvailable()` also triggers a deprecation now that will be suppressed in all calls coming from our core bundles' extensions.

Commits
-------

f058905 Deprecate Composer 1
@derrabus derrabus force-pushed the improvement/require-composer-runtime-api branch from 12ae615 to 7df301a Compare October 29, 2021 09:37
@derrabus
Copy link
Member Author

Ready.

@carsonbot carsonbot changed the title [DependencyInjection] Require Composer's runtime API to be present [DependencyInjection][FrameworkBundle][SecurityBundle][TwigBundle] Require Composer's runtime API to be present Oct 29, 2021
@derrabus derrabus force-pushed the improvement/require-composer-runtime-api branch from 7df301a to 2cab2f5 Compare October 29, 2021 10:34
fabpot added a commit that referenced this pull request Oct 29, 2021
…ecise about the required Composer version (derrabus)

This PR was merged into the 5.4 branch.

Discussion
----------

[FrameworkBundle][SecurityBundle][TwigBundle] Be more precise about the required Composer version

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #43788 (comment)
| License       | MIT
| Doc PR        | N/A

Commits
-------

295240a Be more precise about the required Composer version
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull request Oct 29, 2021
…ecise about the required Composer version (derrabus)

This PR was merged into the 5.4 branch.

Discussion
----------

[FrameworkBundle][SecurityBundle][TwigBundle] Be more precise about the required Composer version

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | symfony/symfony#43788 (comment)
| License       | MIT
| Doc PR        | N/A

Commits
-------

295240a4f5 Be more precise about the required Composer version
@derrabus derrabus force-pushed the improvement/require-composer-runtime-api branch from 2cab2f5 to e08b362 Compare October 29, 2021 12:44
@fabpot
Copy link
Member

fabpot commented Oct 30, 2021

Thank you @derrabus.

@fabpot fabpot merged commit 0865ede into symfony:6.0 Oct 30, 2021
@derrabus derrabus deleted the improvement/require-composer-runtime-api branch October 30, 2021 08:50
@fabpot fabpot mentioned this pull request Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection Feature FrameworkBundle RFC RFC = Request For Comments (proposals about features that you want to be discussed) SecurityBundle Status: Reviewed TwigBundle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants