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

[FrameworkBundle] Add integration of http-client component #30419

Merged
merged 2 commits into from Mar 17, 2019

Conversation

@Ioni14
Copy link
Contributor

Ioni14 commented Mar 1, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

This PR adds the integration of the HttpClient component on FrameworkBundle.
By default, two services are provided, one implementing SFC-HttpClient, and another PSR18:

  • http_client + its autowiring alias for Symfony\Contracts\HttpClient\HttpClientInterface)
    This service is automatically set to the best HTTP client available with the configuration given under the framework.http_client key.

  • psr18.http_client + its autowiring alias for Psr\Http\Client\ClientInterface). To make it work, one needs to provide autowiring aliases for ResponseFactoryInterface and StreamFactoryInterface, which are provided by the recipe for nyholm/psr7 (but could be overriden by apps when using something else).

  • one can also configure the default options, and "scoped" clients. For example:

http_client:
    default_options:
        capath: '...'
    clients:
        github_client:
            default_options:
                base_uri: 'https://api.github.com'

This definition create a github_client service implementing SFC-HttpClient and a psr18.github_client one implementing PSR18, +2 corresponding named autowiring aliases: HttpClientInterface $githubClient, and ClientInterface $githubClient.

@Ioni14 Ioni14 force-pushed the Ioni14:fwb-http-client-integration branch from 7d979a8 to 75530a4 Mar 1, 2019

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Mar 1, 2019

@Ioni14 thanks for your hard work on this feature!

A minor question: could we maintain the global default_options option ... but remove it from the individual HTTP clients? In the example you showed in the description, we'd maintain the first default_options but we'd remove the other two. Don't they look a bit redundant? Thanks.

@Ioni14

This comment has been minimized.

Copy link
Contributor Author

Ioni14 commented Mar 1, 2019

@javiereguiluz thanks ;)

It's probably "YAGNI" but if we want to add other configuration keys for clients in the future it'll be useful to have default_options.

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 4, 2019

@nicolas-grekas nicolas-grekas force-pushed the Ioni14:fwb-http-client-integration branch from 75530a4 to 3570058 Mar 8, 2019

@nicolas-grekas nicolas-grekas force-pushed the Ioni14:fwb-http-client-integration branch 3 times, most recently from 300902d to 6acbeef Mar 8, 2019

@nicolas-grekas nicolas-grekas referenced this pull request Mar 9, 2019

Open

[HttpClient] Next steps #30502

8 of 17 tasks complete

@nicolas-grekas nicolas-grekas force-pushed the Ioni14:fwb-http-client-integration branch from 6acbeef to 8b85b09 Mar 11, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 11, 2019

PR and description updated, ready.

Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated
Show resolved Hide resolved ...ymfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated
Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd Outdated
Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
Show resolved Hide resolved ...ymfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated
->setArguments([$options, $clientConfig['max_host_connections']]);
$container->register('psr18.'.$name, Psr18Client::class)
->setAutowired(true)

This comment has been minimized.

@xabbuh

xabbuh Mar 12, 2019

Member

Do we really want to use autowiring here? IIRC we don't do that anywhere else with core services.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 12, 2019

Member

autowiring allow providing working services once the required aliases are installed
see eg https://github.com/symfony/recipes/blob/master/nyholm/psr7/1.0/config/packages/nyholm_psr7.yaml
This looked the best to achieve loose coupling at the configuration level to me. WDYT?

This comment has been minimized.

@weaverryan

weaverryan Mar 13, 2019

Member

Maybe related to this, I'm currently getting a fatal error when running debug:container or debug:config with this component enabled in the config:

