Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Internal refactoring, remove unneeded MessageFactory helper class #380

Merged
merged 1 commit into from
Jul 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/Browser.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
namespace React\Http;

use Psr\Http\Message\ResponseInterface;
use RingCentral\Psr7\Request;
use RingCentral\Psr7\Uri;
use React\EventLoop\LoopInterface;
use React\Http\Io\MessageFactory;
use React\Http\Io\ReadableBodyStream;
use React\Http\Io\Sender;
use React\Http\Io\Transaction;
use React\Promise\PromiseInterface;
Expand All @@ -19,7 +20,6 @@
class Browser
{
private $transaction;
private $messageFactory;
private $baseUrl;
private $protocolVersion = '1.1';

Expand Down Expand Up @@ -59,10 +59,8 @@ class Browser
*/
public function __construct(LoopInterface $loop, ConnectorInterface $connector = null)
{
$this->messageFactory = new MessageFactory();
$this->transaction = new Transaction(
Sender::createFromLoop($loop, $connector, $this->messageFactory),
$this->messageFactory,
Sender::createFromLoop($loop, $connector),
$loop
);
}
Expand Down Expand Up @@ -721,18 +719,22 @@ private function withOptions(array $options)
* @param string $method
* @param string $url
* @param array $headers
* @param string|ReadableStreamInterface $contents
* @param string|ReadableStreamInterface $body
* @return PromiseInterface<ResponseInterface,Exception>
*/
private function requestMayBeStreaming($method, $url, array $headers = array(), $contents = '')
private function requestMayBeStreaming($method, $url, array $headers = array(), $body = '')
{
if ($this->baseUrl !== null) {
// ensure we're actually below the base URL
$url = Uri::resolve($this->baseUrl, $url);
}

$request = $this->messageFactory->request($method, $url, $headers, $contents, $this->protocolVersion);
if ($body instanceof ReadableStreamInterface) {
$body = new ReadableBodyStream($body);
}

return $this->transaction->send($request);
return $this->transaction->send(
new Request($method, $url, $headers, $body, $this->protocolVersion)
);
}
}
15 changes: 14 additions & 1 deletion src/Client/Response.php
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,24 @@ private function getHeader($name)
return isset($normalized[$name]) ? (array)$normalized[$name] : array();
}

private function getHeaderLine($name)
/**
* @param string $name
* @return string
*/
public function getHeaderLine($name)
{
return implode(', ' , $this->getHeader($name));
}

/**
* @param string $name
* @return bool
*/
public function hasHeader($name)
{
return $this->getHeader($name) !== array();
}

