Removes output buffer awareness from Server, emitters

By removing a call to `ob_start()` within `Server::listen()`, we can solve the issue of detecting when we have both a response and content in the output buffer; in most cases, we will have already sent headers, which will cause an exception to be raised; we can also check the _current_ output buffer and, if non-empty, raise an exception. This means we can: - Remove the `SapiEmitterTrait::flush()` implementation, and all calls to it. - Remove the `$maxBufferLevel` argument to each emitter. - Remove tests regarding interactions of emitters with the output buffer. This is a backwards-incompatible change. However, it fixes a rather tricky problem that occurs currently when mixing buffered output and response instances.
zendframework · Sep 11, 2017 · ba5b16e · ba5b16e
1 parent 5f0c491
commit ba5b16e
Show file tree

Hide file tree

Showing 8 changed files with 30 additions and 117 deletions.
diff --git a/src/Response/SapiEmitter.php b/src/Response/SapiEmitter.php
@@ -23,19 +23,15 @@ class SapiEmitter implements EmitterInterface
      * body content via the output buffer.
      *
      * @param ResponseInterface $response
-     * @param null|int $maxBufferLevel Maximum output buffering level to unwrap.
      */
-    public function emit(ResponseInterface $response, $maxBufferLevel = null)
+    public function emit(ResponseInterface $response)
     {
-        if (headers_sent()) {
-            throw new RuntimeException('Unable to emit response; headers already sent');
-        }
+        $this->checkForPreviousOutput();
 
         $response = $this->injectContentLength($response);
 
         $this->emitStatusLine($response);
         $this->emitHeaders($response);
-        $this->flush($maxBufferLevel);
         $this->emitBody($response);
     }
 

diff --git a/src/Response/SapiEmitterTrait.php b/src/Response/SapiEmitterTrait.php
@@ -10,9 +10,30 @@
 namespace Zend\Diactoros\Response;
 
 use Psr\Http\Message\ResponseInterface;
