Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 2 commits intosymfony:masterfromIoni14:fwb-http-client-integration
Mar 17, 2019
Merged

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

nicolas-grekas merged 2 commits intosymfony:masterfromIoni14:fwb-http-client-integration
Mar 17, 2019

Conversation

@Ioni14
Copy link
Contributor

@Ioni14Ioni14 commentedMar 1, 2019•
edited by nicolas-grekas
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
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 forSymfony\Contracts\HttpClient\HttpClientInterface)
    This service is automatically set to the best HTTP client available with the configuration given under theframework.http_client key.

  • psr18.http_client + its autowiring alias forPsr\Http\Client\ClientInterface). To make it work, one needs to provide autowiring aliases forResponseFactoryInterface andStreamFactoryInterface, which are provided bythe recipe fornyholm/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 agithub_client service implementing SFC-HttpClient and apsr18.github_client one implementing PSR18, +2 corresponding named autowiring aliases:HttpClientInterface $githubClient, andClientInterface $githubClient.

@javiereguiluz
Copy link
Member

@Ioni14 thanks for your hard work on this feature!

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

@Ioni14
Copy link
ContributorAuthor

@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 havedefault_options.

@nicolas-grekas
Copy link
Member

PR and description updated, ready.

->setArguments([$options,$clientConfig['max_host_connections']]);

$container->register('psr18.'.$name, Psr18Client::class)
->setAutowired(true)
Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

autowiring allow providing working services once the required aliases are installed
see eghttps://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?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe related to this, I'm currently getting a fatal error when runningdebug:container ordebug: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 229PHP Stack trace:PHP   1. {main}() /Users/weaverryan/Sites/os/docs/http_client_play/bin/console:0PHP   2. Symfony\Bundle\FrameworkBundle\Console\Application->run() /Users/weaverryan/Sites/os/docs/http_client_play/bin/console:38PHP   3. Symfony\Bundle\FrameworkBundle\Console\Application->doRun() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:145PHP   4. Symfony\Bundle\FrameworkBundle\Console\Application->doRun() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:73PHP   5. Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:269PHP   6. Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:87PHP   7. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->run() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:926PHP   8. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->execute() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Command/Command.php:255PHP   9. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->getContainerBuilder() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php:117PHP  10. Symfony\Component\DependencyInjection\ContainerBuilder->compile() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php:213PHP  11. Symfony\Component\DependencyInjection\Compiler\Compiler->compile() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:754PHP  12. Symfony\Component\DependencyInjection\Compiler\AutowirePass->process() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/Compiler.php:95PHP  13. Symfony\Component\DependencyInjection\Compiler\AutowirePass->process() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:52PHP  14. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php:46PHP  15. Symfony\Component\DependencyInjection\Compiler\AutowirePass->doProcessValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:71PHP  16. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:99PHP  17. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php:82PHP  18. Symfony\Component\DependencyInjection\Compiler\AutowirePass->doProcessValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:71PHP  19. Symfony\Component\DependencyInjection\Compiler\AutowirePass->autowireCalls() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:122PHP  20. Symfony\Component\DependencyInjection\Compiler\AutowirePass->autowireMethod() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:164PHP  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 thepsr18.http_client service - not sure what the problem is though!

Copy link
Member

Choose a reason for hiding this comment

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

->setArguments([$options,$clientConfig['max_host_connections']]);

$container->register('psr18.'.$name, Psr18Client::class)
->setAutowired(true)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe related to this, I'm currently getting a fatal error when runningdebug:container ordebug: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 229PHP Stack trace:PHP   1. {main}() /Users/weaverryan/Sites/os/docs/http_client_play/bin/console:0PHP   2. Symfony\Bundle\FrameworkBundle\Console\Application->run() /Users/weaverryan/Sites/os/docs/http_client_play/bin/console:38PHP   3. Symfony\Bundle\FrameworkBundle\Console\Application->doRun() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:145PHP   4. Symfony\Bundle\FrameworkBundle\Console\Application->doRun() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:73PHP   5. Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:269PHP   6. Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:87PHP   7. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->run() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Application.php:926PHP   8. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->execute() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/Console/Command/Command.php:255PHP   9. Symfony\Bundle\FrameworkBundle\Command\ContainerDebugCommand->getContainerBuilder() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php:117PHP  10. Symfony\Component\DependencyInjection\ContainerBuilder->compile() /Users/weaverryan/Sites/os/symfony/src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php:213PHP  11. Symfony\Component\DependencyInjection\Compiler\Compiler->compile() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php:754PHP  12. Symfony\Component\DependencyInjection\Compiler\AutowirePass->process() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/Compiler.php:95PHP  13. Symfony\Component\DependencyInjection\Compiler\AutowirePass->process() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:52PHP  14. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php:46PHP  15. Symfony\Component\DependencyInjection\Compiler\AutowirePass->doProcessValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:71PHP  16. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:99PHP  17. Symfony\Component\DependencyInjection\Compiler\AutowirePass->processValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AbstractRecursivePass.php:82PHP  18. Symfony\Component\DependencyInjection\Compiler\AutowirePass->doProcessValue() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:71PHP  19. Symfony\Component\DependencyInjection\Compiler\AutowirePass->autowireCalls() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:122PHP  20. Symfony\Component\DependencyInjection\Compiler\AutowirePass->autowireMethod() /Users/weaverryan/Sites/os/symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php:164PHP  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 thepsr18.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')
Copy link
Member

Choose a reason for hiding this comment

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

These are the default options for each request that's made. What aboutdefault_request_options (it's even documented asDefault requests' options inHttpClient phpdoc). And also, should we make these optionsnot 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 itonly (A) and clarify the name.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@nicolas-grekasnicolas-grekasMar 13, 2019•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 seemvery 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).

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, ok, really fine by me then.

@weaverryan
Copy link
Member

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

@weaverryan
Copy link
Member

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

@nicolas-grekas
Copy link
Member

Thank you@Ioni14.

@nicolas-grekas
Copy link
Member

Thank you@nicoweb

@nicolas-grekasnicolas-grekas merged commitf2d2cf3 intosymfony:masterMar 17, 2019
nicolas-grekas added a commit that referenced this pull requestMar 17, 2019
…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:```yamlhttp_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 config0023a71 [FrameworkBundle] Add integration of http-client component
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh left review comments

@stofstofstof requested changes

@weaverryanweaverryanweaverryan approved these changes

@derrabusderrabusderrabus approved these changes

+2 more reviewers

@sstoksstoksstok left review comments

@linaorilinaorilinaori left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

11 participants

@Ioni14@javiereguiluz@nicolas-grekas@weaverryan@stof@sstok@derrabus@linaori@xabbuh@carsonbot@nicoweb
[8]ページ先頭
©2009-2025 Movatter.jp