Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[WebProfilerBundle] add debugbar onStreamedResponse#58789
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/Symfony/Bundle/WebProfilerBundle/EventListener/WebDebugToolbarListener.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/WebProfilerBundle/EventListener/WebDebugToolbarListener.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Bundle/WebProfilerBundle/EventListener/WebDebugToolbarListener.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| class MockedStreamedResponse extends StreamedResponse | ||
| { | ||
| public function getContent(): string|false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would not override thegetContent method of StreamedResponse for that, as it means this MockedStreamedResponse does not actually behave like a StreamedResponse (in which youcannot callgetContent to get the actual content), which would potentially hide bugs in tests.
Instead, I would suggest writing a separate test covering the injection of the toolbar in a streamed response, which would perform this output buffering in the test (using$response->sendContent()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right and indeed I missed a bug on StreamedResponsehere.
Let me know if it's ok for you.
9a631e3 to642ebedCompareac59dd8 to5f7393bCompare5f7393b to6f0f764Compare7787e2a to677f95cCompareStreamedResponse| @@ -5,6 +5,7 @@ CHANGELOG | |||
| --- | |||
| * Add `ajax_replace` option for replacing toolbar on AJAX requests | |||
| * Show debug bar when using a streamed response | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| *Show debug bar when using a streamed response | |
| *Add support for streamed responses in the debug toolbar |
| return $buffer; | ||
| }, 8); // length of '</body>' | ||
| ($callback)(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| ($callback)(); | |
| $callback(); |
| $response = new StreamedResponse($this->createCallbackFromContent($content)); | ||
| $m->invoke($listener, $response, Request::create('/'), ['csp_script_nonce' => 'scripto', 'csp_style_nonce' => 'stylo']); | ||
| $this->assertEquals($expected, $this->getContentFromStreamedResponse($response)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Prefer usingassertSame() wherever possible in the whole test class
| $this->assertEquals($expected, $this->getContentFromStreamedResponse($response)); | ||
| } | ||
| public static function getInjectToolbarTests() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| publicstaticfunctiongetInjectToolbarTests() | |
| publicstaticfunctionprovideInjectedToolbarHtml() |
Uh oh!
There was an error while loading.Please reload this page.