-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[HttpClient] Add note about a required decoration priority > 5 #18087
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
[HttpClient] Add note about a required decoration priority > 5 #18087
Conversation
756b175
to
5f875cf
Compare
This PR assumed a decoration like this is the intended way: services:
decorated.http_client:
class: HttpClientDecorator
arguments: [decorated.http_client.inner]
decorates: http_client
decoration_priority: 10 Issues like this getsentry/sentry-symfony#700 show, it is not clear how to decorate (all ways http clients can be bootstrapped) in the correct way. |
Friendly ping @nicolas-grekas |
I'm not sure it makes sense documenting this. What Sentry does is uncommon to me. |
I searches around how the http client had been decorated in examples, issues and more. It shows there is a bigger diversity but in general there aren't that many sources, if you search for help. Here are sources where others decorated the scoped clients:
If the tagged clients are decorated, the decorator does not receive the final URL which should cause issues in may use cases. |
This made me realize that symfony/symfony#49513 should have solved this issue with decoration. This could be worth documenting. |
I prefer a different way of documenting this as well, especially would prefer a fix for this :) |
friendly ping @fancyweb, as you are the author of |
After reading the entire discussion, let's close this one as "won't merge" ... and let's add the missing docs about the new HttpClient decoration strategy in #19363. Thanks. |
In symfony/symfony#47836 the decoration priority of the
TraceableHttpClient
services had been increased to 5. This results in the own decorator with a default priority of 5 is not added to the chain of any scoped client.As this is not obvious and caused some trouble for us, I like to add the info to the documentation.
Increasing the priority is only needed when
TraceableHttpClient
is decorated but I am not sure how to word the condition, nor if this is really important to tell here.