-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
Add server-side parameters to request object #174
Conversation
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.
Nice!
src/Server.php
Outdated
} | ||
|
||
$request = $request->withServerParams($serverParams); | ||
|
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.
Does it make sense to move this whole block to the request parser instead? 👍
README.md
Outdated
Unix timestamp when the complete request header has been received, | ||
as float similar to `microtime(true)` | ||
* `https` | ||
Set to 'on' if the request used HTTPS, otherwise it will be set to `null` |
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.
AFAICT $_SERVER['https']
will be unset for plain HTTP, does it really make sense to assume a null
value here?
examples/02-client-ip.php
Outdated
|
||
$server = new \React\Http\Server($socket, function (ServerRequestInterface $request) { | ||
$body = "The method of the request is: " . $request->getMethod(); | ||
$body .= "The requested path is: " . $request->getUri()->getPath(); |
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.
?! Does not match example from the README.
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.
Oopsie. Wrong example.
Doesn't it make sense to make the keys match the keys from |
@jsor I have no preference here as the keys are rather arbitrary anyway, how do others handle this? |
Most PSR7 implementations pass https://github.com/zendframework/zend-diactoros/blob/master/src/ServerRequestFactory.php#L59 So, i think it makes sense to follow common behaviour. |
0cf4b27
to
dc008c0
Compare
src/Server.php
Outdated
@@ -174,7 +175,7 @@ public function handleConnection(ConnectionInterface $conn) | |||
} | |||
|
|||
/** @internal */ | |||
public function handleRequest(ConnectionInterface $conn, ServerRequestInterface $request) | |||
public function handleRequest(ConnectionInterface $conn, ServerRequest $request) |
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.
Still needed?
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.
Nope you are right.
src/RequestHeaderParser.php
Outdated
'REQUEST_TIME_FLOAT' => microtime(true) | ||
); | ||
|
||
if (!empty($this->remoteAddress)) { |
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 a big fan of empty()
here ("0"
could technically be a valid UDS address). Does it make sense to use a null
check here and below?
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.
Replaced empty()
in newest commit.
src/RequestHeaderParser.php
Outdated
{ | ||
$this->uri = $localSocketUri; | ||
$this->remoteAddress = $remoteAddress; |
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.
Naming convention is a bit unclear WRT local address / URI?
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.
Renamed vars in newest commit.
examples/02-client-ip.php
Outdated
$socket = new Server(isset($argv[1]) ? $argv[1] : '0.0.0.0:0', $loop); | ||
|
||
$server = new \React\Http\Server($socket, function (ServerRequestInterface $request) { | ||
$body = "Your IP is: " . $request->getServerParams()['remote_address']; |
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.
👊
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.
😨
3eab518
to
677076c
Compare
Ping @clue Added some tests to the |
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.
Nice, changes LGTM! 👍
This PR adds server-side parameters similar to parameters of the $_SERVER from PHP.
This should cover the most important parameters.