Skip to content

Add cookies to request object - #175

Merged
WyriHaximus merged 3 commits into
reactphp:masterfrom
legionth:cookies
May 10, 2017
Merged

Add cookies to request object#175
WyriHaximus merged 3 commits into
reactphp:masterfrom
legionth:cookies

Conversation

@legionth

Copy link
Copy Markdown
Contributor

This PR adds cookies to the PSR-7 ServerRequest. The implementation of cookies is line with the implementation of PHP.

I added a simple example to this to test this via a web browser.

@WyriHaximus
WyriHaximus requested review from clue and jsorApril 23, 2017 09:01

@clueclue left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

The RFC suggests that there must be a space between multiple cookie-pairs, such as Cookie: SID=31d4d96e407aad42; lang=en-US. Can you verify https://tools.ietf.org/html/rfc6265#section-4.2.1?

Comment threadsrc/RequestHeaderParser.php Outdated
@legionth

Copy link
Copy Markdown
ContributorAuthor

@clue you're right about the space between multiple cookies. Fixed it.

@clueclue added this to the v0.7.0 milestone Apr 26, 2017

@clueclue left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changes LGTM, thanks! 👍

The RFC suggests there MUST be space after the semicolon, are clients that omit this space relevant? May I ask you to check how other parsers handle this situation? 👍

@legionth
legionthforce-pushed the cookies branch 2 times, most recently from d736267 to 3974e62CompareApril 27, 2017 15:40
Comment threadsrc/ServerRequest.php Outdated
*/
public static function parseCookie($cookie)
{
// PSR-7 `getHeadline('Cookies')` will return multiple

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should be getHeaderLine

Comment threadsrc/ServerRequest.php Outdated
public static function parseCookie($cookie)
{
// PSR-7 `getHeadline('Cookies')` will return multiple
// cookie header coma-seperated. Multiple cookie headers

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comma

Comment threadtests/ServerTest.php Outdated
$this->assertEquals(array(), $requestValidation->getCookieParams());
}

public function testCookieWithSepeartorWillBeAddedToServerRequest()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MEthod name typo
M

@WyriHaximus

Copy link
Copy Markdown
Member

@legionth could you resolve merge conflicts and address @andig points?

@legionth
legionthforce-pushed the cookies branch 3 times, most recently from 04f89d2 to 0a4e5e1CompareMay 9, 2017 09:50
@legionth

Copy link
Copy Markdown
ContributorAuthor

Sorry I'm very busy currently.

@andig Have a look. This should cover your remarks :)

@clue I checked Slim and Symfony. Both trim the spaces from the cookie string. I changed the behavior in this PR. Have a look!

Comment threadsrc/ServerRequest.php
{
// PSR-7 `getHeaderLine('Cookies')` will return multiple
// cookie header comma-seperated. Multiple cookie headers
// are not allowed according to https://tools.ietf.org/html/rfc6265#section-5.4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should imho read

... PSR-7 getHeaderLine('Cookie') will return multiple comma-seperated cookie headers.

@WyriHaximus
WyriHaximus merged commit bb4b14a into reactphp:masterMay 10, 2017
@WyriHaximus

Copy link
Copy Markdown
Member

FYI Travis failure was unrelated to this PR and only on the HHVM job.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@legionth@WyriHaximus@jsor@andig@clue@kelunik