Add a bundle #94

derrabus · 2021-03-10T17:10:48Z

This bundle is an attempt to create a nicer and more seamless integration than the recipe previously provided.

Services

The bundle will provide the following services:

psr_http_message.http_foundation_factory

This is HttpFoundationFactory and allows us to convert PSR-7 messages into HttpFoundation objects. This service is always enabled and it's the only service that does not require any additional packages. An autowiring alias for Symfony\Bridge\PsrHttpMessage\HttpFoundationFactoryInterface is provided.

PSR-17 factories

psr_http_message.psr.request_factory
psr_http_message.psr.response_factory
psr_http_message.psr.server_request_factory
psr_http_message.psr.stream_factory
psr_http_message.psr.uploaded_file_factory
psr_http_message.psr.uri_factory

Those services require a PSR-17 implementation to be present. If nyholm/psr7 is installed, the factories are enabled automatically. Alternatively, the bundle can also configure laminas/laminas-diactoros.

Autowiring aliases for all PSR-17 interfaces are provided.

psr_http_message.psr_http_factory

This is PsrHttpFactory. It allows up to convert HttpFoundation objects into PSR-7 messages. The service is available if the PSR-17 factories have been enabled (because of the obvious dependency). Symfony\Bridge\PsrHttpMessage\HttpMessageFactoryInterface is registered for autowiring.

Message converters

PsrResponseListener and PsrServerRequestResolver have been added to the bridge in version 2.1.0. Enabling those allows us to write controllers that operate on PSR-7 messages instead of HttpFoundation. This functionality is disabled by default and can be enabled.

Configuration reference

psr_http_message:

    message_factories:
        enabled : true
        implementation: nyholm # alternatively: "diactoros"

    message_converters:
        enabled: false

    response_buffer: 16372 # Fine-tuning for HttpFoundationFactory

Bundle/DependencyInjection/Configuration.php

ajgarlag · 2021-03-10T18:42:38Z

I wrote a drop-in replacement for PSR-7 functionality removed from sensio/framework-extra-bundle

You can see it at https://github.com/ajgarlag/psr-http-message-bundle

Nyholm

Im happy with this.

@ajgarlag, Is there any feature your bundle supports that should be added here?

Bundle/DependencyInjection/Configuration.php

composer.json

ajgarlag · 2021-03-11T11:01:53Z

@ajgarlag, Is there any feature your bundle supports that should be added here?

There are at least two things I miss in this bundle:

Rely on any PSR-17 implementation. I would allow a third value in psr_http_message.message_factories.implementations that could be called custom. With this value, the developer must define the required PSR-17 factories in the container using FQCN as identifiers. The bundle should check that these services are available.
Provide and document a smooth upgrade path for projects thar require PSR-7 support from sensio/framework-extra-bundle:<6.0 + symfony/psr-http-message-bridge:^1.1, to symfony/psr-http-message-bridge:^2.2

Nyholm · 2021-03-11T11:11:03Z

I agree that these things should be added to the bundle. Is that something you want to help adding?

weaverryan

Thanks for this!

weaverryan · 2021-03-11T19:32:06Z

Bundle/DependencyInjection/PsrHttpMessageExtension.php

+        $configuration = $this->getConfiguration($configs, $container);
+        $config = $this->processConfiguration($configuration, $configs);
+
+        $container->setParameter('psr_http_message.response_buffer', $config['response_buffer']);


Does this need to be a parameter? Or could we pass it directly into the psr_http_message.http_foundation_factory service?

weaverryan · 2021-03-11T19:34:35Z

Bundle/DependencyInjection/PsrHttpMessageExtension.php

