[WebProfilerBundle] add debugbar on StreamedResponse
#58789
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
Contributor
damienfern
commented
Nov 6, 2024
•
| Q | A |
|---|---|
| Branch? | 7.3 |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | no |
| Issues | Fix #57288 |
| License | MIT |
stof
reviewed
Nov 6, 2024
src/Symfony/Bundle/WebProfilerBundle/EventListener/WebDebugToolbarListener.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/WebProfilerBundle/EventListener/WebDebugToolbarListener.php
Show resolved
Hide resolved
src/Symfony/Bundle/WebProfilerBundle/EventListener/WebDebugToolbarListener.php
Outdated
Show resolved
Hide resolved
|
|
||
| class MockedStreamedResponse extends StreamedResponse | ||
| { | ||
| public function getContent(): string|false |
There was a problem hiding this comment.
I would not override the getContent method of StreamedResponse for that, as it means this MockedStreamedResponse does not actually behave like a StreamedResponse (in which you cannot call getContent 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.
You're right and indeed I missed a bug on StreamedResponse here.
Let me know if it's ok for you.
9a631e3 to
642ebed
Compare
ac59dd8 to
5f7393b
Compare
5f7393b to
6f0f764
Compare
7787e2a to
677f95c
Compare
StreamedResponse
| @@ -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.
Suggested change
| * 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.
Suggested change
| ($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.
Prefer using assertSame() wherever possible in the whole test class
| $this->assertEquals($expected, $this->getContentFromStreamedResponse($response)); | ||
| } | ||
|
|
||
| public static function getInjectToolbarTests() |
There was a problem hiding this comment.
Suggested change
| public static function getInjectToolbarTests() | |
| public static function provideInjectedToolbarHtml() |
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.