PHP Fatal error:  Method class@anonymous::__toString() must not throw an exception, caught Symfony\Component\DependencyInjection\Exception\AutowiringFailedException:  in /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php on line 229
PHP Stack trace:
PHP   1. {main}() /Users/weaverryan/Sites/os/docs/http_client_play/bin/console:0
PHP   2. Symfony\Bundle\FrameworkBundle\Console\Application->run() /Users/weaverryan/Sites/os/docs/http_client_play/bin/console:38
PHP   3. Symfony\Bundle\FrameworkBundle\Console\Application->doRun() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:145
PHP   4. Symfony\Bundle\FrameworkBundle\Console\Application->doRun() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:73
PHP   5. Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:269
PHP   6. Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:87
PHP   7. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->run() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:926
PHP   8. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->execute() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Command/Command.php:255
PHP   9. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->getContainerBuilder() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php:117
PHP  10. Symfony\Component\DependencyInjection\ContainerBuilder->compile() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php:213
PHP  11. Symfony\Component\DependencyInjection\Compiler\Compiler->compile() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:754
PHP  12. Symfony\Component\DependencyInjection\Compiler\AutowirePass->process() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/Compiler.php:95
PHP  13. Symfony\Component\DependencyInjection\Compiler\AutowirePass->process() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:52
PHP  14. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php:46
PHP  15. Symfony\Component\DependencyInjection\Compiler\AutowirePass->doProcessValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:71
PHP  16. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:99
PHP  17. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php:82
PHP  18. Symfony\Component\DependencyInjection\Compiler\AutowirePass->doProcessValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:71
PHP  19. Symfony\Component\DependencyInjection\Compiler\AutowirePass->autowireCalls() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:122
PHP  20. Symfony\Component\DependencyInjection\Compiler\AutowirePass->autowireMethod() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:164
PHP  21. Symfony\Component\DependencyInjection\Compiler\AutowirePass->Symfony\Component\DependencyInjection\Compiler\{closure}() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:255

This is coming from the psr18.http_client service - not sure what the problem is though!

This comment has been minimized.

@weaverryan
Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/Resources/config/http_client.xml
Show resolved Hide resolved src/Symfony/Component/HttpClient/CurlHttpClient.php Outdated
Show resolved Hide resolved src/Symfony/Component/HttpClient/NativeHttpClient.php Outdated

@nicolas-grekas nicolas-grekas force-pushed the Ioni14:fwb-http-client-integration branch from 8b85b09 to b015045 Mar 12, 2019

Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated
->setArguments([$options, $clientConfig['max_host_connections']]);
$container->register('psr18.'.$name, Psr18Client::class)
->setAutowired(true)

This comment has been minimized.

@weaverryan

weaverryan Mar 13, 2019

Member

Maybe related to this, I'm currently getting a fatal error when running debug:container or debug:config with this component enabled in the config:

PHP Fatal error:  Method class@anonymous::__toString() must not throw an exception, caught Symfony\Component\DependencyInjection\Exception\AutowiringFailedException:  in /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php on line 229
PHP Stack trace:
PHP   1. {main}() /Users/weaverryan/Sites/os/docs/http_client_play/bin/console:0
PHP   2. Symfony\Bundle\FrameworkBundle\Console\Application->run() /Users/weaverryan/Sites/os/docs/http_client_play/bin/console:38
PHP   3. Symfony\Bundle\FrameworkBundle\Console\Application->doRun() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:145
PHP   4. Symfony\Bundle\FrameworkBundle\Console\Application->doRun() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:73
PHP   5. Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:269
PHP   6. Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:87
PHP   7. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->run() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:926
PHP   8. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->execute() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Command/Command.php:255
PHP   9. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->getContainerBuilder() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php:117
PHP  10. Symfony\Component\DependencyInjection\ContainerBuilder->compile() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php:213
PHP  11. Symfony\Component\DependencyInjection\Compiler\Compiler->compile() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:754
PHP  12. Symfony\Component\DependencyInjection\Compiler\AutowirePass->process() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/Compiler.php:95
PHP  13. Symfony\Component\DependencyInjection\Compiler\AutowirePass->process() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:52
PHP  14. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php:46
PHP  15. Symfony\Component\DependencyInjection\Compiler\AutowirePass->doProcessValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:71
PHP  16. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:99
PHP  17. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php:82
PHP  18. Symfony\Component\DependencyInjection\Compiler\AutowirePass->doProcessValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:71
PHP  19. Symfony\Component\DependencyInjection\Compiler\AutowirePass->autowireCalls() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:122
PHP  20. Symfony\Component\DependencyInjection\Compiler\AutowirePass->autowireMethod() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:164
PHP  21. Symfony\Component\DependencyInjection\Compiler\AutowirePass->Symfony\Component\DependencyInjection\Compiler\{closure}() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:255

This is coming from the psr18.http_client service - not sure what the problem is though!

->integerNode('max_host_connections')
->info('The maximum number of connections to a single host.')
->end()
->arrayNode('default_options')

This comment has been minimized.

@weaverryan

weaverryan Mar 13, 2019

Member