+    public function load(array $configs, ContainerBuilder $container): void
+    {
+        $loader = new PhpFileLoader($container, new FileLocator(\dirname(__DIR__).'/Resources/config'));
+        $loader->load('http_foundation.php');


So if I'm reading things correctly, out of the box (with no config), the bundle would just give you a service for HttpFoundationFactory, correct? That seems to make sense :)

weaverryan · 2021-03-11T19:40:44Z

Bundle/Resources/config/message_converters.php

+        ->set('psr_http_message.server_request_resolver', PsrServerRequestResolver::class)
+            ->args([new Reference('psr_http_message.psr_http_factory')])
+            ->tag('controller.argument_value_resolver')
+    ;


Is it possible to enable these automatically (and then maybe allow them to be DISABALED)? I'm trying to avoid users almost ever needing the config file. The first could always be enabled (the service is available, is there a downside)? The second could be enabled if/once psr_http_message.psr_http_factory is present. I'm trying to give the user more, with less config. WDYT?

The downside is runtime overhead on the hotpath yes.

Ok, let's not do that then ;)

derrabus · 2021-03-12T19:31:28Z

I've added a CI job that tests the bundle's behavior without any PSR-7 implementations. I think, the PR is ready now.

nicolas-grekas · 2021-03-17T07:54:23Z

Bundle/Resources/config/psr_factories.php

+        ->alias(ServerRequestFactoryInterface::class, 'psr_http_message.psr.server_request_factory')
+        ->alias(StreamFactoryInterface::class, 'psr_http_message.psr.stream_factory')
+        ->alias(UploadedFileFactoryInterface::class, 'psr_http_message.psr.uploaded_file_factory')
+        ->alias(UriFactoryInterface::class, 'psr_http_message.psr.uri_factory')


there is a soft conflict between these and https://github.com/symfony/recipes/blob/master/nyholm/psr7/1.0/config/packages/nyholm_psr7.yaml

should we remove the recipe for nyholm/psr7 in favor of this bundle?

I think we should.

I was about to send a PR, but then: ppl won't be able to add only one package to have working psr17 factories. If they install the bridge, they won't have the implementation. And if we they install the implementation and we remove the recipe, they won't have the aliases :(

Can you see derrabus#1? It's an attempt to support any PSR-17 implementation.

Then again, if we just leave everything as is, not much will break. The Nyholm recipe will steal the aliases, but they point to the same stateless classes anyway.

What we could do:

We could comment out the aliases in the Nyholm recipe and add a note that the developer should either un-comment them or install the bundle for a better integration.

Add a pack that depends on the bridge and the Nyholm implementation and remove the Nyholm recipe.

@ajgarlag Thank you! Let's merge this PR first and move your PR to this repo afterwards.

It'd like to give the pack approach a try before merging, because I'm not sure this bundle is the best approach, especially since we're still talking about a pack here...

Okay, go ahead.

@derrabus I could split my PR into two commits:

Conditionally add PSR-17 aliases in a Compiler pass if not defined previously.

Support any PSR-17 implementation.

I think that the first one could be included here. WDYT?

Let's wait for Nicolas first.

nicolas-grekas · 2021-03-18T14:52:39Z

I'm been hesitating a lot here, and now I think we should go with the "pack + recipe" approach.
Unless we make nyholm/psr7 a required dependency, we cannot wire any implementation by default.
This kills the approach proposed in this PR to me.

There are alternatives / ways forward, eg removing the recipe bound to nyholm/psr7 and not binding any recipe to psr7-pack. Or create a separate psr7-bundle that requires nyholm/psr7+this-bridge. But I think these approaches have significant drawbacks compared to the "pack + recipe" approach.

See symfony/recipes#911 for what I think is enough to make all this work.

nicolas-grekas · 2021-03-23T09:53:13Z

Closing in favor of symfony/recipes#911
Thanks @derrabus

derrabus mentioned this pull request Mar 10, 2021

New symfony/psr-http-message-bridge recipe KO's out-of-the-box with sentry/symfony symfony/recipes#909

Closed

derrabus requested review from nicolas-grekas and weaverryan March 10, 2021 17:44

stof reviewed Mar 10, 2021

View reviewed changes

Bundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

Bundle/DependencyInjection/Configuration.php Show resolved Hide resolved

derrabus force-pushed the improvement/bundle branch from a8f1fc1 to 17fd440 Compare March 10, 2021 18:08

derrabus force-pushed the improvement/bundle branch 3 times, most recently from 1481e6a to dd0aa2b Compare March 10, 2021 18:49

derrabus mentioned this pull request Mar 10, 2021

Integrate the PsrHttpMessage bundle symfony/recipes#910

Closed

derrabus force-pushed the improvement/bundle branch from dd0aa2b to 751081d Compare March 10, 2021 22:35

Nyholm approved these changes Mar 11, 2021

View reviewed changes

nicolas-grekas reviewed Mar 11, 2021

View reviewed changes

Jean85 reviewed Mar 11, 2021

View reviewed changes

composer.json Outdated Show resolved Hide resolved

derrabus force-pushed the improvement/bundle branch 2 times, most recently from 71292a1 to 358ff66 Compare March 11, 2021 09:18

nicolas-grekas approved these changes Mar 11, 2021

View reviewed changes

weaverryan reviewed Mar 11, 2021

View reviewed changes

nicolas-grekas reviewed Mar 17, 2021

View reviewed changes

nicolas-grekas mentioned this pull request Mar 17, 2021

Wire symfony/psr7-pack symfony/recipes#911

Merged

Add a bundle

6e965e0

nicolas-grekas force-pushed the improvement/bundle branch from 7ea9959 to 6e965e0 Compare March 23, 2021 09:07

nicolas-grekas closed this Mar 23, 2021

nicolas-grekas mentioned this pull request Mar 23, 2021

Add support for custom PSR-17 implementations derrabus/psr-http-message-bridge#1

Closed

derrabus mentioned this pull request Aug 27, 2021

Packs must never add/modify files in a project symfony/recipes#988

Closed

derrabus deleted the improvement/bundle branch July 26, 2023 11:49

Uh oh!

Add a bundle #94

Add a bundle #94

Uh oh!

Conversation

derrabus commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Services

psr_http_message.http_foundation_factory

PSR-17 factories

psr_http_message.psr_http_factory

Message converters

Configuration reference

Uh oh!

Uh oh!

Uh oh!

ajgarlag commented Mar 10, 2021

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajgarlag commented Mar 11, 2021

Uh oh!

Nyholm commented Mar 11, 2021

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derrabus commented Mar 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Mar 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

derrabus commented Mar 10, 2021 •

edited

Loading

nicolas-grekas commented Mar 18, 2021 •

edited

Loading