[2.2] [output-stream] Refactor StreamingResponse implementation to use an OutputStream #4146

igorw · 2012-04-28T00:40:39Z

This makes it possible to write through other channels than stdout. Using stdout will still work for "normal" webservers however.

…utputStream

stof · 2012-04-29T15:06:08Z

src/Symfony/Component/HttpFoundation/OutputStream/StreamOutputStream.php

+    static public function create($filename)
+    {
+        $stream = fopen($filename, 'w');
+        return new static($stream);


missing empty line

ghost · 2012-04-30T04:21:01Z

src/Symfony/Component/HttpFoundation/Tests/OutputStream/OutputStreamTest.php

+        try {
+            fread($fp, 1);
+            $this->fail();
+        } catch (\PHPUnit_Framework_Error_Warning $e) {


Why not @expectedException PHPUnit_Framework_Error_Warning?

This is more explicit as it verifies exactly where the notice occurs.

@eriksencosta thus, I'm not sure we can use expectedException for PHPUnit internal exceptions

@stof Yes, we can :)

@igorw

Commits ------- 5b92b9e [Console] Selectively output to STDOUT or OUTPUT stream Discussion ---------- [Console] Selectively output to STDOUT or OUTPUT stream Originally opened in this PR targeting master, but asked to target 2.0 instead: #4148 Bug fix: yes Feature addition: no Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #1434 Todo: - As noted in the ticket discussion and linked discussion threads, IBM i5 Series has issues with writing output to STDOUT when viewed via their QP2TERM console. The output is likely not being converted to the correct character-encoding on the system level. This PR changes the default output stream from `php://stdout` to `php://output` for OS400 environments, which does not exhibit this issue. I'm using `php_uname('s')` to check for the presence of "OS400", which is at least one of the IBM OS's exhibiting this issue. This check is only done once when executing a Console task and shouldn't see any adverse affects in speed on the 99% other platforms using Symfony. I'm not a native to the OS400 platform so I can't really anticipate any other possible regressions that might occur from switching output streams for that platform. On my Mac, this change would strip all color output, but the PR code only changes output for OS400 environment. To my knowledge the QP2TERM console doesn't even support color, so no loss there. I think this change is best to make the Console component at least usable out of the box for anyone else trying to build CLI applications for use on OS400. --------------------------------------------------------------------------- by igorw at 2012-04-29T19:41:21Z #4146 might also need a fix for this. --------------------------------------------------------------------------- by johnkary at 2012-04-29T20:21:39Z @igorw Hmm. In this case for #4152 when creating a CLI application, `Symfony\Component\Console\Output\ConsoleOutput` is the [default implementation](https://github.com/johnkary/symfony/blob/5b92b9ed432ea1d5f2c596517fc7f8aa825b1977/src/Symfony/Component/Console/Application.php#L113) used by `Symfony\Component\Console\Application` when not specifying your own `OutputInterface`. Our hard-coded defaults were causing problems out of the box. I haven't looked too closely at the PRs surrounding the additions of `StreamingResponse` and your recent `OutputStream` but are we assuming anywhere that `php://stdout` is the default stream used when creating a streaming response? If so it MAY require a check similar to what I implemented for Console. My addition was only necessary because the output was being sent to a CLI console. If output is sent to a browser, I don't believe this would be an issue. If you have something that needs testing on OS400 just ping me.

igorw · 2012-05-05T15:39:52Z

I don't see how that's related. This PR does not deal with console output.

stof · 2012-05-05T16:51:35Z

@juliendidier Your refactoring (which has nothing to do with this PR and should be discussed elsewhere) is not BC

fabpot · 2012-05-15T06:09:53Z

src/Symfony/Component/HttpFoundation/StreamedResponse.php

@@ -102,7 +115,11 @@ public function sendContent()
            throw new \LogicException('The Response callback must not be null.');
        }