These are the default options for each request that's made. What about default_request_options (it's even documented as Default requests' options in HttpClient phpdoc). And also, should we make these options not automatically be used as the "default options" for all the other HTTP clients?

The problem is that there are 2 concepts conflicting right now. Does "default" mean:
A) default options for the request that can be overridden on each request
OR
B) default options that will be applied to all other HTTP clients

Right now, it's both - that confused me. It's really (A), so maybe we should make it only (A) and clarify the name.

This comment has been minimized.

@stof

stof Mar 13, 2019

Member

right now, it is default options applied to all other HTTP clients for their default request options...

This comment has been minimized.

@weaverryan

weaverryan Mar 13, 2019

Member

exactly - the "default" means to things - it's (A) and (B).

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 13, 2019

Member

Correct, it's A and B. If one wants to configure the default request options for the default client only, one should set them under framework.http_client.clients.http_client.default_options (since http_client is the name of the service that is aliased for HttpClientInterface).
I'm not sure I understand how your suggestion would apply. To me, all is fine :)

This comment has been minimized.

@weaverryan

weaverryan Mar 14, 2019

Member

I was suggesting that we remove (B), but it's subjective. The question is: is it valuable for these options to cascade onto other clients? Or is it annoying/surprising, and then I am unsetting options on my individual clients, because I don't want them to cascade from my default client?

I'm not convinced that re-using options across clients will be all that common - things like auth, base_uri, headers all seem very client-specific. I could argue that it might be surprising if my auth on my default client starts getting sent to another server via another client (because I didn't realize the options would cascade down to my client).

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 14, 2019

Member

e.g. cabundle/capath: you want to configure them once for all

This comment has been minimized.

@weaverryan

weaverryan Mar 17, 2019

Member

Hmm, ok, really fine by me then.

@weaverryan

This comment has been minimized.

Copy link
Member

weaverryan commented Mar 13, 2019

If/when #30547 is merged, the auth option in the config will need to change and the bearer_auth will need to be added.

@nicolas-grekas nicolas-grekas force-pushed the Ioni14:fwb-http-client-integration branch 3 times, most recently from bfa816f to 89451d3 Mar 15, 2019

@weaverryan

This comment has been minimized.

Copy link
Member

weaverryan commented Mar 17, 2019

Tests failing on low deps, but other than that - +1 from me!

@nicolas-grekas nicolas-grekas force-pushed the Ioni14:fwb-http-client-integration branch from 89451d3 to 9a91fb4 Mar 17, 2019

@nicolas-grekas nicolas-grekas force-pushed the Ioni14:fwb-http-client-integration branch from 9a91fb4 to f2d2cf3 Mar 17, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 17, 2019

Thank you @Ioni14.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 17, 2019

Thank you @nicoweb

@nicolas-grekas nicolas-grekas merged commit f2d2cf3 into symfony:master Mar 17, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Mar 17, 2019

feature #30419 [FrameworkBundle] Add integration of http-client compo…
…nent (Ioni14, nicoweb)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[FrameworkBundle] Add integration of http-client component

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This PR adds the integration of the HttpClient component on FrameworkBundle.
By default, two services are provided, one implementing SFC-HttpClient, and another PSR18:
* `http_client` + its autowiring alias for `Symfony\Contracts\HttpClient\HttpClientInterface`)
This service is automatically set to the best HTTP client available with the configuration given under the `framework.http_client` key.
* `psr18.http_client` + its autowiring alias for `Psr\Http\Client\ClientInterface`). To make it work, one needs to provide autowiring aliases for `ResponseFactoryInterface` and `StreamFactoryInterface`, which are provided by [the recipe](https://github.com/symfony/recipes-contrib/blob/master/nyholm/psr7/1.0/config/packages/nyholm_psr7.yaml) for `nyholm/psr7` (but could be overriden by apps when using something else).

* one can also configure the default options, and "scoped" clients. For example:
```yaml
http_client:
    default_options:
        capath: '...'
    clients:
        github_client:
            default_options:
                base_uri: 'https://api.github.com'
```

This definition create a `github_client` service implementing SFC-HttpClient and a `psr18.github_client` one implementing PSR18, +2 corresponding named autowiring aliases: `HttpClientInterface $githubClient`,  and `ClientInterface $githubClient`.

Commits
-------

f2d2cf3 work with attributes for xml config
0023a71 [FrameworkBundle] Add integration of http-client component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.