-
-
Notifications
You must be signed in to change notification settings - Fork 165
[WIP] Expose DuplexStreamInterface for connections that requires Upgrades #382
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
Changes from all commits
f73b33f
7cc5c5a
662df47
ccca59c
6004504
3585dca
9a23b03
9a02480
88d862f
532755a
65f563c
b3264a5
7f3890f
26b3b9a
17b503a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
use React\Http\Io\Sender; | ||
use React\Http\Io\Transaction; | ||
use React\Promise\PromiseInterface; | ||
use React\Socket\ConnectionInterface; | ||
use React\Socket\ConnectorInterface; | ||
use React\Stream\ReadableStreamInterface; | ||
use InvalidArgumentException; | ||
|
@@ -757,7 +758,7 @@ public function withResponseBuffer($maximumSize) | |
* @see self::withFollowRedirects() | ||
* @see self::withRejectErrorResponse() | ||
*/ | ||
private function withOptions(array $options) | ||
public function withOptions(array $options) | ||
{ | ||
$browser = clone $this; | ||
$browser->transaction = $this->transaction->withOptions($options); | ||
|
@@ -770,7 +771,7 @@ private function withOptions(array $options) | |
* @param string $url | ||
* @param array $headers | ||
* @param string|ReadableStreamInterface $body | ||
* @return PromiseInterface<ResponseInterface,\Exception> | ||
* @return PromiseInterface<ResponseInterface,\Exception,ConnectionInterface> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this supposed to do? The |
||
*/ | ||
private function requestMayBeStreaming($method, $url, array $headers = array(), $body = '') | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
<?php | ||
|
||
namespace React\Http\Client; | ||
|
||
use Psr\Http\Message\RequestInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
use React\Stream\DuplexStreamInterface; | ||
|
||
class UpgradedResponse | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This adds a new class as part of our public API in the internal |
||
{ | ||
/** | ||
* @var DuplexStreamInterface | ||
*/ | ||
private $connection; | ||
|
||
/** | ||
* @var ResponseInterface | ||
*/ | ||
private $response; | ||
|
||
/** | ||
* @var RequestInterface | ||
*/ | ||
private $request; | ||
|
||
public function __construct(DuplexStreamInterface $connection, ResponseInterface $response, RequestInterface $request) | ||
{ | ||
$this->connection = $connection; | ||
$this->response = $response; | ||
$this->request = $request; | ||
} | ||
|
||
/** | ||
* @return DuplexStreamInterface | ||
*/ | ||
public function getConnection() | ||
{ | ||
return $this->connection; | ||
} | ||
|
||
/** | ||
* @return ResponseInterface | ||
*/ | ||
public function getResponse() | ||
{ | ||
return $this->response; | ||
} | ||
|
||
/** | ||
* @return RequestInterface | ||
*/ | ||
public function getRequest() | ||
{ | ||
return $this->request; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: missing EOL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I like exposing the
withOptions()
method again. It has been deprecated with clue/reactphp-buzz#172 not too long ago.Do we really need this method? It's my understanding we could just rely on the
Connection: upgrade
and/orUpgrade: …
request header(s) being present?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clue I didn't know the method was deprecated.
But my thought behind it was to let the consumer using
Browser
to be able to choose to watch out for the upgrade event or not. As by default,Browser
will not. This means that, if an upgrade did happen, I need to configure browser to capture it like$browser->withOptions(['upgrade' => true])
(default isfalse
inTransaction.php
).This, however, won't be necessary if the default mode should be to capture the upgrade. That was the reason I made the method public (also seeing it was public in
clue/reactphp-buzz
)