-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Comments
My guess is that the activation strategy that drives the 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! |
H Nicolas, My guesses in the bug were that the issue was with the initialisation of the profiler and not necessarily the 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 |
Can you please confirm that #60489 fixes the issue? |
C'est tres bien, merci. |
…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
Uh oh!
There was an error while loading. Please reload this page.
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 HttpClienthttps://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, invokingProfilerStateChecker
,$this->container->get('profiler')
is not retrievable from the container yet.$this->container->get('profiler')?->isEnabled()
returnsnull
as a result.I've traced that the
_construct
is not being called on theProfiler
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
The text was updated successfully, but these errors were encountered: