8000 bug #26675 [HttpKernel] DumpDataCollector: do not flush when a dumper… · symfony/symfony@98ee8ab · GitHub
[go: up one dir, main page]
Skip to content

Commit 98ee8ab

Browse files
fabpotfabpot
committed
bug #26675 [HttpKernel] DumpDataCollector: do not flush when a dumper is provided (ogizanagi)
This PR was merged into the 2.7 branch. Discussion ---------- [HttpKernel] DumpDataCollector: do not flush when a dumper is provided | Q | A | ------------- | --- | Branch? | 2.7 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | https://github.com/ogizanagi/symfony/blob/3db14045d41eecf78e3557c2d64f28fe27ed3a66/src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php#L208-L209 <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | N/A This explains [the workaround I initially used](https://github.com/ogizanagi/symfony/blob/3db14045d41eecf78e3557c2d64f28fe27ed3a66/src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php#L208-L209) in the server dumper PR original code. I might be wrong on the intent, but as soon as a dumper is provided (e.g by setting `debug.dump_destination: php://stderr`), I think there is no need to set the `DumpDataCollector::$isCollected` flag to `false` as we explicitly ask for the dump to be output directly somewhere. So ne need to output again on `__destruct`. Spotted by running tests on the `symfony/demo` with the server dumper enabled: dumps were output twice. Once on the server, once at the end of the tests. But this can be easily seen as well by using `debug.dump_destination: php://stderr` on `test` env: ```diff diff --git a/src/Controller/BlogController.php b/src/Controller/BlogController.php index e3e30aa..bf9744e 100644 --- a/src/Controller/BlogController.php +++ b/src/Controller/BlogController.php @@ -50,6 +50,7 @@ class BlogController extends AbstractController */ public function index(int $page, string $_format, PostRepository $posts): Response { + dump(get_class($posts)); $latestPosts = $posts->findLatest($page); // Every template name also has two extensions that specify the format and ``` ### Before ```sh vendor/bin/simple-phpunit --filter=BlogControllerTest::testIndex PHPUnit 6.5.7 by Sebastian Bergmann and contributors. Testing Project Test Suite BlogController.php on line 53: "App\Repository\PostRepository" . 1 / 1 (100%) Time: 3.34 seconds, Memory: 44.25MB OK (1 test, 1 assertion) BlogController.php on line 53: "App\Repository\PostRepository" ``` ### After ```sh vendor/bin/simple-phpunit --filter=BlogControllerTest::testIndex PHPUnit 6.5.7 by Sebastian Bergmann and contributors. Testing Project Test Suite BlogController.php on line 53: "App\Repository\PostRepository" . 1 / 1 (100%) Time: 731 ms, Memory: 28.00MB OK (1 test, 1 assertion) ``` Commits ------- 11a0392 [HttpKernel] DumpDataCollector: do not flush when a dumper is provided
2 parents 4243db5 + 11a0392 commit 98ee8ab

File tree

2 files changed

+22
-1
lines changed

2 files changed

+22
-1
lines changed

‎src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public function dump(Data $data)
6767
if ($this->stopwatch) {
6868
$this->stopwatch->start('dump');
6969
}
70-
if ($this->isCollected) {
70+
if ($this->isCollected && !$this->dumper) {
7171
$this->isCollected = false;
7272
}
7373

‎src/Symfony/Component/HttpKernel/Tests/DataCollector/DumpDataCollectorTest.php

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Symfony\Component\HttpFoundation\Request;
1717
use Symfony\Component\HttpFoundation\Response;
1818
use Symfony\Component\VarDumper\Cloner\Data;
19+
use Symfony\Component\VarDumper\Dumper\CliDumper;
1920

2021
/**
2122
* @author Nicolas Grekas <p@tchwork.com>
@@ -130,4 +131,24 @@ public function testFlush()
130131
$this->assertSame("\"DumpDataCollectorTest.php on line {$line}:\"\n456\n", ob_get_clean());
131132
}
132133
}
134+
135+
public function testFlushNothingWhenDataDumperIsProvided()
136+
{
137+
$data = new Data(array(array(456)));
138+
$dumper = new CliDumper('php://output');
139+
$collector = new DumpDataCollector(null, null, null, null, $dumper);
140+
141+
ob_start();
142+
$collector->dump($data);
143+
$line = __LINE__ - 1;
144+
if (\PHP_VERSION_ID >= 50400) {
145+
$this->assertSame("DumpDataCollectorTest.php on line {$line}:\n456\n", ob_get_clean());
146+
} else {
147+
$this->assertSame("\"DumpDataCollectorTest.php on line {$line}:\"\n456\n", ob_get_clean());
148+
}
149+
150+
ob_start();
151+
$collector->__destruct();
152+
$this->assertEmpty(ob_get_clean());
153+
}
133154
}

0 commit comments

Comments
 (0)
0