I prefer a different way of documenting this as well, especially would prefer a fix for this :) To me this is no niche scenario tho. When sentry fixes its decoration, they are most likely very much effected to decide of a priority and cant stick to zero.

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. Decoration is communicated to be the key extension tool and imho it is not enough to just point to the general decoration document while it is unclear what exactly to decorate and how it behaves in the different ways the clients can be bootstrapped. It is kinda offtopic to the current state of the PR, but I could extend it to make decoration more clear and show the intended way.

Here are sources where others decorated the scoped clients:

• https://github.com/symfony/symfony/discussions/49246
• https://jolicode.com/blog/aggressive-caching-with-symfony-http-client

If the tagged clients are decorated, the decorator does not receive the final URL which should cause issues in may use cases. Additionally the decorator is twice in the chain as the sentry issue shows. I did not really debug how this happen, but most likely because the default http_client service as well as the scoped http service are decorated and "merged together" in a later step. On the other hand If the http_client service is decorated, we need a minimum priority of 5/6 because otherwise the scoped clients are already decorated, when our decoration would be processed which could maybe considered an resolvable issue. At least to me it is hidden knowledge you can just find out by try and error or could end up doing exactly this tagged client decoration, because your own decorator does not appear otherwise.

Another general problem you'll face when decorating every tagged client and being placed in front of the ScopingHttpClient is to not having the complete URL. Depending on the type of the scoped client you'll often just receive the relative url and the base_uri is prefixed by the ScopingHttpClient which all later clients receive in the request. I noticed that the URL is also handled by your client which does not work correctly then.

@ste93cry do you mean if the TraceableHttpClient is actively used / part of the decoration? As I wrote at the end of #700 it does not happen with TraceableHttpClient being part of the chain. It happened on our servers and took me quiet some time to find the root cause in my environment where debugging and the webprofiler are active.

This PR assumed a decoration like this is the intended way:

´´´yaml services: decorated.http_client: class: HttpClientDecorator arguments: [decorated.http_client.inner] decorates: http_client decoration_priority: 10 ´´´

Issues like this https://github.com/getsentry/sentry-symfony/issues/700 show, it is not clear how to decorate (all ways http clients can be bootstrapped) in the correct way.

Removed tag

Yeah, I know this had to be the source. First of all I am by far no decoration expert and just learned a lot of the container building in the last weeks. I indeed cannot find something official the way we are doing it but to me it is simple, straight forward and I cannot see a "reasonable" scenario it fails. One could be self created services tagged by http_client.client which is no promoted tag in any way. Symfony in the few extension descriptions it has, just points to a general decoration document which surely does not help here.

Removed parameter use in test

This MR Fixes #700 and resolves #701

I had a look how this bundle decorates the http-client. It iterates over all tagged http client services and creates new decorated services of them. This is not the intended decoration way and should also not be necessary (?).

Instead just the default http_client needs to be decorated which is put into all other (scoped) services. This changes the Sentry\SentryBundle\Tracing\HttpClient\TraceableHttpClient not to be the first service in the decoration chain anymore, but this is hopefully not important.

Note: I did not test the function of sentries TraceableHttpClient before or after my change, I just tested the decoration chain to be intact for the default http client or when used as a scoped service. I tested it in Symfony 6.2 and can test this next week also in Symfony 5.4 which has no bigger internal changed behavior in this regard tho.

[HttpClient] Add note about a required decoration priority > 5

In https://github.com/symfony/symfony/pull/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.

[HttpCache] Inject $env in example

[HttpFoundation] Add support for the 103 status code

[Mailer] add more info for debugging

minor #17950 [Mailer] add more info for debugging (94noni)

This PR was submitted for the 6.2 branch but it was squashed and merged into the 5.4 branch instead.

Discussion

[Mailer] add more info for debugging

Hi

I did not understand this section of getDebug until I navigate the code and see the swap to do with

• MailerInterface and TransportInterface

MailerInterface

$a = $this->mailer->send($email); // $a is null

TransportInterface

$a = $this->mailer->send($email); // $a is SentMessage

Commits

82606fe6c [Mailer] add more info for debugging

Tweak

Merge branch '5.4' into 6.2

• 5.4: [Mailer] add more info for debugging

minor #17969 [HttpCache] Inject $env in example (HypeMC)

This PR was merged into the 6.2 branch.

Discussion

[HttpCache] Inject $env in example

A sort of followup to #17639, IMO makes the example a bit clearer.

Commits

7ae2f9409 [HttpCache] Inject $env in example

Merge branch '6.2' into 6.3

• 6.2: [Mailer] add more info for debugging [HttpCache] Inject $env in example

minor #18055 [HttpFoundation] Add support for the 103 status code (alexandre-daubois)

This PR was merged into the 6.3 branch.

Discussion

[HttpFoundation] Add support for the 103 status code

Fixes https://github.com/symfony/symfony-docs/issues/18053

Commits

15a408f96 [HttpFoundation] Add support for the 103 status code

Tweaks

Fix the reference links

mention french inflector and interface in String doc

minor #18060 [String] Mention FrenchInflector and Interface (MrYamous)

This PR was submitted for the 6.2 branch but it was merged into the 5.4 branch instead.

Discussion

[String] Mention FrenchInflector and Interface

Mention FrenchInflector :rooster:

Commits

d173ef3ba mention french inflector and interface in String doc

Tweak

Merge branch '5.4' into 6.2

• 5.4: mention french inflector and interface in String doc

Merge branch '6.2' into 6.3

• 6.2: mention french inflector and interface in String doc

Update create_custom_field_type.rst

Adding return type for setNormalizer callback function

minor #18059 Update create_custom_field_type.rst (hbgamra)

This PR was submitted for the 6.3 branch but it was merged into the 6.2 branch instead.

Discussion

Update create_custom_field_type.rst

Adding return type for setNormalizer callback function

Commits

d887b5aa0 Update create_custom_field_type.rst

Merge branch '6.2' into 6.3

• 6.2: Update create_custom_field_type.rst

[DependencyInjection] Use constructor property promotion (CPP)

This is a small adjustment of the Httplug blockof the HttpClient documentation, following the changes of https://github.com/symfony/symfony/pull/49691

Review feedback

Reverting checked interface existence as well

Revert deprecating HttplugClient::sendRequest()

Revert deprecating HttplugClient::sendRequest()

there should be also no HttpClient service deprecation then as this would actually disable injecting httplug with this use case anyway

That's two very different things to me. Removing an autowiring alias just sends a message saying that in the context of symfony apps (where we have some authority), we're discouraging using this interface. On the broader php ecosystem, we don't, and thus we can't.

Ok got it, I have to see the remaining use outside of a symfony bundle integration. 👍

Expect deprecation in tests

Review feedback

Expect deprecation in tests

Review feedback