-        call_user_func($this->callback);
+        if (null === $this->outputStream) {
+            throw new \LogicException('The Response output stream must not be null.');


What about using stdout by default here instead of throwing an exception?

@igorw what do you think about this ?

fabpot · 2012-07-03T08:52:39Z

@igorw Any news on this PR?

* upstream/master: (633 commits) [Security] Fix the default authentication handlers config definition updated VERSION for 2.0.16 update CONTRIBUTORS for 2.0.16 updated CHANGELOG for 2.0.16 [Validator] Improved error messages displayed when the Valid constraint is misused [Form] Fixed TransformationFailedExceptions to be caught in the model transformers [Form] Improved ValidatorTypeGuesser to interpret the constraints True and False [Validator] Deprecated the Size constraint [Validator] Fix docblocks [Process] remade ProcessBuilder::setTimeout fluent [Process] fixed setting the timeout to null [Validator] Reverted the changes done to the Size constraint in 3a5e84f [Validator] Added the constraints MinCount and MaxCount [Validator] Removed the Range constraint as it duplicates functionality given in Min and Max [Form] Enabled error bubbling from the parts of a date/time field to the main field [Locale] Fixed error resetting in StubIntlDateFormatter::parse() [Form] Renamed the options "data_timezone" and "user_timezone" [Process] re-added the possibility to set the Process timeout to null (to disable it) (closes symfony#4843) added missing listener priority changes in the UPGRADE file (refs symfony#2680) [Form] Fixed tests failing on systems with timezones other than +01:00 ... Conflicts: src/Symfony/Component/HttpFoundation/StreamedResponse.php

stof · 2012-10-13T16:49:36Z

@igorw can you rebase your PR ?
@fabpot is there anything left for this feature ?

* master: (554 commits) [WebProfilerBundle] fixed typo [WebProfileBundle] remove dependency on the DIC [WebProfilerBundle] removed dependency on the templating component [HttpKernel] unified the way the traceable event dispatcher injects information into the profiler (closes symfony#5733) [WebProfilerBundle] avoid the usage of the global app variable [WebProfilerBundle] changed all includes to use the new Twig's notation [WebProfilerBundle] moved all static assets directly into the templates [Console] [ProgressHelper] better percentage precision [Form] Removed unused method ChoiceView::isSelected() Fix incorrect inheritdoc blocks [DomCrawler] fixed a bad merge [Console] Fixes in ProgressHelper [Security] Added Pbkdf2PasswordEncoder FIX [2.1][ClassLoader]UniversalClassLoader not working with AnnotationRegistry::registerLoader fixed typo [Config] added some phpdocs added entry to changelog fixed CS adds two convenience methods for optional configuration sections added doc comments ... Conflicts: src/Symfony/Component/HttpFoundation/Tests/StreamedResponseTest.php

igorw · 2012-10-13T17:16:48Z

Done. Tests currently failing due to failing master.

stof · 2012-10-13T18:18:02Z

src/Symfony/Component/HttpFoundation/OutputStream/StreamOutputStream.php

+    /**
+     * Static factory method
+     */
+    static public function create($filename)


should be public static

igorw · 2013-04-18T23:16:27Z

I'm closing this PR as I'm not convinced this is the best approach.

[output-stream] Refactor StreamingResponse implementation to use an O…

a2f32a9

…utputStream

stof reviewed Apr 29, 2012
View reviewed changes

igorw mentioned this pull request Apr 29, 2012

[Console] Selectively output to STDOUT or OUTPUT stream #4152

Merged

ghost reviewed Apr 30, 2012
View reviewed changes

fabpot reviewed May 15, 2012
View reviewed changes

igorw added 5 commits July 12, 2012 01:06

[output-stream] Cosmetic tweak

56a9192

[output-stream] Close OutputStream after serving master request

cc8fca3

[output-stream] Expose the stream resource of StreamOutputStream

27e8732

[output-stream] Do not close stream for twig subrequests

8c6fcec

fabpot mentioned this pull request Jul 13, 2012

[2.1] Transport agnostic HTTP streaming #3177

Closed

stof reviewed Oct 13, 2012
View reviewed changes

igorw closed this Apr 18, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.2] [output-stream] Refactor StreamingResponse implementation to use an OutputStream #4146

[2.2] [output-stream] Refactor StreamingResponse implementation to use an OutputStream #4146

igorw commented Apr 28, 2012

stof Apr 29, 2012

ghost Apr 30, 2012

igorw Apr 30, 2012

stof Oct 13, 2012

ghost Oct 15, 2012

igorw commented May 5, 2012

stof commented May 5, 2012

fabpot May 15, 2012

stof Oct 13, 2012

fabpot commented Jul 3, 2012

stof commented Oct 13, 2012

igorw commented Oct 13, 2012

stof Oct 13, 2012

igorw commented Apr 18, 2013

[2.2] [output-stream] Refactor StreamingResponse implementation to use an OutputStream #4146

[2.2] [output-stream] Refactor StreamingResponse implementation to use an OutputStream #4146

Conversation

igorw commented Apr 28, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorw commented May 5, 2012

stof commented May 5, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jul 3, 2012

stof commented Oct 13, 2012

igorw commented Oct 13, 2012

Choose a reason for hiding this comment

igorw commented Apr 18, 2013