Skip to content

[HttpClient] Bug on upcoming 7.3 for TraceableHttpClient profiling #60481

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

Closed
silverbackdan opened this issue May 19, 2025 · 4 comments · Fixed by #60489
Closed

[HttpClient] Bug on upcoming 7.3 for TraceableHttpClient profiling #60481

silverbackdan opened this issue May 19, 2025 · 4 comments · Fixed by #60489

Comments

@silverbackdan
Copy link

silverbackdan commented May 19, 2025

Symfony version(s) affected

7.3-BETA2

Description

I cannot see any notes in the ChangeLog about this update to add a disabled constructor argument and an additional check before profiling the HttpClient
https://github.com/symfony/http-client/blob/7.3/TraceableHttpClient.php#L27-L43

In my tests, this is preventing some http client calls to a cache server endpoint from being collected, resulting in a false-negative test result stating my application has not done the HTTP request to clear a resource from a cache.

Additionally I cannot see docs just yet on manually disabling or enabling the profiler.

During the test, when $this->disabled?->__invoke() is called, invoking ProfilerStateChecker, $this->container->get('profiler') is not retrievable from the container yet. $this->container->get('profiler')?->isEnabled() returns null as a result.

I've traced that the _construct is not being called on the Profiler until after the Http requests have been made now. Has a priority changed, or something new required in Behat tests for this?

How to reproduce

I'm sorry this is not a minimal reproduction, but sometimes having this brought up will quickly get a eurika moment from the core devs. But the tests are failing on the next version testing in my bundle here:
https://github.com/components-web-app/api-components-bundle/

Possible Solution

No response

Additional Context

No response

@nicolas-grekas
Copy link
Member

My guess is that the activation strategy that drives the $defaultEnabled argument of ProfilerStateChecker is not correct for the test env: there, it should be true, while it's false right now (because the current default is to set it to false when running on the CLI).

We'd need a better trigger than checking the SAPI. Something that the console component should drive I imagine.

Note that's its funny how what you describe tells about everything around this argument, and specifically misses it, while it's on the same line as the call to isEnabled :)

Also that's not a BC break: a BC break is an interface-level change. Here, it's a behavioral one: just a bug.

Anyway, thanks for the report, it's still useful to understand the deeper issue!

@silverbackdan silverbackdan changed the title [HttpClient] BC break on upcoming 7.3 for TraceableHttpClient profiling [HttpClient] Bug on upcoming 7.3 for TraceableHttpClient profiling May 20, 2025
@silverbackdan
Copy link
Author

H Nicolas,

My guesses in the bug were that the issue was with the initialisation of the profiler and not necessarily the $defaultEnabled argument. Sorry to skirt around this being the likely cause of the issue.

I'll check in my test environment about setting this value, and perhaps enabling it just for the traceable HTTP client in 7.3 as I don't need profiling of other services.

And I see about BC - BC in my mind before this was just backwards compatibility on a level where a new version has a different behaviour to the previous with the same implementation. Understood for future PRs.

Thanks as always

@nicolas-grekas
Copy link
Member

Can you please confirm that #60489 fixes the issue?

@silverbackdan
Copy link
Author

C'est tres bien, merci.

nicolas-grekas added a commit that referenced this issue May 20, 2025
…orators (nicolas-grekas)

This PR was merged into the 7.3 branch.

Discussion
----------

[FrameworkBundle] Fix activation strategy of traceable decorators

| Q             | A
| ------------- | ---
| Branch?       | 7.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #60481
| License       | MIT

With this change, we're going to auto-enable traceable decorators in the test env, which should fix #60481
The added logic relies on the SymfonyRuntime component, which is the one exposing the currently running app under `$GLOBALS['app']`. If the global var is not found, we just fallback to auto-enabled mode.

Commits
-------

307d743 [FrameworkBundle] Fix activation strategy of traceable decorators
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants