Skip to content

use options resolver where applicable, fix decoder plugin gzip handling #45

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

Merged
merged 1 commit into from
Jan 25, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
# Change Log

### Changed

- Using array options for DecoderPlugin, RedirectPlugin and RetryPlugin

### Fixed

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

## 0.1.0 - 2016-01-13

Expand Down
2 changes: 1 addition & 1 deletion spec/DecoderPluginSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ function it_decodes_inflate(RequestInterface $request, ResponseInterface $respon

function it_does_not_decode_with_content_encoding(RequestInterface $request, ResponseInterface $response)
{
$this->beConstructedWith(false);
$this->beConstructedWith(['use_content_encoding' => false]);

$request->withHeader('TE', ['gzip', 'deflate', 'compress', 'chunked'])->shouldBeCalled()->willReturn($request);
$request->withHeader('Accept-Encoding', ['gzip', 'deflate', 'compress'])->shouldNotBeCalled();
Expand Down
8 changes: 4 additions & 4 deletions spec/RedirectPluginSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ function it_throw_multi_redirect_exception_on_300(RequestInterface $request, Res
}
};

$this->beConstructedWith(true, false);
$this->beConstructedWith(['preserve_header' => true, 'use_default_for_multiple' => false]);
$responseRedirect->getStatusCode()->willReturn('300');

$promise = $this->handleRequest($request, $next, function () {});
Expand Down Expand Up @@ -301,7 +301,7 @@ function it_clears_headers(
ResponseInterface $finalResponse,
Promise $promise
) {
$this->beConstructedWith(['Accept']);
$this->beConstructedWith(['preserve_header' => ['Accept']]);

$request->getRequestTarget()->willReturn('/original');

Expand Down Expand Up @@ -377,9 +377,9 @@ function it_throws_circular_redirection_exception(UriInterface $uri, UriInterfac

class RedirectPluginStub extends RedirectPlugin
{
public function __construct(UriInterface $uri, $storedUrl, $status, $preserveHeader = true, $useDefaultForMultiple = true)
public function __construct(UriInterface $uri, $storedUrl, $status, array $config = [])
{
parent::__construct($preserveHeader, $useDefaultForMultiple);
parent::__construct($config);

$this->redirectStorage[$storedUrl] = [
'uri' => $uri,
Expand Down
2 changes: 1 addition & 1 deletion src/CachePlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class CachePlugin implements Plugin
private $config;

/**
* Available options are
* Available options for $config are
* - respect_cache_headers: Whether to look at the cache directives or ignore them.
* - default_ttl: If we do not respect cache headers or can't calculate a good ttl, use this value.
*
Expand Down
29 changes: 22 additions & 7 deletions src/DecoderPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,15 @@
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\StreamInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

/**
* Allow to decode response body with a chunk, deflate, compress or gzip encoding.
*
* If zlib is not installed, only chunked encoding can be handled.
*
* If Content-Encoding is not disabled, the plugin will add an Accept-Encoding header for the encoding methods it supports.
*
* @author Joel Wurtz <[email protected]>
*/
class DecoderPlugin implements Plugin
Expand All @@ -25,25 +30,35 @@ class DecoderPlugin implements Plugin
private $useContentEncoding;

/**
* @param bool $useContentEncoding Whether this plugin decode stream with value in the Content-Encoding header (default to true).
* Available options for $config are:
* - use_content_encoding: Whether this plugin should look at the Content-Encoding header first or only at the Transfer-Encoding (defaults to true).
*
* If set to false only the Transfer-Encoding header will be used.
* @param array $config
*/
public function __construct($useContentEncoding = true)
public function __construct(array $config = [])
{
$this->useContentEncoding = $useContentEncoding;
$resolver = new OptionsResolver();
$resolver->setDefaults([
'use_content_encoding' => true,
]);
$resolver->setAllowedTypes('use_content_encoding', 'bool');
$options = $resolver->resolve($config);

$this->useContentEncoding = $options['use_content_encoding'];
}

/**
* {@inheritdoc}
*/
public function handleRequest(RequestInterface $request, callable $next, callable $first)
{
$request = $request->withHeader('TE', ['gzip', 'deflate', 'compress', 'chunked']);
$encodings = extension_loaded('zlib') ? ['gzip', 'deflate', 'compress'] : [];

if ($this->useContentEncoding) {
$request = $request->withHeader('Accept-Encoding', ['gzip', 'deflate', 'compress']);
if ($this->useContentEncoding && count($encodings)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is it correct that servers that respect Accept-Encoding will understand us when not sending this header? or should we send an empty header or something to say we can't handle encodings?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, from the RFC :

If the Accept-Encoding field-value is empty, then only the "identity" encoding is acceptable.

And "identity" encoding = no encoding

Copy link
Member

Choose a reason for hiding this comment

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

Speak to quick

If no Accept-Encoding field is present in a request, the server MAY assume that the client will accept any content coding. In this case, if "identity" is one of the available content-codings, then the server SHOULD use the "identity" content-coding, unless it has additional information that a different content-coding is meaningful to the client.

Copy link
Member

Choose a reason for hiding this comment

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

So we should set a default header IMO and add identity to the encoding list.

$request = $request->withHeader('Accept-Encoding', $encodings);
}
$encodings[] = 'chunked';
$request = $request->withHeader('TE', $encodings);

return $next($request)->then(function (ResponseInterface $response) {
return $this->decodeResponse($response);
Expand Down
32 changes: 26 additions & 6 deletions src/RedirectPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\UriInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

/**
* Follow redirections.
Expand Down Expand Up @@ -74,7 +75,7 @@ class RedirectPlugin implements Plugin
*
* true will keep all previous headers (default value)
* false will ditch all previous headers
* string[] will keep only headers specified in this array
* string[] will keep only headers with the specified names
*/
protected $preserveHeader;

Expand All @@ -98,13 +99,32 @@ class RedirectPlugin implements Plugin
protected $circularDetection = [];

/**
* @param bool $preserveHeader
* @param bool $useDefaultForMultiple
* Available options for $config are:
* - preserve_header: bool|string[] True keeps all headers, false remove all of them, an array is interpreted as a list of header names to keep.
* - use_default_for_multiple: Whether the location header must be directly used for a multiple redirection status code (300).
*
* @param array $config
*/
public function __construct($preserveHeader = true, $useDefaultForMultiple = true)
public function __construct(array $config = [])
{
$this->useDefaultForMultiple = $useDefaultForMultiple;
$this->preserveHeader = false === $preserveHeader ? [] : $preserveHeader;
$resolver = new OptionsResolver();
$resolver->setDefaults([
'preserve_header' => true,
'use_default_for_multiple' => true,
]);
$resolver->setAllowedTypes('preserve_header', ['bool', 'array']);
$resolver->setAllowedTypes('use_default_for_multiple', 'bool');
$resolver->setNormalizer('preserve_header', function (OptionsResolver $resolver, $value) {
if (is_bool($value) && false === $value) {
return [];
}

return $value;
});
$options = $resolver->resolve($config);

$this->preserveHeader = $options['preserve_header'];
$this->useDefaultForMultiple = $options['use_default_for_multiple'];
}

/**
Expand Down
19 changes: 15 additions & 4 deletions src/RetryPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
use Http\Client\Exception;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;

/**
* Retry the request if it has somehow failed.
* Retry the request if an exception is thrown.
*
* By default will retry only one time.
*
Expand All @@ -30,11 +31,21 @@ class RetryPlugin implements Plugin
private $retryStorage = [];

/**
* @param int $retry Number of retry before sending an exception
* Available options for $config are:
* - retries: Number of retries to attempt if an exception occurs before letting the exception bubble up.
*
* @param array $config
*/
public function __construct($retry = 1)
public function __construct(array $config = [])
{
$this->retry = $retry;
$resolver = new OptionsResolver();
$resolver->setDefaults([
'retries' => 1,
]);
$resolver->setAllowedTypes('retries', 'int');
$options = $resolver->resolve($config);

$this->retry = $options['retries'];
}

/**
Expand Down