Skip to content

[FrameworkBundle] Integrate PsrHttpMessageBridge #52372

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

Conversation

derrabus
Copy link
Member

Q A
Branch? 6.4
Bug fix? no
New feature? yes 😓
Deprecations? no
Issues Follows #51100, #51197
License MIT

This PR teaches FrameworkBundle how to integrate the PSR HTTP Message bridge. Sorry for submitting this PR that late. I kinda assumed we've done this already, but when I tried to upgrade one of my projects to 6.4 I noticed that this part was missing. 😓

PsrHttpFactory requires a set of four PSR-17 factories, e.g. ServerRequestFactoryInterface. If the app registers autowire aliases for those contracts, they are wired into our PsrHttpFactory services. The recipe for nyholm/psr7 sets up those autowire aliases for instance.

If the autowire aliases are missing, we register PsrHttpFactory without dependencies, leveraging the fallback introduced with #51197.

It is also possible to install version 2.3 of the bridge. This is important in order to give projects that run Symfony 6.3 at the moment a smooth upgrade path because with its integration into the monorepo, the bridge jumped from 2.3 to 6.4. However, enabling the integration requires version 6.4 because we need the changes from #51197.

@carsonbot carsonbot added this to the 6.4 milestone Oct 30, 2023
@derrabus derrabus force-pushed the improvement/psr-http-message-integration branch from 54f96ab to eedda81 Compare October 30, 2023 22:29
@OskarStark OskarStark changed the title [FrameworkBundle] Integrate PsrHttpMessageBridge [FrameworkBundle] Integrate PsrHttpMessageBridge Oct 31, 2023
@nicolas-grekas
Copy link
Member

What about https://github.com/symfony/recipes/blob/main/symfony/psr7-pack/1.0/config/packages/psr_http_message_bridge.yaml?

Doesn't that solve the issue?

I like that currently the listener + value resolver are opt-in. With this change, they would be registered automatically.
Is that what we want? (I'm honestly wondering :) )

@derrabus
Copy link
Member Author

Doesn't that solve the issue?

I was under the impression, we wanted to get rid of the pack. Also, since the bridge is now part of the monorepo, I framework bundle should at least register services and autowire aliases for the two factories. For whatever reason the bridge is installed: We will at least need one of those two services.

I like that currently the listener + value resolver are opt-in. With this change, they would be registered automatically.

I can add a config key to make them opt-in if you like. But I don't necessarily see a problem with registering them, tbh.

Is that what we want? (I'm honestly wondering :) )

I usually install the bridge because I want that behavior, yes.

@nicolas-grekas
Copy link
Member

The pack is still useful to me, it brings a nice DX. Which means the recipe might be enough?

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@derrabus
Copy link
Member Author

Which means the recipe might be enough?

Well, it's like with any other core component. We could totally register any service through recipes. But I don't think it's nice to work with. I get that you alienate with those two listeners (don't really get why, but well…), but the two factories don't hurt anyone if we let the bundle wire them.

Please advise how I should proceed here. Scrapping the whole PR would really be a pity.

@derrabus derrabus force-pushed the improvement/psr-http-message-integration branch from eedda81 to 527ddd2 Compare November 15, 2023 22:46

->set('psr_http_message_bridge.psr_server_request_resolver', PsrServerRequestResolver::class)
->args([service('psr_http_message_bridge.psr_http_factory')])
->tag('controller.argument_value_resolver', ['priority' => -100])
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 17, 2023

Choose a reason for hiding this comment

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

I'm trying to run this PR to see how it goes. I did install a webapp-pack and added the psr bridge.
Then I created a controller and added a PSR RequestInterface as argument.
Because this priority is below ServiceValueResolver , I've an error telling me no such service.
Then I changed the -100 to 50 (same level as RequestValueResolver), and the error is now "no PSR-17 factories have been provided."
This is consistent with the fact that the bridge is ... a bridge between Symfony and ... a library that needs to be installed.
A solution to this could be to require php-http/discovery + psr/http-factory-implementation. That's the only way for us to provide a nice out of the box experience.

-- or --
we keep the psr7-pack + its recipe? 👼

@nicolas-grekas
Copy link
Member

Let me close: too much work for too little benefits compared to the current way.
(Feel free to resubmit of course, taking my previous comment into account)

@derrabus derrabus deleted the improvement/psr-http-message-integration branch March 25, 2024 21:41
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.

3 participants