+use RuntimeException;
 
 trait SapiEmitterTrait
 {
+    /**
+     * Checks to see if content has previously been sent.
+     *
+     * If either headers have been sent, or the current output buffer contains
+     * content, raises an exception.
+     *
+     * @throws RuntimeException if headers have already been sent.
+     * @throws RuntimeException if the current output buffer is not empty.
+     */
+    private function checkForPreviousOutput()
+    {
+        if (headers_sent()) {
+            throw new RuntimeException('Unable to emit response; headers already sent');
+        }
+        $bufferContents = ob_get_contents();
+        if (! empty($bufferContents)) {
+            throw new RuntimeException('Output has been emitted previously; cannot emit response: ' . $bufferContents);
+        }
+    }
+
     /**
      * Inject the Content-Length header if is not already present.
      *
@@ -77,23 +98,6 @@ private function emitHeaders(ResponseInterface $response)
         }
     }
 
-    /**
-     * Loops through the output buffer, flushing each, before emitting
-     * the response.
-     *
-     * @param int|null $maxBufferLevel Flush up to this buffer level.
-     */
-    private function flush($maxBufferLevel = null)
-    {
-        if (null === $maxBufferLevel) {
-            $maxBufferLevel = ob_get_level();
-        }
-
-        while (ob_get_level() > $maxBufferLevel) {
-            ob_end_flush();
-        }
-    }
-
     /**
      * Filter a header name to wordcase
      *

diff --git a/src/Response/SapiStreamEmitter.php b/src/Response/SapiStreamEmitter.php
@@ -24,20 +24,16 @@ class SapiStreamEmitter implements EmitterInterface
      * body content via the output buffer.
      *
      * @param ResponseInterface $response
-     * @param null|int $maxBufferLevel Maximum output buffering level to unwrap.
      * @param int $maxBufferLength Maximum output buffering size for each iteration
      */
-    public function emit(ResponseInterface $response, $maxBufferLevel = null, $maxBufferLength = 8192)
+    public function emit(ResponseInterface $response, $maxBufferLength = 8192)
     {
-        if (headers_sent()) {
-            throw new RuntimeException('Unable to emit response; headers already sent');
-        }
+        $this->checkForPreviousOutput();
 
         $response = $this->injectContentLength($response);
 
         $this->emitStatusLine($response);
         $this->emitHeaders($response);
-        $this->flush($maxBufferLevel);
 
         $range = $this->parseContentRange($response->getHeaderLine('Content-Range'));
 

diff --git a/src/Server.php b/src/Server.php
@@ -150,24 +150,18 @@ public static function createServerFromRequest(
      * If provided a $finalHandler, that callable will be used for
      * incomplete requests.
      *
-     * Output buffering is enabled prior to invoking the attached
-     * callback; any output buffered will be sent prior to any
-     * response body content.
-     *
      * @param null|callable $finalHandler
      */
     public function listen(callable $finalHandler = null)
     {
         $callback = $this->callback;
 
-        ob_start();
-        $bufferLevel = ob_get_level();
-
         $response = $callback($this->request, $this->response, $finalHandler);
         if (! $response instanceof ResponseInterface) {
             $response = $this->response;
         }
-        $this->getEmitter()->emit($response, $bufferLevel);
+
+        $this->getEmitter()->emit($response);
     }
 
     /**

diff --git a/test/Response/SapiEmitterTest.php b/test/Response/SapiEmitterTest.php
@@ -13,26 +13,4 @@
 
 class SapiEmitterTest extends AbstractEmitterTest
 {
-    public function testEmitsBufferLevel()
-    {
-        ob_start();
-        echo "level" . ob_get_level() . " "; // 2
-        ob_start();
-        echo "level" . ob_get_level() . " "; // 3
-        ob_start();
-        echo "level" . ob_get_level() . " "; // 4
-        $response = (new Response())
-            ->withStatus(200)
-            ->withAddedHeader('Content-Type', 'text/plain');
-        $response->getBody()->write('Content!');
-        ob_start();
-        $this->emitter->emit($response);
-        $this->assertEquals('Content!', ob_get_contents());
-        ob_end_clean();
-        $this->assertEquals('level4 ', ob_get_contents(), 'current buffer level string must remains after emit');
-        ob_end_clean();
-        $this->emitter->emit($response, 2);
-        $this->assertEquals('level2 level3 Content!', ob_get_contents(), 'must buffer until specified level');
-        ob_end_clean();
-    }
 }
diff --git a/test/Response/SapiStreamEmitterTest.php b/test/Response/SapiStreamEmitterTest.php
@@ -210,7 +210,7 @@ function ($bufferLength) use (& $peakBufferLength) {
             ->withBody($stream->reveal());
 
         ob_start();
-        $this->emitter->emit($response, null, $maxBufferLength);
+        $this->emitter->emit($response, $maxBufferLength);
         $emittedContents = ob_get_clean();
 
         if ($seekable) {
@@ -351,7 +351,7 @@ function ($bufferLength) use (& $peakBufferLength) {
             ->withBody($stream->reveal());
 
         ob_start();
-        $this->emitter->emit($response, null, $maxBufferLength);
+        $this->emitter->emit($response, $maxBufferLength);
         $emittedContents = ob_get_clean();
 
         $stream->rewind()->shouldNotBeCalled();
@@ -497,7 +497,7 @@ function () use (& $closureTrackMemoryUsage) {
 
         gc_disable();
 
-        $this->emitter->emit($response, null, $maxBufferLength);
+        $this->emitter->emit($response, $maxBufferLength);
 
         ob_end_flush();
 

diff --git a/test/ServerIntegrationTest.php b/test/ServerIntegrationTest.php
diff --git a/test/ServerTest.php b/test/ServerTest.php
@@ -109,7 +109,6 @@ public function testEmitterSetter()
 
         $this->expectOutputString('');
         $server->listen();
-        ob_end_flush();
     }
 
     public function testCreateServerWillCreateDefaultInstancesForRequestAndResponse()
@@ -153,7 +152,6 @@ public function testListenInvokesCallbackAndSendsResponse()
 
         $this->expectOutputString('FOOBAR');
         $server->listen();
-        ob_end_flush();
 
         $this->assertContains('HTTP/1.1 200 OK', HeaderStack::stack());
         $this->assertContains('Content-Type: text/plain', HeaderStack::stack());
@@ -179,7 +177,6 @@ public function testListenEmitsStatusHeaderWithoutReasonPhraseIfNoReasonPhrase()
 
         $this->expectOutputString('FOOBAR');
         $server->listen();
-        ob_end_flush();
 
         $this->assertContains('HTTP/1.1 299', HeaderStack::stack());
         $this->assertContains('Content-Type: text/plain', HeaderStack::stack());
@@ -204,7 +201,6 @@ public function testEnsurePercentCharactersDoNotResultInOutputError()
 
         $this->expectOutputString('100%');
         $server->listen();
-        ob_end_flush();
 
         $this->assertContains('HTTP/1.1 200 OK', HeaderStack::stack());
         $this->assertContains('Content-Type: text/plain', HeaderStack::stack());
@@ -233,7 +229,6 @@ public function testEmitsHeadersWithMultipleValuesMultipleTimes()
         $server = Server::createServer($callback, $server, [], [], [], []);
 
         $server->listen();
-        ob_end_flush();
 
         $this->assertContains('HTTP/1.1 200 OK', HeaderStack::stack());
         $this->assertContains('Content-Type: text/plain', HeaderStack::stack());
@@ -311,7 +306,6 @@ public function testListenPassesCallableArgumentToCallback()
             $this->response
         );
         $server->listen($final);
-        ob_end_flush();
         $this->assertTrue($invoked);
     }
 }