[WebProfilerBundle ] Fixed an edge case on WDT loading #10773
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There is an issue here: you are handling retriesCount as a global variable. This is broken. Each Ajax loading through And for other elements loaded through Ajax, retyring may not always make sense. IMO, the |
| @@ -239,7 +239,7 @@ public function toolbarAction(Request $request, $token) | |||
| $this->profiler->disable(); | |||
|
|
|||
| if (!$profile = $this->profiler->loadProfile($token)) { | |||
| return new Response('', 200, array('Content-Type' => 'text/html')); | |||
| return new Response('', 204, array('Content-Type' => 'text/html')); | |||
There was a problem hiding this comment.
why 204 ? a 400 is a better status code IMHO.
There was a problem hiding this comment.
404 then, not 400. but indeed, 404 would be better if the profile does not exist
There was a problem hiding this comment.
This is exactly why I asked the question if it was accurate :)
However I didn't take the initiative of sending a 404 because it was a 200 before and thought there was a reason to that
But yeah I also prefer 404
|
@stof yeah you're right I will update it, but I did not know that something else were using the |
For example when the kernel.terminate last a really long time
|
@stof updated |
|
Thank you @tucksaun. |
fabpot
added a commit
that referenced
this pull request
Apr 25, 2014
…cksaun) This PR was squashed before being merged into the 2.3 branch (closes #10773). Discussion ---------- [WebProfilerBundle ] Fixed an edge case on WDT loading | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #6824 ? | License | MIT | Doc PR | none In some case you can notice the WDT just disappears. By tracking it down, I noticed that the XHR call returns an empty response with 200 as status code, but if you go directly on the _wdt/my_token URL it works correctly. What's happening is that when you have a slow listener on `kernel.terminate` (for example the SwiftMailer one with a slow connection), you response (and therefore the WDT javascript) is sent and processed by the browser, the XHR call is done, but the Profiler storage didn't happened yet so no profiling data is available and the `ProfilerController` just sends an empty response with 200 as status code. Here we change to instead send a specific status code, and treat it in javascript by retrying several times before failing. The question are: * Is 204 the most appropriate response code? * Are 500 ms and 5 max retries good values? Commits ------- 9d885ed [WebProfilerBundle ] Fixed an edge case on WDT loading
tucksaun
added a commit
to tucksaun/symfony
that referenced
this pull request
Apr 30, 2014
tucksaun
added a commit
to tucksaun/symfony
that referenced
this pull request
Apr 30, 2014
tucksaun
added a commit
to tucksaun/symfony
that referenced
this pull request
Apr 30, 2014
fabpot
added a commit
that referenced
this pull request
Apr 30, 2014
…ty token (tucksaun) This PR was merged into the 2.3 branch. Discussion ---------- [WebProfilerBundle] Fixed profiler seach/homepage with empty token | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #10806 | License | MIT | Doc PR | none Commits ------- 16dd0e5 [WebProfilerBundle] added test case for #10773 5b91e70 [WebProfilerBundle] fixed profiler homepage, fixed #10806 7b425d2 [WebProfilerBundle] Added test case for #10806
fabpot
added a commit
that referenced
this pull request
May 12, 2014
* 2.3: [Console]Improve formatter for double-width character Lower mbstring dep, remove it for Yaml and CssSelector components [Security] Add check for supported attributes in AclVoter [Form] Fixed TrimListenerTest as of PHP 5.5 Added more IDE links [DependencyInjection] Fix parameter description in ConfigurationExtensionInterface [Finder] fixed typehint of the Finder::addAdapter() method [TwigBridge][Transchoice] set %count% from the current context. [DependencyInjection] Fix travis unit tests Update PHPUnit before run [Validator] fixed wrong test [WebProfilerBundle] added test case for #10773 [WebProfilerBundle] fixed profiler homepage, fixed #10806 [WebProfilerBundle] Added test case for #10806 changed travis to run on the nightly builds of HHVM until everything gets stable Fixed issue #5427 Allow URLs that don't contain a path Conflicts: .travis.yml
fabpot
added a commit
that referenced
this pull request
May 12, 2014
* 2.4: Lower mbstring dependency [Console]Improve formatter for double-width character Lower mbstring dep, remove it for Yaml and CssSelector components [Security] Add check for supported attributes in AclVoter [Form] Fixed TrimListenerTest as of PHP 5.5 Added more IDE links [DependencyInjection] Fix parameter description in ConfigurationExtensionInterface [Security] fixed wrong PHPDoc of the TokenGeneratorInterface [Finder] fixed typehint of the Finder::addAdapter() method [TwigBridge][Transchoice] set %count% from the current context. [DependencyInjection] Fix travis unit tests Update PHPUnit before run [Validator] fixed wrong test [WebProfilerBundle] added test case for #10773 [WebProfilerBundle] fixed profiler homepage, fixed #10806 [WebProfilerBundle] Added test case for #10806 changed travis to run on the nightly builds of HHVM until everything gets stable Fixed issue #5427 Allow URLs that don't contain a path Conflicts: .travis.yml src/Symfony/Component/Console/Application.php
Closed
ostrolucky
pushed a commit
to ostrolucky/symfony
that referenced
this pull request
Mar 25, 2018
ostrolucky
pushed a commit
to ostrolucky/symfony
that referenced
this pull request
Mar 25, 2018
* 2.3: [Console]Improve formatter for double-width character Lower mbstring dep, remove it for Yaml and CssSelector components [Security] Add check for supported attributes in AclVoter [Form] Fixed TrimListenerTest as of PHP 5.5 Added more IDE links [DependencyInjection] Fix parameter description in ConfigurationExtensionInterface [Finder] fixed typehint of the Finder::addAdapter() method [TwigBridge][Transchoice] set %count% from the current context. [DependencyInjection] Fix travis unit tests Update PHPUnit before run [Validator] fixed wrong test [WebProfilerBundle] added test case for symfony#10773 [WebProfilerBundle] fixed profiler homepage, fixed symfony#10806 [WebProfilerBundle] Added test case for symfony#10806 changed travis to run on the nightly builds of HHVM until everything gets stable Fixed issue symfony#5427 Allow URLs that don't contain a path Conflicts: .travis.yml
ostrolucky
pushed a commit
to ostrolucky/symfony
that referenced
this pull request
Mar 25, 2018
* 2.4: Lower mbstring dependency [Console]Improve formatter for double-width character Lower mbstring dep, remove it for Yaml and CssSelector components [Security] Add check for supported attributes in AclVoter [Form] Fixed TrimListenerTest as of PHP 5.5 Added more IDE links [DependencyInjection] Fix parameter description in ConfigurationExtensionInterface [Security] fixed wrong PHPDoc of the TokenGeneratorInterface [Finder] fixed typehint of the Finder::addAdapter() method [TwigBridge][Transchoice] set %count% from the current context. [DependencyInjection] Fix travis unit tests Update PHPUnit before run [Validator] fixed wrong test [WebProfilerBundle] added test case for symfony#10773 [WebProfilerBundle] fixed profiler homepage, fixed symfony#10806 [WebProfilerBundle] Added test case for symfony#10806 changed travis to run on the nightly builds of HHVM until everything gets stable Fixed issue symfony#5427 Allow URLs that don't contain a path Conflicts: .travis.yml src/Symfony/Component/Console/Application.php
jderusse
pushed a commit
to jderusse/symfony
that referenced
this pull request
Dec 15, 2020
* 2.3: [Console]Improve formatter for double-width character Lower mbstring dep, remove it for Yaml and CssSelector components [Security] Add check for supported attributes in AclVoter [Form] Fixed TrimListenerTest as of PHP 5.5 Added more IDE links [DependencyInjection] Fix parameter description in ConfigurationExtensionInterface [Finder] fixed typehint of the Finder::addAdapter() method [TwigBridge][Transchoice] set %count% from the current context. [DependencyInjection] Fix travis unit tests Update PHPUnit before run [Validator] fixed wrong test [WebProfilerBundle] added test case for symfony#10773 [WebProfilerBundle] fixed profiler homepage, fixed symfony#10806 [WebProfilerBundle] Added test case for symfony#10806 changed travis to run on the nightly builds of HHVM until everything gets stable Fixed issue symfony#5427 Allow URLs that don't contain a path Conflicts: .travis.yml
jderusse
pushed a commit
to jderusse/symfony
that referenced
this pull request
Dec 15, 2020
* 2.4: Lower mbstring dependency [Console]Improve formatter for double-width character Lower mbstring dep, remove it for Yaml and CssSelector components [Security] Add check for supported attributes in AclVoter [Form] Fixed TrimListenerTest as of PHP 5.5 Added more IDE links [DependencyInjection] Fix parameter description in ConfigurationExtensionInterface [Security] fixed wrong PHPDoc of the TokenGeneratorInterface [Finder] fixed typehint of the Finder::addAdapter() method [TwigBridge][Transchoice] set %count% from the current context. [DependencyInjection] Fix travis unit tests Update PHPUnit before run [Validator] fixed wrong test [WebProfilerBundle] added test case for symfony#10773 [WebProfilerBundle] fixed profiler homepage, fixed symfony#10806 [WebProfilerBundle] Added test case for symfony#10806 changed travis to run on the nightly builds of HHVM until everything gets stable Fixed issue symfony#5427 Allow URLs that don't contain a path Conflicts: .travis.yml src/Symfony/Component/Console/Application.php
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In some case you can notice the WDT just disappears.
By tracking it down, I noticed that the XHR call returns an empty response with 200 as status code, but if you go directly on the _wdt/my_token URL it works correctly.
What's happening is that when you have a slow listener on
kernel.terminate(for example the SwiftMailer one with a slow connection), you response (and therefore the WDT javascript) is sent and processed by the browser, the XHR call is done, but the Profiler storage didn't happened yet so no profiling data is available and theProfilerControllerjust sends an empty response with 200 as status code.Here we change to instead send a specific status code, and treat it in javascript by retrying several times before failing.
The question are: