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.

Sign up for GitHub

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

Use Psr7 consistently #239

Closed
wants to merge 1 commit into from
Closed

Use Psr7 consistently #239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/RequestHeaderParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

use Evenement\EventEmitter;
use Exception;
use RingCentral\Psr7 as g7;
use RingCentral\Psr7;

/**
* @event headers
Expand Down Expand Up @@ -92,7 +92,7 @@ private function parseRequest($headers)
}

// parse request headers into obj implementing RequestInterface
$request = g7\parse_request($headers);
$request = Psr7\parse_request($headers);

// create new obj implementing ServerRequestInterface by preserving all
// previous properties and restoring original request-target
Expand Down
8 changes: 4 additions & 4 deletions src/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use React\Promise\Promise;
use RingCentral\Psr7 as Psr7Implementation;
use RingCentral\Psr7;
use Psr\Http\Message\ServerRequestInterface;
use React\Promise\CancellablePromiseInterface;
use React\Stream\WritableStreamInterface;
Expand Down Expand Up @@ -353,7 +353,7 @@ public function handleResponse(ConnectionInterface $connection, ServerRequestInt
// response to HEAD and 1xx, 204 and 304 responses MUST NOT include a body
// exclude status 101 (Switching Protocols) here for Upgrade request handling below
if ($request->getMethod() === 'HEAD' || $code === 100 || ($code > 101 && $code < 200) || $code === 204 || $code === 304) {
$response = $response->withBody(Psr7Implementation\stream_for(''));
$response = $response->withBody(Psr7\stream_for(''));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, the Psr7 name implies this is the PSR-7 interface, whereas this is actually a concrete package that happens to implement this interface and in this particular case only references a function that resides in the same namespace. I'm all for consistency and I'm not opposed to improving things here, but what do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, the Psr7 name implies this is the PSR-7 interface,

Fair point. On the other hand side we don't care about what the actual implementation here is as long as there's any one. From that perspective and by concvention these two are interchangable for me.

What does imho not work is g7 vs. Psr7 vs. Psr7Implementation. Personally I prefer Psr7 as synonym which is also simple to use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line of code does very much care about which implementation is being used here, as this function is not part of PSR-7. This function happens to be implemented by both guzzle/psr-7 and also ringcentral/psr-7, but there's no guarantee it's available for other implementations, because it's not actually part of PSR-7.

I agree that the existing code base is inconsistent here, but I think naming this just "Psr7" is actually misleading. What do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better than before and personally I prefer short names. Psr7Implementation is too long and Psr7 is imho a wide-spread, if not perfect, replacement. The fact that there may be multiple implementations shouldn't matter as a) we're only using one and b) it's entirely clear from the use statement which one that is. I really appreciate your eye on the details but would also feel it doesn't need to get academically correct?

}

// 101 (Switching Protocols) response uses Connection: upgrade header
Expand Down Expand Up @@ -388,7 +388,7 @@ public function handleResponse(ConnectionInterface $connection, ServerRequestInt
private function handleResponseBody(ResponseInterface $response, ConnectionInterface $connection)
{
if (!$response->getBody() instanceof HttpBodyStream) {
$connection->write(Psr7Implementation\str($response));
$connection->write(Psr7\str($response));
return $connection->end();
}

Expand All @@ -399,7 +399,7 @@ private function handleResponseBody(ResponseInterface $response, ConnectionInter
return $stream->close();
}

$connection->write(Psr7Implementation\str($response));
$connection->write(Psr7\str($response));

if ($stream->isReadable()) {
if ($response->getHeaderLine('Transfer-Encoding') === 'chunked') {
Expand Down