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

Add PSR-7 ServerRequestInterface to the PSR-7 Request #160

Closed
wants to merge 4 commits into from

Conversation

legionth
Copy link
Contributor

@legionth legionth commented Apr 2, 2017

This PR adds a PSR-ServerRequestInterface to the request.

This PR adds the remote address to the server params(Fixes #153). Also cookies are parsed from the raw request and added to the request object.
Uploaded files are not handled in this PR, because this would be a much bigger task and needs an asynchronous approach(which would be too much to review).

The new class makes it possible to handle parsed request bodies (#105 for v0.8.0).


if (!isset($cookies[$key])) {
$result[$key] = $value;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some tests for multiple cookies with the same name and those using special chars?

Copy link
Member

Choose a reason for hiding this comment

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

Invalid reference?

README.md Outdated
[PSR-7 RequestInterface](https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-7-http-message.md#32-psrhttpmessagerequestinterface)
and implements the
[PSR-7 ServerRequestInterface](https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-7-http-message.md#321-psrhttpmessageserverrequestinterface)
Copy link
Member

Choose a reason for hiding this comment

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

…implements the ServerRequestInterface which in turn extends the RequestInterface?

README.md Outdated
@@ -269,6 +273,20 @@ $http = new Server($socket, function (RequestInterface $request) {
});
```

As mentioned before the request implements the `ServerRequestInterface`.
The `Server` will add server-side parameters like the remote address or cookies
this object.
Copy link
Member

Choose a reason for hiding this comment

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

…to this object.

README.md Outdated
return new Response(
200,
array('Content-Type' => 'text/plain'),
"The remote: " . $request->getServerParams()['remote_address']
Copy link
Member

Choose a reason for hiding this comment

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

Where does this name come from, are there any other params? (needs some documentation)


if (preg_match('(\,)', $cookie) >= 1) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be moved up and could use some documentation? Looks like strpos()?

@clue clue added this to the v0.7.0 milestone Apr 3, 2017
@maciejmrozinski
Copy link

Just wanted to ask if we really need our own implementation of Psr\Http\Message\ServerRequestInterface? Cannot we use Zend Diactoros for example and extend it for our needs?

@clue
Copy link
Member

clue commented Apr 3, 2017

@maciejmrozinski I think you're raising a valid point here 👍

I'm perfectly fine with switching to any other PSR-7 implementation because we only expose its interfaces and everything else is basically just an implementation detail.

In this particular case, this PR only adds a very simple implementation of the ServerRequestInterface which extends an existing Request class and marks this new class as internal and not for outside usage. As such, this adds very little complexity afaict.

Switching to another implementation now would create a bigger changeset and would likely break compatibility with legacy PHP < 5.4 (which I'm not saying is a deal-breaker), but I suppose this is best left up to another PR? 👍

@maciejmrozinski
Copy link

maciejmrozinski commented Apr 3, 2017

@legionth OK, thx for the info, I totally agree :) Just wanted to know Your plan guys and possibilities here.

EDIT: Sorry, I meant @clue ;)

Copy link

@maciejmrozinski maciejmrozinski left a comment

Choose a reason for hiding this comment

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

File Server.php should be reviewed for RequestInterface methods parameters and occurrences should be changed to ServerRequestInterface.

Lines: 8, 170, 194, 333, 357

@legionth
Copy link
Contributor Author

Ping @clue. I think added all the changes you mentioned.

@maciejmrozinski
Copy link

@legionth how about this #160 (review) ? Should it be changed or RequestInterface is correct here?

README.md Outdated
Unix timestamp of the moment, the complete request header was received
* `request_time_float`
Unix timestamp in microseconds of the moment, the complete request header wa
received
Copy link
Member

Choose a reason for hiding this comment

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

Where does this list come from? (ref please)

Copy link
Member

Choose a reason for hiding this comment

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

Example please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an reference to the text and an example.

README.md Outdated
received

The cookies of the request, will also be added to the request object by the `Server`.
Use the `getCookies` method to receive these cookies.
Copy link
Member

Choose a reason for hiding this comment

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

Example please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added example and more explanation for the cookies.

README.md Outdated
The `Server` will currently add several server-side parameters to the request like:

* `server_address`
The current IP address of the server
Copy link
Member

Choose a reason for hiding this comment

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

What about the port?


if (!isset($cookies[$key])) {
$result[$key] = $value;
}
Copy link
Member

Choose a reason for hiding this comment

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

Invalid reference?

src/Server.php Outdated
@@ -90,6 +106,7 @@
class Server extends EventEmitter
{
private $callback;
private $serverAddress;
Copy link
Member

Choose a reason for hiding this comment

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

Should probably use $conn->getLocalAddress() instead in case this is a "listen all" address?

Copy link
Contributor Author

@legionth legionth Apr 10, 2017

Choose a reason for hiding this comment

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

Done. Restructured the code a bit and added an additional server parameter

@legionth legionth force-pushed the server-request branch 4 times, most recently from cb9d103 to 1802135 Compare April 10, 2017 15:44
@legionth
Copy link
Contributor Author

@clue Have look, I added some docuementation and test for the cookies and server params.

@maciejmrozinski Thank you! Changed the type hints. Have look

@maciejmrozinski
Copy link

@legionth looks good to me

$body
);
});
```
Copy link
Member

Choose a reason for hiding this comment

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

LGTM and should also be added to examples/ 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to examples/

"Your cookie has been set."
);
});
```
Copy link
Member

Choose a reason for hiding this comment

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

LGTM, but should probably be merged with above example 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged the two examples into one and added to examples/

{
$cookieString = 'hello=world;hello=abc';
$cookies = ServerRequest::parseCookie($cookieString);
$this->assertEquals(array('hello' => 'world'), $cookies);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying this is wrong, but this is certainly unexpected. Does this need explicit documentation or is this common behavior / does this follow a common implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked the $_COOKIE behavior of PHP. Changed the behavior a bit, have a look.

@andig
Copy link
Contributor

andig commented Apr 17, 2017

I'm working with @maciejmrozinski at porting php-pm to the next react/http version based on this PR. After only getting 400 responses I've dug into the Server code and found- using socket 0.5- the following issue:

Any idea what might be wrong here?

@andig
Copy link
Contributor

andig commented Apr 17, 2017

Update

fixed for me by changing the socket parseAddress() to:

        if ($address == false || $address == '' . "\0" . '') {
            return null;
        }

Should I open an issue at socket? Or is it possible that anything in the Server->__construct()->on('connection', 'handleConnection()') is firing with a half-initialized stream if such a thing exists?

I've also opened https://bugs.php.net/bug.php?id=74458 for time being.

@andig
Copy link
Contributor

andig commented Apr 17, 2017

pinging @clue I have the suspicion- though I can't find the root cause- that reactphp/socket@a967ef1 might be causing (or not fixing) the problem I'm seeing. Apart from false php seems to return string \0, too.

@clue
Copy link
Member

clue commented Apr 17, 2017

@andig Thanks for spotting this and filing reactphp/socket#92 which I think is the correct place to handle this 👍


@legionth This PR should probably be rebased now that #167 is in and we should probably add the equivalent of $_SERVER['HTTPS'] = 'on' here 👍

@legionth
Copy link
Contributor Author

@clue I think this should cover your change requests. Added the https to the server params. Have a look.

@legionth
Copy link
Contributor Author

This PR adds a bit too much features and I can't cover them all. The fact is that this PR gets bigger and bigger, so I decided to open several smaller PRs.

@legionth legionth closed this Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants