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] change the way http clients are configured by leveraging ScopingHttpClient #30674

Merged
merged 1 commit into from Apr 3, 2019

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Copy link
Member

commented Mar 24, 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 allows configuring scoped HTTP clients ("scoped_clients" replaces the previous "clients" options):

framework:
  http_client:
    max_host_connections: 4
    default_options:
      # ...
    scoped_clients:
      github_client:
        base_uri: https://api.github.com
        headers:
          Authorization: token abc123
          # ...

The base URI is turned into a scoping regular expression so that the token will be sent only when the github_client service is requesting the corresponding URLs.
When the base URI is too restrictive, the scope option can be used explicitly to define the regexp that URLs must match before any other options are applied.

All defined scopes are passed to a new scoping_http_client service, that can be used to hit endpoints with authentication pre-configured for several hosts. Its named autowiring alias is HttpClientInterface $scopingClient (this cannot be done with http_client as we want safe defaults, e.g. credentials should not be used implicitly when writing webhooks/crawlers.)

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fwb-hc-scope branch 3 times, most recently from 1fb972a to 2580204 Mar 24, 2019

@joelwurtz

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

What do you think about using the same concept as the RequestMatcher (like a ClientRequestMatcher) for tiggering this ?

So someone could match on headers (like content-type / accept and even if the header is already present) or even body response ?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

So someone could match on headers (like content-type / accept and even if the header is already present) or even body response ?

I suppose you mean body request?
Anyway, why not, but I don't know how precisely, and I would wait for someone to have a use case first (and contribute the code ;) )

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fwb-hc-scope branch from 2580204 to 86ea4b4 Mar 29, 2019

->cannotBeEmpty()
->end()
->scalarNode('base_uri')
->info('The URI to resolve relative URLs, following rules in RFC 3985, section 2.')

This comment has been minimized.

Copy link
@weaverryan

weaverryan Mar 29, 2019

Member

This is not the full purpose of this option. Well, it IS the full purpose if scope is not defined :p. But assuming it's not defined, this is both the base URI to use, but also for matching. Gotta find a way to "squeeze" that in. We could add an info on the parent scopes node?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 29, 2019

Author Member

info updated

Show resolved Hide resolved ...ymfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated
Show resolved Hide resolved src/Symfony/Bundle/FrameworkBundle/Resources/config/http_client.xml Outdated

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fwb-hc-scope branch 5 times, most recently from 5bd1967 to ab34374 Mar 29, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

Status: needs review
Green :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fwb-hc-scope branch 2 times, most recently from 51126c4 to 56b6bae Apr 2, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

(failure is unrelated: some old commit for contracts us checked out)
Votes pending

@weaverryan
Copy link
Member

left a comment

+1 from a feature standpoint - I chatted about this with Nicolas.

<argument type="service" id="http_client" />
<argument type="collection" />
</service>
<service id="Symfony\Contracts\HttpClient\HttpClientInterface $scopingHttpClient" alias="scoping_http_client" />

This comment has been minimized.

Copy link
@fabpot

fabpot Apr 2, 2019

Member

So, a scoped HTTP client is the result, but $scopingHttpClientdoes not sound good here.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 2, 2019

Member

I thought that $scopedHttpClient sounded like it was already "scoped". But really, this gives you a big object that is aware of all of the scopes, and has the ability to scope, based on the URI. The naming came from this thinking, but maybe it's still confusing?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 2, 2019

Author Member

What Ryan said. The same reasoning is why ScopingHttpClient, the class, is named that way also.

@fabpot

fabpot approved these changes Apr 3, 2019

Copy link
Member

left a comment

Still not a big fan of the names, but I don't have a better alternative.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:fwb-hc-scope branch from 56b6bae to f1a26b9 Apr 3, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

I removed the scoping_http_client service and related autowiring aliases. It can be reintroduced later if needed.

@nicolas-grekas nicolas-grekas merged commit f1a26b9 into symfony:master Apr 3, 2019

2 of 3 checks passed

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

nicolas-grekas added a commit that referenced this pull request Apr 3, 2019

feature #30674 [FrameworkBundle] change the way http clients are conf…
…igured by leveraging ScopingHttpClient (nicolas-grekas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[FrameworkBundle] change the way http clients are configured by leveraging ScopingHttpClient

| 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 allows configuring scoped HTTP clients ("scoped_clients" replaces the previous "clients" options):

```yaml
framework:
  http_client:
    max_host_connections: 4
    default_options:
      # ...
    scoped_clients:
      github_client:
        base_uri: https://api.github.com
        headers:
          Authorization: token abc123
          # ...
```

The base URI is turned into a scoping regular expression so that the token will be sent only when the `github_client` service is requesting the corresponding URLs.
When the base URI is too restrictive, the `scope` option can be used explicitly to define the regexp that URLs must match before any other options are applied.

~All defined scopes are passed to a new `scoping_http_client` service, that can be used to hit endpoints with authentication pre-configured for several hosts. Its named autowiring alias is `HttpClientInterface $scopingClient` (this cannot be done with `http_client` as we want safe defaults, e.g. credentials should not be used implicitly when writing webhooks/crawlers.)~

Commits
-------

f1a26b9 [FrameworkBundle] change the way http clients are configured by leveraging ScopingHttpClient

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:fwb-hc-scope branch Apr 3, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

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.