Please express your acceptance/rejection with a +/-1

Are we happy with the ClientPool? Are we confident to tag 1.3.0?

I suggest tagging version 1.2. I need the debug_plugins option in the next release of httplugBundle

During #114 we started working toward a 2.0, with a dedicated branch. With that merged, 2.0 is compatible with HTTPlug 2 and PSR-18. What else do we want to add? I'll try to suggest some stuff and track it here. Maybe we can use a milestone?

• Update the docs/changelog
• Use PHP 7 language features, since it's now the minimum
  • Scalar type in method parameters and return types
• Remove deprecations
• Make all classes that are not intended to be extended final, verify that everything that is protected really needs to be protected

Actual Behavior

ContentLengthPlugin has a conflict when Diactoros is used. MessageInterface::withHeader expects a string or a string collection for the value parameter and Diactoros checks if value respects this. ContentLengthPlugin inject an integer so an exception is thrown by Diactoros.

Expected Behavior

inject a string for the value paramter into MessageInterface::withHeader method

Steps to Reproduce

use the ContentLengthPlugin with Diactoros

Possible Solutions

cast StreamInterface::getSize return value in string here :

$request = $request->withHeader('Content-Length', (string) $stream->getSize());

Would be useful if the retry plugin would backoff before retrying.

Actual Behavior

Once your PluginClient has been initialized, you can't add new plugins to it

Expected Behavior

Being able to add some new plugins on the fly. I'm currently registering it in a Behat container, so I registered a few plugins on the construct https://github.com/Taluu/Behapi/blob/master/src/Container.php#L85.

But I would like to be able to give the possibility for the users to add some new plugins (such as the auth plugin or a random listener) so that they can do stuff on their own.

Possible Solutions

Add a addPlugin or something to the PluginClient

As per @dbu's concerns in one of the PRs.

His proposition is HttpSyncAsyncClient which sounds a little bit weird to me.

Any input?

Before releasing 1.6 we should review #71 once again. Is it a better way to achieve the same thing? Should we really use a callable?

Is there a way to send concurrent requests and wait for their results in a same way as Guzzle supports?
Ex.: Load entities by ids in a function from on API that can only load entities one by one.