/** @internal */
public function handleData($data)
{
Expand Down
78 changes: 0 additions & 78 deletions src/Io/MessageFactory.php

This file was deleted.

29 changes: 17 additions & 12 deletions src/Io/Sender.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use React\EventLoop\LoopInterface;
use React\Http\Client\Client as HttpClient;
use React\Http\Client\Response as ResponseStream;
use React\Http\Message\Response;
use React\Promise\PromiseInterface;
use React\Promise\Deferred;
use React\Socket\ConnectorInterface;
Expand Down Expand Up @@ -47,24 +48,22 @@ class Sender
* @param ConnectorInterface|null $connector
* @return self
*/
public static function createFromLoop(LoopInterface $loop, ConnectorInterface $connector = null, MessageFactory $messageFactory)
public static function createFromLoop(LoopInterface $loop, ConnectorInterface $connector = null)
{
return new self(new HttpClient($loop, $connector), $messageFactory);
return new self(new HttpClient($loop, $connector));
}

private $http;
private $messageFactory;

/**
* [internal] Instantiate Sender
*
* @param HttpClient $http
* @internal
*/
public function __construct(HttpClient $http, MessageFactory $messageFactory)
public function __construct(HttpClient $http)
{
$this->http = $http;
$this->messageFactory = $messageFactory;
}

/**
Expand Down Expand Up @@ -109,16 +108,22 @@ public function send(RequestInterface $request)
$deferred->reject($error);
});

$messageFactory = $this->messageFactory;
$requestStream->on('response', function (ResponseStream $responseStream) use ($deferred, $messageFactory, $request) {
$requestStream->on('response', function (ResponseStream $responseStream) use ($deferred, $request) {
$length = null;
$code = $responseStream->getCode();
if ($request->getMethod() === 'HEAD' || ($code >= 100 && $code < 200) || $code == 204 || $code == 304) {
$length = 0;
} elseif ($responseStream->hasHeader('Content-Length')) {
$length = (int) $responseStream->getHeaderLine('Content-Length');
}

// apply response header values from response stream
$deferred->resolve($messageFactory->response(
$responseStream->getVersion(),
$deferred->resolve(new Response(
$responseStream->getCode(),
$responseStream->getReasonPhrase(),
$responseStream->getHeaders(),
$responseStream,
$request->getMethod()
new ReadableBodyStream($responseStream, $length),
$responseStream->getVersion(),
$responseStream->getReasonPhrase()
));
});

Expand Down
14 changes: 6 additions & 8 deletions src/Io/Transaction.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\UriInterface;
use RingCentral\Psr7\Request;
use RingCentral\Psr7\Uri;
use React\EventLoop\LoopInterface;
use React\Http\Message\ResponseException;
Expand All @@ -18,7 +19,6 @@
class Transaction
{
private $sender;
private $messageFactory;
private $loop;

// context: http.timeout (ini_get('default_socket_timeout'): 60)
Expand All @@ -37,10 +37,9 @@ class Transaction

private $maximumSize = 16777216; // 16 MiB = 2^24 bytes

public function __construct(Sender $sender, MessageFactory $messageFactory, LoopInterface $loop)
public function __construct(Sender $sender, LoopInterface $loop)
{
$this->sender = $sender;
$this->messageFactory = $messageFactory;
$this->loop = $loop;
}

Expand All @@ -55,7 +54,7 @@ public function withOptions(array $options)
if (property_exists($transaction, $name)) {
// restore default value if null is given
if ($value === null) {
$default = new self($this->sender, $this->messageFactory, $this->loop);
$default = new self($this->sender, $this->loop);
$value = $default->$name;
}

Expand Down Expand Up @@ -186,11 +185,10 @@ public function bufferResponse(ResponseInterface $response, $deferred)
}

// buffer stream and resolve with buffered body
$messageFactory = $this->messageFactory;
$maximumSize = $this->maximumSize;
$promise = \React\Promise\Stream\buffer($stream, $maximumSize)->then(
function ($body) use ($response, $messageFactory) {
return $response->withBody($messageFactory->body($body));
function ($body) use ($response) {
return $response->withBody(\RingCentral\Psr7\stream_for($body));
},
function ($e) use ($stream, $maximumSize) {
// try to close stream if buffering fails (or is cancelled)
Expand Down Expand Up @@ -280,7 +278,7 @@ private function makeRedirectRequest(RequestInterface $request, UriInterface $lo
// naïve approach..
$method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET';

return $this->messageFactory->request($method, $location, $request->getHeaders());
return new Request($method, $location, $request->getHeaders());
}

private function progress($name, array $args = array())
Expand Down
62 changes: 53 additions & 9 deletions tests/FunctionalBrowserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,22 @@ public function setUpBrowserAndServer()
);
}

if ($path === '/status/300') {
if ($path === '/status/204') {
return new Response(
300,
204,
array(),
''
);
}

if ($path === '/status/304') {
return new Response(
304,
array(),
'Not modified'
);
}

if ($path === '/status/404') {
return new Response(
404,
Expand Down Expand Up @@ -308,12 +316,24 @@ public function testFollowRedirectsZeroRejectsOnRedirect()
Block\await($browser->get($this->base . 'redirect-to?url=get'), $this->loop);
}

/**
* @doesNotPerformAssertions
*/
public function testResponseStatus300WithoutLocationShouldResolveWithoutFollowingRedirect()
public function testResponseStatus204ShouldResolveWithEmptyBody()
{
Block\await($this->browser->get($this->base . 'status/300'), $this->loop);
$response = Block\await($this->browser->get($this->base . 'status/204'), $this->loop);
$this->assertFalse($response->hasHeader('Content-Length'));

$body = $response->getBody();
$this->assertEquals(0, $body->getSize());
$this->assertEquals('', (string) $body);
}

public function testResponseStatus304ShouldResolveWithEmptyBodyButContentLengthResponseHeader()
{
$response = Block\await($this->browser->get($this->base . 'status/304'), $this->loop);
$this->assertEquals('12', $response->getHeaderLine('Content-Length'));

$body = $response->getBody();
$this->assertEquals(0, $body->getSize());
$this->assertEquals('', (string) $body);
}

/**
Expand Down Expand Up @@ -595,9 +615,33 @@ public function testSendsExplicitHttp10Request()
public function testHeadRequestReceivesResponseWithEmptyBodyButWithContentLengthResponseHeader()
{
$response = Block\await($this->browser->head($this->base . 'get'), $this->loop);
$this->assertEquals('', (string)$response->getBody());
$this->assertEquals(0, $response->getBody()->getSize());
$this->assertEquals('5', $response->getHeaderLine('Content-Length'));

$body = $response->getBody();
$this->assertEquals(0, $body->getSize());
$this->assertEquals('', (string) $body);
}

public function testRequestStreamingGetReceivesResponseWithStreamingBodyAndKnownSize()
{
$response = Block\await($this->browser->requestStreaming('GET', $this->base . 'get'), $this->loop);
$this->assertEquals('5', $response->getHeaderLine('Content-Length'));

$body = $response->getBody();
$this->assertEquals(5, $body->getSize());
$this->assertEquals('', (string) $body);
$this->assertInstanceOf('React\Stream\ReadableStreamInterface', $body);
}

public function testRequestStreamingGetReceivesResponseWithStreamingBodyAndUnknownSizeFromStreamingEndpoint()
{
$response = Block\await($this->browser->requestStreaming('GET', $this->base . 'stream/1'), $this->loop);
$this->assertFalse($response->hasHeader('Content-Length'));

$body = $response->getBody();
$this->assertNull($body->getSize());
$this->assertEquals('', (string) $body);
$this->assertInstanceOf('React\Stream\ReadableStreamInterface', $body);
}

public function testRequestStreamingGetReceivesStreamingResponseBody()
Expand Down
Loading