Merge pull request #51 from clue-labs/error-event

Improve error reporting for invalid responses by using `ResponseException`

Author: clue

README.md

@@ -193,14 +193,17 @@ The `error` event will be emitted when the EventSource connection fails.
 The event receives a single `Exception` argument for the error instance.
 
 ```php
-$redis->on('error', function (Exception $e) {
+$es->on('error', function (Exception $e) {
     echo 'Error: ' . $e->getMessage() . PHP_EOL;
 });
 ```
 
 The EventSource connection will be retried automatically when it is temporarily
 disconnected. If the server sends a non-successful HTTP status code or an
 invalid `Content-Type` response header, the connection will fail permanently.
+In this case, the `error` event will receive a
+[`ResponseException`](https://github.com/reactphp/http#responseexception)
+that provides access to the response via the `getResponse()` method.
 
 ```php
 $es->on('error', function (Exception $e) use ($es) {

src/EventSource.php

@@ -7,6 +7,7 @@
 use React\EventLoop\Loop;
 use React\EventLoop\LoopInterface;
 use React\Http\Browser;
+use React\Http\Message\ResponseException;
 use React\Stream\ReadableStreamInterface;
 
 /**
@@ -193,15 +194,22 @@ private function request()
         $this->request->then(function (ResponseInterface $response) {
             if ($response->getStatusCode() !== 200) {
                 $this->readyState = self::CLOSED;
-                $this->emit('error', array(new \UnexpectedValueException('Unexpected status code')));
+                $this->emit('error', [new ResponseException(
+                    $response,
+                    'Expected "200 OK" response status, ' . $this->quote($response->getStatusCode() . ' ' . $response->getReasonPhrase()) . ' response status returned'
+                )]);
                 $this->close();
                 return;
             }
 
             // match `Content-Type: text/event-stream` (case insensitive and ignore additional parameters)
-            if (!preg_match('/^text\/event-stream(?:$|;)/i', $response->getHeaderLine('Content-Type'))) {
+            $contentType = $response->getHeaderLine('Content-Type');
+            if (!preg_match('/^text\/event-stream(?:$|;)/i', $contentType)) {
                 $this->readyState = self::CLOSED;
-                $this->emit('error', array(new \UnexpectedValueException('Unexpected Content-Type')));
+                $this->emit('error', [new ResponseException(
+                    $response,
+                    'Expected "Content-Type: text/event-stream" response header, ' . (!$response->hasHeader('Content-Type') ? 'no "Content-Type"' : $this->quote('Content-Type: ' . $contentType)) . ' response header returned'
+                )]);
                 $this->close();
                 return;
             }
@@ -290,4 +298,14 @@ public function close()
 
         $this->removeAllListeners();
     }
+
+    /**
+     * @param string $string
+     * @return string
+     * @throws void
+     */
+    private function quote($string)
+    {
+        return '"' . \addcslashes(\substr($string, 0, 100), "\x00..\x1f\"\\\x7f..\xff") . '"';
+    }
 }

tests/EventSourceTest.php

@@ -228,7 +228,28 @@ public function testCloseAfterGetRequestFromConstructorFailsWillCancelPendingRet
         $es->close();
     }
 
-    public function testConstructorWillReportFatalErrorWhenGetResponseResolvesWithInvalidStatusCode()
+    public function provideInvalidStatusCode()
+    {
+        return [
+            [
+                new Response(400, ['Content-Type' => 'text/event-stream'], ''),
+                'Expected "200 OK" response status, "400 Bad Request" response status returned'
+            ],
+            [
+                new Response(500, ['Content-Type' => 'text/event-stream'], '', '1.1', "Intern\xE4l Server Err\xF6r"),
+                'Expected "200 OK" response status, "500 Intern\344l Server Err\366r" response status returned'
+            ],
+            [
+                new Response(400, ['Content-Type' => 'text/event-stream'], '', '1.1', str_repeat('a', 200)),
+                'Expected "200 OK" response status, "400 ' . str_repeat('a', 96) . '" response status returned'
+            ]
+        ];
+    }
+
+    /**
+     * @dataProvider provideInvalidStatusCode
+     */
+    public function testConstructorWillReportFatalErrorWhenGetResponseResolvesWithInvalidStatusCode($response, $expectedMessage)
     {
         $deferred = new Deferred();
         $browser = $this->getMockBuilder('React\Http\Browser')->disableOriginalConstructor()->getMock();
@@ -244,14 +265,45 @@ public function testConstructorWillReportFatalErrorWhenGetResponseResolvesWithIn
             $caught = $e;
         });
 
-        $response = new Response(400, array('Content-Type' => 'text/event-stream'), '');
         $deferred->resolve($response);
 
         $this->assertEquals(EventSource::CLOSED, $readyState);
-        $this->assertInstanceOf('UnexpectedValueException', $caught);
-    }
-
-    public function testConstructorWillReportFatalErrorWhenGetResponseResolvesWithInvalidContentType()
+        $this->assertInstanceOf('React\Http\Message\ResponseException', $caught);
+        $this->assertEquals($expectedMessage, $caught->getMessage());
+        $this->assertEquals($response->getStatusCode(), $caught->getCode());
+        $this->assertSame($response, $caught->getResponse());
+    }
+
+    public function provideInvalidContentType()
+    {
+        return [
+            [
+                new Response(200, [], ''),
+                'Expected "Content-Type: text/event-stream" response header, no "Content-Type" response header returned'
+            ],
+            [
+                new Response(200, ['Content-Type' => ''], ''),
+                'Expected "Content-Type: text/event-stream" response header, "Content-Type: " response header returned'
+            ],
+            [
+                new Response(200, ['Content-Type' => 'text/html'], ''),
+                'Expected "Content-Type: text/event-stream" response header, "Content-Type: text/html" response header returned'
+            ],
+            [
+                new Response(200, ['Content-Type' => "application/json; invalid=a\xE4b"], ''),
+                'Expected "Content-Type: text/event-stream" response header, "Content-Type: application/json; invalid=a\344b" response header returned'
+            ],
+            [
+                new Response(200, ['Content-Type' => str_repeat('a', 200)], ''),
+                'Expected "Content-Type: text/event-stream" response header, "Content-Type: ' . str_repeat('a', 86) . '" response header returned'
+            ]
+        ];
+    }
+
+    /**
+     * @dataProvider provideInvalidContentType
+     */
+    public function testConstructorWillReportFatalErrorWhenGetResponseResolvesWithInvalidContentType($response, $expectedMessage)
     {
         $deferred = new Deferred();
         $browser = $this->getMockBuilder('React\Http\Browser')->disableOriginalConstructor()->getMock();
@@ -267,11 +319,12 @@ public function testConstructorWillReportFatalErrorWhenGetResponseResolvesWithIn
             $caught = $e;
         });
 
-        $response = new Response(200, array(), '');
         $deferred->resolve($response);
 
         $this->assertEquals(EventSource::CLOSED, $readyState);
-        $this->assertInstanceOf('UnexpectedValueException', $caught);
+        $this->assertInstanceOf('React\Http\Message\ResponseException', $caught);
+        $this->assertEquals($expectedMessage, $caught->getMessage());
+        $this->assertSame($response, $caught->getResponse());
     }
 
     public function testConstructorWillReportOpenWhenGetResponseResolvesWithValidResponse()