[Bridge][Twig] Optionally pass dumper into DumpExtension #18466
Conversation
| @@ -22,7 +23,7 @@ class DumpExtensionTest extends \PHPUnit_Framework_TestCase | |||
| */ | |||
| public function testDumpTag($template, $debug, $expectedOutput, $expectedDumped) | |||
| { | |||
| $extension = new DumpExtension(new VarCloner()); | |||
| $extension = new DumpExtension(new VarCloner(), new HtmlDumper()); | |||
There was a problem hiding this comment.
This change should be reverted to check that the expected behaviour does not change when still not injecting a dumper. You could add another test which gets another dumper injected and test for the expected output.
|
@xabbuh Created a separate test. Tests are passing other than an unrelated HHVM/Travis failure. |
| @@ -67,11 +70,14 @@ public function dump(\Twig_Environment $env, $context) | |||
| } | |||
|
|
|||
| $dump = fopen('php://memory', 'r+b'); | |||
| $dumper = new HtmlDumper($dump); | |||
| $prevOutput = $this->dumper->setOutput($dump); | |||
There was a problem hiding this comment.
The setOutput() method is not part of the DataDumperInterface.
There was a problem hiding this comment.
Ouch...that's an oversight.
There was a problem hiding this comment.
What do you recommend? Should I require AbstractDumper in constructor? I don't think it would be ok to assume the dumper has null output.
There was a problem hiding this comment.
Instead of setOutput, you could use dump's 2nd arg.
An you could also create a new interface to enforce what you need here.
There was a problem hiding this comment.
@nicolas-grekas Using the 2nd arg doesn't work as it resets the header sent flag which causes tests to fail (due to recursion I'm assuming).
Even now isn't ideal with the header being dumped once per twig call. Thoughts?
Maybe we set the header to empty string after the first dump call? .... Hmm or maybe not. I guess we can't guarantee that that twig block will get into the actual html page.
|
Any progress on this one? @nicolas-grekas What would be your take on fixing the |
|
👍, ok for |
…on (CarsonF) This PR was merged into the 3.2-dev branch. Discussion ---------- [Bridge][Twig] Optionally pass dumper into DumpExtension | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Allow the dumper to be passed into `DumpExtension` constructor. This allows a different dumper to be used or even just `HtmlDumper` with a non-default configuration, such as different styles. Note: The dumper's output is ignored. Commits ------- d8c0f1d [Bridge][Twig] Optionally pass dumper into DumpExtension
Allow the dumper to be passed into
DumpExtensionconstructor. This allows a different dumper to be used or even justHtmlDumperwith a non-default configuration, such as different styles.Note: The dumper's output is ignored.