(#7 Might be related to this question.)

Missing release date

I am working on a 3rd party integration for my library that uses HTTplug and this library. When I integrated my library to Drupal 8 that has its own Guzzle client wrapper I faced with an interesting problem. By using the \Http\Client\Common\Plugin\DecoderPlugin sometimes I got back an empty string instead of the JSON that API has actually returned.

I think this is the combination of two possible issues:

1. If something reads the content of the full response body before the \Http\Client\Common\Plugin\DecoderPlugin plugin can do that then when it starts processing the response (ex.: with the \Http\Message\Encoding\DechunkStream) it does not rewinds the stream, therefore when the \Http\Message\Encoding\FilteredStream::getContents() is being called the while loop does not do anything because the wrapped (underlying) stream has already in the end.
   Maybe this can be solved by adding this condition to the constructor of the FilteredStream class.

if ($this->stream->isSeekable()) {
  $this->stream->rewind();
}

2. If the wrapped stream was being closed then any of the \Http\Message\Encoding\FilteredStream implementations could not read the content of the stream again. It means that if I use the \Http\Client\Common\Plugin\DecoderPlugin on my client an try to get the body of the response multiple times like this: (string) $response->getBody() then after the first call I always get back an empty string. (Or if we add the suggested condition to the FilteredStream class' constructor an exception, because the read() method tries to read on a closed stream after the first (string) $response->getBody()). If I remove the plugin then the originally wrapped \GuzzleHttp\Psr7\Stream object inside an instance of \GuzzleHttp\Psr7\Response can always return the same response body (string) multiple times.

Actual Behavior

An Http\Client\Common\Exception\CircularRedirectionException is thrown when executing the following code:

<?php

require 'vendor/autoload.php';

use Http\Adapter\Guzzle6\Client as HttpClient;
use Http\Client\Common\PluginClient;
use Http\Client\Common\Plugin\RedirectPlugin;
use Http\Message\MessageFactory\GuzzleMessageFactory as MessageFactory;

$message = new MessageFactory;

$pluginClient = new PluginClient(new HttpClient, [
    new RedirectPlugin,
]);

$request = $message->createRequest('GET', 'http://www.artstation.com/artwork.rss');
$response = $pluginClient->sendRequest($request);

var_dump($response->getBody()->getContents());

Expected Behavior

No exception should be thrown, since http://www.artstation.com/artwork.rss redirects to https://www.artstation.com/artwork.rss (http -> https)

Steps to Reproduce

Execute the code above.

Possible Solutions

The issue is on line 162, where the circular redirection detection is done.

This line of code:

if (in_array($redirectRequest->getRequestTarget(), $this->circularDetection[$chainIdentifier]))

is checking if the current request target already exists in the circular detection map.

Because two lines before (160), the same value is being added, it will always be true.

Hi,

Do you think a plugin which will automatically set the content-type header based on what it can find in the body request would be a good idea ? I can do a pull request on it if you approved this idea.

Thanks in advance.

Actual Behavior

Plugin implementations only accept Http\Client\Exception instances in their rejection handler (here and here) while there is no such limitation in the promise spec or the RejectedPromise.

Expected Behavior

Plugins should accept any kind of exceptions and handle only which are acceptable for them and rethrow the rest. This is not a breaking change since we just need to widen the accepted exception types in the plugins.

Steps to Reproduce

See #33

Possible Solutions

As said, widen the range of accepted exception types.

I talk to an API where I identify myself with query parameters. (yes, that is wrong but I cannot change this.) I see a value in having a plugin much like DefaultHeaderPlugin but for URI queries.

Actual Behavior

Actually the HttpClientRouter use a RequestException which is not easily identifiable

Expected Behavior

Have an exception that pinpoint this specific use case (and we could profit from the exception added by ClientPool)

#56

Actual Behavior

Actually the error plugin always throws an exception on status >= 400

Expected Behavior

It would be better to add options for only throwing status >= 500 or 500 > status >= 400

As an example when i want to retry requests on a web server, i may want to retry only on server errors and not on client errors.

Also 400 errors are often not errors and more a normal comportement that i need to take into account, as i would like to know if some resource exists on the server i don't want to deal that with an exception having a 404 may be a normal thing for me.

However 500 errors are never normals so i want to treat them specialy compared to the 400 ones.

WDYT ?

I don't know the whole history of debug_plugins option (which was introduced in #22), but since this option is no longer used by HTTPlugBundle (php-http/HttplugBundle#128), it could be deprecated and then removed in the next major version.

This will keep the PluginClient more simple.

Actual Behavior

What is the actual behavior?

running

composer require php-http/client-common

receive the following error

 Problem 1
    - The requested package php-http/client-common could not be found in any version, there may be a typo in the package name.

Expected Behavior

What is the behavior you expect?

It installs!

Steps to Reproduce

What are the steps to reproduce this bug? Please add code examples,
screenshots or links to GitHub repositories that reproduce the problem.

Possible Solutions

If you have already ideas how to solve the issue, add them here.
(remove this section if not needed)

This is a feature request for adding a soap client (extending the one from PHP) which use a HttpClient implementation to send the soap request.

Having this allow to profit of all the debugging offer by the HttplugBundle for Soap requests.

Here is an example of what we have done in a project :

/**
 * Wrapper around PHP SoapClient to use a HttpClient instead of php, allow better control over the request.
 */
class SoapClient extends \SoapClient
{
    private $httpClient;

    public function __construct(HttpClient $httpClient, $wsdl, array $options = [])
    {
        $this->httpClient = $httpClient;

        parent::__construct($wsdl, $options);
    }

    /**
     * {@inheritdoc}
     */
    public function __doRequest($body, $location, $action, $version, $one_way = 0)
    {
        $headers = [
            'Content-Type' => sprintf('application/soap+xml; charset=utf-8; action="%s"', $action),
        ];

        if ($version === SOAP_1_1) {
            $headers = [
                'Content-Type' => 'text/xml; charset=utf-8',
                'SOAPAction'   => $action,
            ];
        }

        try {
            $response = $this->httpClient->sendRequest(new Request('POST', $location, $headers, $body));
        } catch (HttpException $exception) {
            return $exception->getResponse()->getBody()->getContents();
        }

        return $response->getBody()->getContents();
    }
}

Having a client that accept multi http clients, when it receives a request it will duplicate the call to all the underlying client and return a BatchResponse (like the batchclient)

It should done the call in async mode if possible or in sync mode if no async is available.

One of the use case can be for FosHttpCache when we need to send a request over multiple varnish instance.

in the classes EmulatedHttpAsyncClient and EmulatedHttpClient we have a comment saying "This should be replaced by an anonymous class in PHP 7.". we now bump to php 7, so we could do this. however, i am not sure its worth it. right now, we have spec tests for those emulated clients. if we make them anonymous classes, we can't test them separately anymore, and the FlexibleHttpClient will become very complicated to test, i fear.

The principle would be to add a strategy on how a batch client should fail or not when sending multiple requests.

The default strategy would be the actual one, if one request fail a Exception is thrown.

Other strategy that can be done

• Majority (Quorum ?): if <=50% of requests failed then it will throw an exception
• All: Throw an exception only if all requests have failed

Having an HttpClient / HttpAsyncClient, that accept a list of differents clients and balance requests over them, should also remove a client from the list if one of the request fail.

You can see something like this here : https://github.com/elastic/elasticsearch-php/tree/master/src/Elasticsearch/ConnectionPool

Actual Behavior

When using Plugin Client:

$this->httpClient = new PluginClient(
            $httpClient ?: HttpClientDiscovery::find(),
            [$authenticationPlugin, $redirectPlugin]
        );

this prevents me from being able to attach response objects when using the Mock Client.

$this->httpClient->addResponse($response);

Expected Behavior

I would expect to have access to the underlying http client so I can attach responses when using the Mock Client for testing.

Steps to Reproduce

See Above.

Possible Solutions

Add a method to PluginClient

public function getClient()
{
    return $this->client;
}

ping @Nyholm

https://packagist.org/packages/php-http/client-common#v1.3.1

I know this is not the best place to ask for support but I couldn't find a better one.

I've create a library using php-http you can see a draft here https://github.com/hpatoio/lotadata/
I use PluginClient and as suggested in the doc I've created a factory that create the http client that I then pass to the library.

Everything works and I can use it like this:

<?php

include_once('vendor/autoload.php');

use Hpatoio\Lotadata\HttpClientFactory;

$lotadataApiClient = new Hpatoio\Lotadata\LotadataClient(
    HttpClientFactory::create('here_my_api_key')
);

$categories = $lotadataApiClient->getTaxonomy("EventCategory");

foreach ($categories as $category) {
        echo "CATEGORY ".$category['name']."\n";
}

In the doc doc you say

this allows [..] to inject their own plugins or configure a different client.

but I don't understand how you can inject a plugin.

For instance in the code above :

• how do you inject the Cache Plugin ?
• how do you change the setting for a plugin ? Let's say you want to change log level ?

Thanks.

Actual Behavior

When setting the add_path it cannot be either / or empty.

Expected Behavior

Depending on which server I install the project I require to have a dynamic path prefix. Sometimes it will be empty sometimes not. I would need either to allow the empty or to allow / (as empty)

Possible Solutions

Remove this test https://github.com/php-http/client-common/blob/master/src/Plugin/AddPathPlugin.php#L33 and add a test if not empty here: https://github.com/php-http/client-common/blob/master/src/Plugin/AddPathPlugin.php#L49

In MessageFormatters we had the issue of reading from streams which might not be readable. Though that was meant for Response objects, what's the situation in case of Requests. Some plugins modify or just read the request body (like content length plugin).

Should this be our concern?

Actual Behavior

Testing our code is pretty hard because of all the mocks necessary to test HTTP Message related code and Plugin Chain.

Expected Behavior

It should be pretty easy to understand tests, modify behaviour, provide new test cases, etc.

Possible Solutions

• Rewrite specifications to simple unit tests using the MockClient.
• Rewrite specifications to use actual Message objects (so only remove mocking, but preserve specifications)

WDYT?

HttpAsyncClient actually returns an Http***Promise, which enforces its result type to be an HTTP Response. That's all nice and dandy, but it actually prevents to transform the result in anything else.

I hited this wall while implemening a REST Client, with a middleware chain (quite like the plugin chain), when the final Middleware failed to transform the HTTP Response into my internal REST Response.

(I know I could also code/find a proper Promise implementation and plug it after the Http****Promise, but doesn't it would defeat the purpose of Promises ? Or am I understanding it wrong ?)

A client accepting a map of regex -> httpclient. Given a specific request, it will check the uri of it and redirect to a specific http client implementation.

This allow to have a single pipeline for multiple library, but still allows using different client for some requests.

Actual Behavior

RetryPlugin used in an async request waits for the response, making the request effectively synchronous.

Expected Behavior

The request stays asynchronous and the RetryPlugin returns an appropriate promise.

Possible Solutions

An attempt was made in #101 but had to be reverted in #111 because of #109.
There is a WIP in #110 but somebody needs to pick that up and revert #111, then test everything in #110 until it works correctly.

Actual Behavior

The request matcher checks if the given request matches the requirements, and executes the delegated plugin. In cases where we want to use an if-else logic to decide between two plugins this does not work well, and currently requires two RequestMatcher plugins.

Possible Solutions

Provide a second plugin to the RequestMatcherPlugin which gets called when the request is not a match.

It would also probably make sense to rename the RequestMatcher plugin to Conditional (this would be aligned with the Conditional authenticator in the message repo)

we currently get styleci failures on master because it wants yoda for everything. we can either change to code to that, or we can add configuration to the styleci file to skip the yoda check.

i vote to change the code. for variables its the right thing imo, and for method calls it does not matter and we can't disable that separately.

Actual Behavior

Actually all plugin dealing with body check if the stream is seekable and also made arrengments when body is too long via an arbitrary value

Expected Behavior

It would be nice to base our default behavior by reading the Content-Type header of the request / response and use the https://github.com/php-http/message/blob/master/src/Stream/BufferedStream.php when a body is not seekable

As an example the logger plugin would have a list of content type authorized for logging (json / text / html / ....) and if stream not seekable, it would be decorated with the BufferedStream class

WDYT ? Is something like this considered as a BC Break ?

Actual Behavior

Cookie coming from the same domain but different subdomain is ignored.

Expected Behavior

The cookie for subdomain should be caught and saved to cookie jar (unless current behavior is what you have initially planned).
Or, at least we should provide a way for developer to choose if he wants to restrict cookies to single domain.

Steps to Reproduce

Imagine that initial request is made to www.example.com which tries to set cookie for all subdomains:
.example.com

Lines 71-74 will ignore that cookie, since strpos('.example.com', 'www.example.com') will return false:

// Restrict setting cookie from another domain
if (false === strpos($cookie->getDomain(), $request->getUri()->getHost())) {
    continue;
}

Possible Solutions

A quick and dirty solution would be to simply switch the arguments inside strpos(), like this:

// Restrict setting cookie from another domain
if (false === strpos($request->getUri()->getHost(), $cookie->getDomain())) {
    continue;
}

I can make a pull request with that change if you find it appropriate.

However, as I have said earlier, it would be better to let the developer decide whether to accept different domain cookies or not.

Behavior

The method "spl_object_hash" can issue the same hash for two different objects if one of the objects has been destroyed (i.e. garbage collector run).

This hash collision can cause a request to pass without actually adding the path.

See the notes for "spl_object_hash" in the PHP documentation: http://php.net/manual/en/function.spl-object-hash.php

Steps to Reproduce

Send multiple requests with the same HTTP client.

Possible Solutions

1. Do not cache requests (as done in 1.70)
2. Do not use 'spl_object_hash' for hashing objects

Actual Behavior

When using the CookiePlugin with a CookieJar filled with multiple applicable Cookies, each will add a new Cookie: key=value header to the Request. Or maybe concatenate them, separated by a command , .

Expected Behavior

Only one Cookie header should be added to the request. Where key-value pair should be separated by ; .

RFC 6265 Section 5.4:

When the user agent generates an HTTP request, the user agent MUST
NOT attach more than one Cookie header field.

Steps to Reproduce

$cookieJar = new \Http\Message\CookieJar();
$cookieJar->addCookies([
    new \Http\Message\Cookie('foo', 'bar'),
    new \Http\Message\Cookie('cookie', 'oreo'),
]);

$plugin = new \Http\Client\Common\Plugin\CookiePlugin($cookieJar);

$request = new \GuzzleHttp\Psr7\Request('GET', 'http://127.0.0.1/');
$response = new \GuzzleHttp\Psr7\Response();

$noop = function(\GuzzleHttp\Psr7\Request $request) use($response) {
    var_dump($request->getHeaderLine('Cookie'));
    return new Http\Promise\FulfilledPromise($response);
};

$plugin->handleRequest($request, $noop, $noop);

// "foo=bar, cookie=oreo"

It's a little hard to demonstrate with only the client-common package, but when using the HttplugBundle one can easily see the problem when firing a request at httpbin/RequestBin

(I was using the Guzzle6 client btw)

Possible Solutions

I think the problem lies in the fact the Request::withAddedHeader('Cookie', ...) gets called multiple times, as can seen here.

The \GuzzleHttp\Psr7\Request object doesn't handle the Cookie header in any special way, so it won't join those lines the proper way.

I believe the CookieJar should join the cookie key-value pairs and only set the Cookie header once.

Some feedback would be welcome, as I'm not 100% sure where this issue should be resolved. (In this package, in guzzle/psr7 or the client(s))

If the route of fixing this package would be taken, you might also want to consider implementing step 2 mentioned in RFC 6265 Section 5.4:

2. The user agent SHOULD sort the cookie-list in the following
   order:

• Cookies with longer paths are listed before cookies with
  shorter paths.

• Among cookies that have equal-length path fields, cookies with
  earlier creation-times are listed before cookies with later
  creation-times.

We should probably not retry all exceptions by default.

See discussion #79 (comment)

So people not using our adapter / interface can still use this library

Clients in this package is not final at the moment. I think they are not intended to be extended, so they should be made final.

Make the rest of clients final. See #18 and #19

• BatchClient
• BatchResult
• Deferred
• LeastUsedClientPool
• RoundRobinClientPool
• HttpMethodsClient
• PluginClient
• PluginClientFactory
• All Plugins

Same as BatchClient but for async http client (or should we do that in the BatchClient and prefer the sendAsync if available ?)

• Update email address
• Update documentation link
• Remove unnecessary interface implementation
• Remove composer stability config
• Update composer dev alias
• Update changelog

Actual Behavior

Error: Can only throw objects
/home/travis/build/php-http/HttplugBundle/vendor/php-http/client-common/src/Deferred.php:129
/home/travis/build/php-http/HttplugBundle/vendor/php-http/client-common/src/PluginClient.php:87
/home/travis/build/php-http/HttplugBundle/Tests/Functional/ProfilingTest.php:96

Behavior

When using the DecoderPlugin, it is possible that $newEncodings in https://github.com/php-http/client-common/blob/master/src/Plugin/DecoderPlugin.php#L110 is an empty array - in the example below, this is the case when a Response has a TransferEncoding header and an empty body ('', not null).

When I use the plugin together with https://github.com/Nyholm/psr7 (/cc @Nyholm :)), that empty array causes an exception in https://github.com/Nyholm/psr7/blob/master/src/MessageTrait.php#L80.

I don't know if this is something that can/should be changed and if so, if it should be changed in the plugin or in nyholm/psr7 - on the one hand, guzzle/psr7 and zend/diactoros don't throw exceptions on empty headers, on the other hand, PSR-7 states that an exception should be thrown on invalid headers (https://github.com/php-fig/http-message/blob/master/src/MessageInterface.php#L130) - which again begs the question if an empty set of headers is to be considered invalid (oh, this is exhausting already 😅).

Steps to Reproduce

$ mkdir showcase && cd $_
$ composer require php-http/mock-client nyholm/psr7 

<?php
// showcase.php
declare(strict_types=1);

require __DIR__.'/vendor/autoload.php';

use Http\Client\Common\Plugin\DecoderPlugin;
use Http\Client\Common\PluginClient;
use Http\Mock\Client as MockClient;
use Nyholm\Psr7\Factory\MessageFactory;

$messages = new MessageFactory();
$client = new MockClient($messages);
$pluginClient = new PluginClient($client, [
    new DecoderPlugin()
]);

$request = $messages->createRequest('GET', 'http://example.com');

$response = $messages->createResponse(200, 'OK', [
    'Transfer-Encoding' => ['chunked'],
], '');

$client->setDefaultResponse($response);

$pluginClient->sendRequest($request);

$ php showcase.php
PHP Fatal error:  Uncaught InvalidArgumentException: 
Header values must be non-empty strings in 
vendor/nyholm/psr7/src/MessageTrait.php:80

Possible Solutions

I see these possible solutions:

• In DecoderPlugin::decodeOnEncodingHeader(), if $newEncodings is empty, don't replace the header.
• In the same function, remove the header if $newEncodings is empty - I haven't looked it up, but I think when a header is empty, it is effectively removed with the guzzle/diactoros implementations.
• In nyholm/psr7, be less strict about empty headers. (The issue occurs with 0.3.0 and dev-master)

What do you think?