Skip to content

Fix #67 Fixed Cookie creation in cookie plugin. #68

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

Closed
wants to merge 5 commits into from
Closed

Fix #67 Fixed Cookie creation in cookie plugin. #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac127fb
Prepare stable release
sagikazarmark Jan 27, 2016
facbfca
Add missing transfer encoding header for chunked
joelwurtz Jan 29, 2016
cfcaf96
Fixed prefer-lowest build
Nyholm Feb 29, 2016
8609533
Do not pull unneeded dependencies
gjuric Feb 28, 2016
8f6c70c
Fix #67 Fixed Cookie creation in cookie plugin.
Mar 17, 2016
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
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# Change Log

## 1.0.1 - 2016-01-29

### Changed

- Set the correct header for the Content-Length when in chunked mode.

## 1.0.0 - 2016-01-28

### Added

- New Header plugins (see the documentation)
Expand All @@ -13,6 +21,7 @@

- Decoder plugin no longer sends accept header for encodings that require gzip if gzip is not available


## 0.1.0 - 2016-01-13

### Added
Expand Down
5 changes: 2 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ $ composer require php-http/plugins

## Documentation

Please see the [official documentation](http://docs.php-http.org).
Please see the [official documentation](http://docs.php-http.org/en/latest/plugins/index.html).


## Testing
Expand All @@ -38,8 +38,7 @@ Please see our [contributing guide](http://docs.php-http.org/en/latest/developme

## Security

If you discover any security related issues, please contact us at [security@httplug.io](mailto:security@httplug.io)
or [security@php-http.org](mailto:security@php-http.org).
If you discover any security related issues, please contact us at [security@php-http.org](mailto:security@php-http.org).


## License
Expand Down
18 changes: 10 additions & 8 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,21 @@
"require": {
"php": ">=5.4",
"php-http/httplug": "^1.0",
"php-http/message-factory": "^1.0",
"psr/log": "^1.0",
"psr/cache": "^1.0",
"php-http/message-factory": "^1.0.2",
"php-http/client-common": "^1.0",
"php-http/message": "^1.0",
"symfony/options-resolver": "^2.6|^3.0"
},
"require-dev": {
"symfony/stopwatch": "^2.3",
"phpspec/phpspec": "^2.4",
"henrikbjorn/phpspec-code-coverage" : "^1.0"
"henrikbjorn/phpspec-code-coverage" : "^1.0",
"psr/log": "^1.0",
"psr/cache": "^1.0"
},
"conflict": {
"psr/log": ">=2.0.0",
"psr/cache": ">=2.0.0"
},
"autoload": {
"psr-4": {
Expand All @@ -46,9 +50,7 @@
},
"extra": {
"branch-alias": {
"dev-master": "0.2-dev"
"dev-master": "1.0-dev"
}
},
"prefer-stable": true,
"minimum-stability": "dev"
}
}
1 change: 1 addition & 0 deletions spec/ContentLengthPluginSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ function it_streams_chunked_if_no_size(RequestInterface $request, StreamInterfac

$stream->getSize()->shouldBeCalled()->willReturn(null);
$request->withBody(Argument::type('Http\Message\Encoding\ChunkStream'))->shouldBeCalled()->willReturn($request);
$request->withAddedHeader('Transfer-Encoding', 'chunked')->shouldBeCalled()->willReturn($request);

$this->handleRequest($request, function () {}, function () {});
}
Expand Down
28 changes: 24 additions & 4 deletions spec/CookiePluginSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@

namespace spec\Http\Client\Plugin;

use Http\Promise\FulfilledPromise;
use Http\Message\Cookie;
use Http\Message\CookieJar;
use Http\Promise\FulfilledPromise;
use Http\Promise\Promise;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\UriInterface;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;

class CookiePluginSpec extends ObjectBehavior
{
Expand Down Expand Up @@ -146,7 +146,7 @@ function it_saves_cookie(RequestInterface $request, ResponseInterface $response,

$response->hasHeader('Set-Cookie')->willReturn(true);
$response->getHeader('Set-Cookie')->willReturn([
'cookie=value',
'cookie=value; expires=Tuesday, 31-Mar-99 07:42:12 GMT; Max-Age=60; path=/; domain=test.com; secure; HttpOnly'
]);

$request->getUri()->willReturn($uri);
Expand All @@ -157,4 +157,24 @@ function it_saves_cookie(RequestInterface $request, ResponseInterface $response,
$promise->shouldHaveType('Http\Promise\Promise');
$promise->wait()->shouldReturnAnInstanceOf('Psr\Http\Message\ResponseInterface');
}

function it_throws_exception_on_invalid_expires_date(RequestInterface $request, ResponseInterface $response, UriInterface $uri)
{
$next = function () use ($response) {
return new FulfilledPromise($response->getWrappedObject());
};

$response->hasHeader('Set-Cookie')->willReturn(true);
$response->getHeader('Set-Cookie')->willReturn([
'cookie=value; expires=i-am-an-invalid-date;'
]);

$request->getUri()->willReturn($uri);
$uri->getHost()->willReturn('test.com');
$uri->getPath()->willReturn('/');

$promise = $this->handleRequest($request, $next, function () {});
$promise->shouldReturnAnInstanceOf('Http\Promise\RejectedPromise');
$promise->shouldThrow('Http\Client\Exception\TransferException')->duringWait();
}
}
1 change: 1 addition & 0 deletions src/ContentLengthPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
if (null === $stream->getSize()) {
$stream = new ChunkStream($stream);
$request = $request->withBody($stream);
$request = $request->withAddedHeader('Transfer-Encoding', 'chunked');
} else {
$request = $request->withHeader('Content-Length', $stream->getSize());
}
Expand Down
23 changes: 19 additions & 4 deletions src/CookiePlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Http\Client\Plugin;

use Http\Client\Exception\TransferException;
use Http\Message\Cookie;
use Http\Message\CookieJar;
use Psr\Http\Message\RequestInterface;
Expand Down Expand Up @@ -86,6 +87,8 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
* @param $setCookie
*
* @return Cookie|null
Copy link
Contributor

Choose a reason for hiding this comment

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

the styleci complaint is here: we require a blank line between the @return and an @throws

*
* @throws \Http\Client\Exception\TransferException
*/
private function createCookie(RequestInterface $request, $setCookie)
{
Expand All @@ -97,7 +100,8 @@ private function createCookie(RequestInterface $request, $setCookie)

list($name, $cookieValue) = $this->createValueKey(array_shift($parts));

$expires = 0;
$maxAge = null;
$expires = null;
$domain = $request->getUri()->getHost();
$path = $request->getUri()->getPath();
$secure = false;
Expand All @@ -109,11 +113,22 @@ private function createCookie(RequestInterface $request, $setCookie)

switch (strtolower($key)) {
case 'expires':
$expires = \DateTime::createFromFormat(DATE_COOKIE, $value);
$expires = \DateTime::createFromFormat(\DateTime::COOKIE, $value);
Copy link
Contributor

Choose a reason for hiding this comment

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

looking at the test failure on hhvm, i wonder if hhvm can't handle dates that far in the future?

which in turn leads me to think we should check the return value as \DateTime::createFromFormat is one of those stupid functions that returns false instead of throwing an exception on failure. if we check for false === $expires we can throw an exception and put the $value in the message to say it was not converted to a date.

Copy link
Author

Choose a reason for hiding this comment

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

And what exception type would you like this to be? It probably needs to implement the name namespace exception interface. Can it be a TransferException? Or since there is a response a HttpException?

Copy link
Member

Choose a reason for hiding this comment

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

I would say it is not an HTTP related exception, so we should just fail with one of the SPL exceptions. UnexpectedValue maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

if i am not mistaken, HttpException is to express a HTTP error code. so it would be TransferException and we say that the server sent unexpected data.

looking at DateTime::COOKIE having "l, d-M-Y H:i:s T" and Y meaning 4 digit year, i have no idea what hhvm is doing here. but if it works for all versions with a two digit year lets go with that - its just mock data after all.

and lets add a test that triggers the exception, with a date value of "this is not a date" or something.

Copy link
Contributor

@dbu dbu Mar 31, 2016 via email

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok, fine from my side.


if (true !== ($expires instanceof \DateTime)) {
throw new TransferException(
sprintf(
'Cookie header `%s` expires value `%s` could not be converted to date',
$name,
$value
)
);
}

break;

case 'max-age':
$expires = (new \DateTime())->add(new \DateInterval('PT'.(int) $value.'S'));
$maxAge = (int) $value;
break;

case 'domain':
Expand All @@ -134,7 +149,7 @@ private function createCookie(RequestInterface $request, $setCookie)
}
}

return new Cookie($name, $cookieValue, $expires, $domain, $path, $secure, $httpOnly);
return new Cookie($name, $cookieValue, $maxAge, $domain, $path, $secure, $httpOnly, $expires);
}

/**
Expand Down