Update guzzle, goutte and mink-goutte-driver to the latest release

Correct. You will have to make that ~5.3 in order to upgrade. You will probably have to composer update guzzlehttp/* because guzzlehttp/ringphp will need an upgrade too.

Try it and see if the tests pass.

I was wrong. It was only necessary to composer update guzzlehttp/*.

It should be 6.0.0 now.

Note this is not possible right now because fabpot/goutte requires Guzzle to be less than version 6.

https://github.com/FriendsOfPHP/Goutte/issues/215

Guzzle 6 might be a major API break, didn't check yet how close the old API's are to PSR-7.

Also, pretty happy that having goutte in core now means we have to wait until that supports it to, actually. Last time we did a major guzzle update days after it was released, I was blocked for weeks from upgrading because of behat/mink/goutte.

We actually can't switch to Guzzle 6, it requires PHP 5.5: https://twitter.com/mtdowling/status/603415290802614272

I replied on twitter to see what the lifespan of Guzzle 5 is likely to be like.

@iMiksu

It probably is not worth upgrading to 5.3 until we know which way to go. It is something we are going to check before the release anyway. See @Berdir's comment on the meta:

#2400407-18: [meta] Ensure vendor (PHP) libraries are on latest stable release

I've struggled through Guzzle 3 to 4, 4 to 5 updates (less so) before. And now again Guzzle 6 is quite a big api change. On the other hand, it is based on a PSR. If we can push the php5.5 requirement and I'd vote for adopting 6 and consolidating there for a while. Mtdownling might say there is no new major planned, but I've come to know Guzzle as a quickly changing library, so don't count on it staying at 6 for long.

#2296557: [policy] Require PHP 5.5 is confirmed and https://github.com/FriendsOfPHP/Goutte/issues/215 is awaiting a review. So the issues blocking this are pretty much lifted.

This also implies that we need to update fabpot/goutte. It makes sense to do it here as current version of Goutte won't work with Guzzle 6 and the update version of Goutte (when it comes) won't work with Guzzle 5. They have to go hand in hand. Should this issue be marked postponed then until we see a new release of Goutte?

Yes.

Yeah there is also a PR for it: https://github.com/FriendsOfPHP/Goutte/pull/218

Just for your information, I've just released Goutte 3.0.0 with Guzzle 6.0 support.

Thank you @fabpot and @csarrazi.

Postponing again. :(

I updated composer.json for both goutte and guzzle, and ran composer update. There is a conflict with behat/mink-goutte-extension.

hw@d8:/var/www/d8task/core-[git 8.0.x] $ composer update fabpot/goutte guzzleht
tp/guzzle
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for fabpot/goutte ~3.0 -> satisfiable by fabpot/goutte[3.0.x-dev, v3.0.0].
    - behat/mink-goutte-driver v1.1.0 requires fabpot/goutte ~1.0.4|~2.0 -> satisfiable by fabpot/goutte[1.0.x-dev].
    - behat/mink-goutte-driver v1.1.0 requires fabpot/goutte ~1.0.4|~2.0 -> satisfiable by fabpot/goutte[1.0.x-dev].
    - behat/mink-goutte-driver v1.1.0 requires fabpot/goutte ~1.0.4|~2.0 -> satisfiable by fabpot/goutte[1.0.x-dev].
    - Conclusion: don't install fabpot/goutte 1.0.x-dev
    - Installation request for behat/mink-goutte-driver == 1.1.0.0 -> satisfiable by behat/mink-goutte-driver[v1.1.0].

I am raising a PR there at https://github.com/minkphp/MinkGoutteDriver/pull/59.

Sorry for hijacking, but these sort of issues will be a lot easier when #2315545: Install composer dependencies to D8 when packaging and #1923582: Add ability for testbot to run 'composer install' during installation are committed because the vendor directory can be removed from the repo.

@timmillwood: I know. I have been waiting for that to land since forever. Until then, we can at least try and keep the libraries updated and see if there are test failures.

FYI: Guzzle 6 also fixes the issue where HTTPS connections using self-signed certificates are allowed by default (when the curl extension is disabled).

Also, someone should create a followup to update the comment in Drupal\Core\Http\Client since Guzzle doesn't include a default CA-bundle anymore.

The PR for MinkGoutteDriver I created (https://github.com/minkphp/MinkGoutteDriver/pull/59) is still in works. It now passes on all PHP versions up to 5.6 but fails on HHVM. The reason is that Guzzle itself fails on HHVM. I have created a PR for this at https://github.com/guzzle/guzzle/pull/1143.

Please help there as I am not very sure if the MinkGoutteDriver PR would get merged in without HHVM tests passing.

My PR on MinkGoutteDriver has been merged, but a release has not yet been created. Once that is done, I believe we will be able to move ahead with this issue.

You can still use the commit hash in composer.json

@jibran: True, but let's just wait and use a release. If it takes a long time, yes, we could use the commit hash.

Come to think of it, we can at least get a preview of what the tests will look. Tests won't pass, of course, since Guzzle requires PHP 5.5 and our testbots are 5.4, but let's give it a shot.

Adding tags and related issues.

Now that we have some reference results, postponing again.

This is a major version upgrade, so priority should be at least major.

According to the issue summary of #2400407: [meta] Ensure vendor (PHP) libraries are on latest stable release, both major and minor (but not patch-level) upgrades of dependencies should be a critical Drupal issue at this point, and I agree with that. So, raising accordingly.

@effulgentsia - It looks as though we won't have RC1 until testbot is updated to 5.5.9 then.

I'll try to start working on updating the usages of guzzle inside core.

@effulgentsia - It looks as though we won't have RC1 until testbot is updated to 5.5.9 then.

Isn't this already the case because of #2508231: Raise minimum required version of PHP to 5.5.9 being critical as well?

So, AFAICT we now have 2 criticals blocked on infrastructure.

The new API is pretty nice to use, is easily grockable and opens up a new area of maintainability of Drupal, see \Drupal\Core\Test\EventSubscriber\HttpRequestSubscriber

Well, this is critical because it sort of changes the API, BUT
this is actually mostly the case for additional things like subscriber, the other API changes are quite small,
so ->get(), ->post() ... still work.

Btw. this at least runs all the REST requests fine.

Given that I would be fine with moving this just to major, even we should really not ship with a unmaintained HTTP library.

I'm sorry, this was wrong, thanks berdir. I ran some OPML test, which uses guzzle.

Not really novice anymore

Took a look at the BrowserTestBase fails, php 5.5 issues too (nice)

Travis tests running (unit + functional only) here https://github.com/larowlan/drupal/pull/5

Unit tests are passing, but functional tests (BrowserTestBase) are not - https://travis-ci.org/larowlan/drupal/jobs/69995483

Mh, that failing test works fine on my machine.

+++ b/core/composer.json
@@ -31,8 +31,8 @@
+    "behat/mink-goutte-driver": "dev-master",

Can we specify the hash needed (and range) rather than an open-ended dependency?
https://getcomposer.org/doc/articles/versions.md#range

Maybe something like:

dev-master#44563f46fa99fef2df50ce083048094828e068b7 || ^1.1.2

(or whatever the next version will be).

This will help people using Composer directly because they wont be chasing master.

Sure, we can do that, I personally hope they will tag rather sooner than later.
For now though we are blocked on 5.5

To be clear, this seems to be more of an issue with travis CI somehow.

Nice we have now a proper bot

Some more work. Things are tricky, given that the architecture changed.
I think we need a handler/middleware stack much like our ordinary middlewares.

Just want to check whether we have less test failures with that.

diff --git a/core/composer.json b/core/composer.json
index 75814e2..45e0742 100644
--- a/core/composer.json
+++ b/core/composer.json
@@ -22,7 +22,7 @@
     "twig/twig": "1.18.*",
     "doctrine/common": "~2.4.2",
     "doctrine/annotations": "1.2.*",
-    "guzzlehttp/guzzle": "~5.0",
+    "guzzlehttp/guzzle": "~6.0",
     "symfony-cmf/routing": "1.3.*",
     "easyrdf/easyrdf": "0.9.*",
     "phpunit/phpunit": "4.6.*",
@@ -31,8 +31,8 @@
     "stack/builder": "1.0.*",
     "egulias/email-validator": "1.2.*",
     "behat/mink": "~1.6",
-    "behat/mink-goutte-driver": "~1.1",
-    "fabpot/goutte": "^2.0.3",
+    "behat/mink-goutte-driver": "dev-master",
+    "fabpot/goutte": "~3.1",
     "masterminds/html5": "~2.1",
     "symfony/psr-http-message-bridge": "v0.2",
     "zendframework/zend-diactoros": "1.1.0"

Isn't that true?

Core only patch form #54 because I want to review it. :)

I agree with #54 that we shouldn't be requiring a dev-master this could cause a whole host of issues for those who plan to manage their D8 install with Composer.

+ "behat/mink-goutte-driver": "dev-master",

No we can require dev-master now and then open another critical issue to pin to an actual version as soon as that's out. We shouldn't release using a dev version, but 8.0.x is a dev version so people managing sites with composer can deal with it for a few weeks.

That change should be much smaller impact than this one, so makes sense to get in front of things now - and if we find issues in the dev version that should help get upstream get a stable out quicker too.

#59,

Since we have a minimum-stabiliity of dev and prefer-stable is set to true, I think behat/mink-goutte-driver should use the version number ^1.3.0. That will install dev-master until 1.3.0 comes out, at which point only stable releases will be used. If we need to update the reference in the future we can do that. But setting it to ^1.3.0 prevents us from having to create release-blocking critical issue.

https://github.com/minkphp/MinkGoutteDriver/issues/63#issuecomment-11995...

Yeah the focus of this issue should be to make things actually working.

• Core uses the Guzzle 5 function to retrieve the actual URL, this API function no longer exists
• The redirect functionality is implemented as middleware now: \GuzzleHttp\RedirectMiddleware
• We need to figure out how to add such a middleware all the time
• Even once we have that, we need a way to pull out the actual URL, still
• Maybe we could implement something which wraps the result into an object which also has the URL stored. Another way would be to somehow use
  \GuzzleHttp\Middleware::history to get out, what is needed. Using that though is not obvious,
  given that Drupal uses a client instance as service, so the reference in ::history is not just there

Note: There is currently no easy way to figure out the actual URL of a response

Is it worth an upstream issue about that? This seems like an awful lot of work for something very simple, and @mtdowling has been very responsive in the past.

Well upstream === PSR-7 interfaces

But sure, let's open a support question: https://github.com/guzzle/guzzle/issues/1166

Can't be bad to add some tag.

poking about

Some progress,
- uses a factory for the handler stack - which ensure we always get the redirects etc
- adds catch redirect/store redirect handlers into the stack to catch changes to the url and store them in an X-Effective-Url header
- not sure on the missing E-tag headers on feeds yet

Well, let's better use the upstream functionality, which is provided for us.

I think it is in general fine for reach caller to explicitly write a callback, when they want this URL.

More work

The __invoke in earlier patches wasn't being used because the default_config key in the constructor is no longer used.

Also ->getHeader() signature has changed

Seeing some tests green locally.

Yep, we can drop our catch_redirect/store_redirect stack handlers when upstream is ready (https://github.com/guzzle/guzzle/pull/1167 for others)

1. +++ b/core/core.services.yml
   @@ -374,10 +374,13 @@ services:
      http_client:
        class: Drupal\Core\Http\Client
   ...
   +    arguments: ['@http_handler_stack', '@service_container']
   
   +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/GuzzleHandlerPass.php
   @@ -0,0 +1,25 @@
   +    $container->getDefinition('http_client')
   +      ->addArgument($service_ids);
   
   +++ b/core/lib/Drupal/Core/Http/Client.php
   @@ -10,17 +10,34 @@
   +    foreach ($service_ids as $id) {
   +      $stack->push($container->get($id)->register());
   +    }
   

   That just doesn't work ... I think we should provide our own factory at least and not make client sort of container aware

2. +++ b/core/lib/Drupal.php
   @@ -395,7 +395,7 @@ public static function state() {
       *
   -   * @return \GuzzleHttp\ClientInterface
   +   * @return \GuzzleHttp\Client
       *   A guzzle http client instance.
   

   Note: this is what we want ... because it has additional documentation we should be able to leverage.

3. +++ b/core/lib/Drupal/Core/Test/EventSubscriber/HttpRequestSubscriber.php
   @@ -7,36 +7,33 @@
   +class HttpRequestSubscriber implements StackHandlerRegistrationInterface {
   ...
   +  public function register() {
   

   Well, I'm not sure whether we want to introduce a custom interface. Yes it helps developers potentially, but it moves us away from helpful documentation about guzzle. I think we should adapt to their stile of solving things.

That just doesn't work ... I think we should provide our own factory at least and not make client sort of container aware

I found it the other way, the __invoke didn't work. Earlier patches were ignoring the 'handler' because we were passing it in $config['default_config']['handler'] instead of just $config['handler']. Soon as we pass the handler through it blows up. First, with the stack referencing itself, and then with the __invoke being treated as a middleware and not a closure. And neither did a factory because our container builder tries to set __serviceId on stuff, which you can't do on a closure. And neither does passing the closure as an argument with addMapping, because arguments need to be statics.

In the docs http://docs.guzzlephp.org/en/latest/handlers-and-middleware.html#middleware you'll note that the push method requires a closure. We're pushing an object, which when it tries to call it (later on) we get the $request, not the handler.

So we need a way for the container to add a method call that pushes the return of $service() (aka __invoke), not the service itself.

I've been tinkering on it for the best part of 1/2 a day and the only thing I could come up with that worked was the pseudo container aware approach. We could make it a different service to avoid polluting the http_client.

Yeah @larowlan, an update seems so obvious and easy at the first view :)
If you want, we can discuss on IRC.

We decided in critical issues discussion meeting to postpone this on https://github.com/guzzle/guzzle/pull/1167.

While postponing is not a bad idea there are still tasks which have to be done.

Here is some conversion of using a factory, which allows you to set up custom options.
At the same time this allows us to get rid of a custom client object, which is not that bad.

1. +++ b/core/lib/Drupal/Core/Http/ClientFactory.php
   @@ -0,0 +1,70 @@
   +    $config = NestedArray::mergeDeep(array($default_config), Settings::get('http_client_config', array()));
   

   The $config parameter passed into fromOptions() is not added here.

2. +++ b/core/lib/Drupal/Core/Http/StackHandlerRegistrationInterface.php
   @@ -0,0 +1,20 @@
   +interface StackHandlerRegistrationInterface {
   

   Looks like the usage of this is more like the Guzzle middlewares rather than just adding a handler to the stack.

PR got merged.

Now we just need to wait on packagist

Updates as well as using the new callback.

So I was thinking a service configurator service is the go for executing the register method on the http_client_handler services and adding them to the stack, to avoid making the client container aware.

I will tackle that in the morning (in 14 hrs).

http://symfony.com/doc/current/components/dependency_injection/configura...

So I was thinking a service configurator service is the go for executing the register method on the http_client_handler services and adding them to the stack, to avoid making the client container aware.

Well, I think we could just put the logic + container awareness into the factory. It also helps with the other usecaess,
where you want to be able to specify custom options ...

@larowlan
Do you really think adding a custom interface is a good idea? I can't imagine that doing that is really great ...

taking a look

removes the interface
adds a configurator
fixes the redirect code
changes the tag to http_client_middleware

configurator not working yet

1. +++ b/core/lib/Drupal/Core/Http/Client.php
   @@ -31,40 +37,17 @@ public function __construct(array $config = []) {
   +        'User-Agent' => 'Drupal/' . \Drupal::VERSION . ' (+https://www.drupal.org/) ' . \GuzzleHttp\default_user_agent(),
   

   A name spaced function, interesting pattern.

2. +++ b/core/lib/Drupal/Core/Http/ClientFactory.php
   @@ -0,0 +1,70 @@
   +      // Security consideration: we must not use the certificate authority
   +      // file shipped with Guzzle because it can easily get outdated if a
   +      // certificate authority is hacked. Instead, we rely on the certificate
   +      // authority file provided by the operating system which is more likely
   +      // going to be updated in a timely fashion. This overrides the default
   +      // path to the pem file bundled with Guzzle.
   +      'verify' => TRUE,
   

   This sounds like we should actually be specifying the new path? I'm guessing that happens somewhere else.

3. +++ b/core/lib/Drupal/Core/Test/EventSubscriber/HttpRequestSubscriber.php
   @@ -7,36 +7,33 @@
   -  /**
   -   * Event callback for the 'before' event
   -   */
   -  public function onBeforeSendRequest(BeforeEvent $event) {
   

   I kind of feel like we've lost some info here. Previously, it was obvious we do something before a request is sent from the method name, now that info is lost? Also, is inheritdoc valid here given the class doesn't extend anything?

4. +++ b/core/modules/aggregator/src/Plugin/aggregator/fetcher/DefaultFetcher.php
   @@ -91,13 +101,17 @@ public function fetch(FeedInterface $feed) {
   +        $feed->setEtag($response->getHeader('ETag')[0]);
   ...
   +        $feed->setLastModified(strtotime($response->getHeader('Last-Modified')[0]));
   

   Could we use getHeaderLine() here?

5. +++ b/core/modules/locale/locale.batch.inc
   @@ -234,15 +237,19 @@ function locale_translation_batch_fetch_finished($success, $results) {
   +    $result['last_modified'] = $response->hasHeader('Last-Modified') ? strtotime($response->getHeader('Last-Modified')[0]) : 0;
   

   here as well.

FailsConfigurator broken during container building because of https://github.com/symfony/symfony/issues/14834

So we need to update symfony/dependency-injection from 2.7.0 to 2.7.1

Do we have an issue for that?

@larowlan looks like that is covered by #2504967: Upgrade to Symfony 2.7.1

If that issue blocks this, then we should raise the priority for that issue to critical.

@larowlan
Do you mind explaining why we need a configuration and the factory would not be enough?

I think I now totally understand the purpose of the configurator, its just to separate the logic out. I like that.

@larowlan
I rewrote the configurator to do things more lazy. I think its always good to give people an example how to do things, even for this case, it probably practically doesn't matter.

This sounds like we should actually be specifying the new path? I'm guessing that happens somewhere else.

I think we actually don't need it, see \GuzzleHttp\Client::configureDefaults which sets this variable to TRUE as well.

I kind of feel like we've lost some info here. Previously, it was obvious we do something before a request is sent from the method name, now that info is lost? Also, is inheritdoc valid here given the class doesn't extend anything?

Well, the concept of a middleware is that you can wrap both before and after.

Could we use getHeaderLine() here?

Good point.

+++ b/core/lib/Drupal/Core/Http/ClientFactory.php
@@ -0,0 +1,70 @@
+class ClientFactory {

Let's add the interface as well.

Let's see whether this works now.

Based on how disruptive this change has been for core (sad panda :\), this is very likely to be disruptive for contrib as well. Therefore, tagging as a "beta target" for one to try and get in before the next beta (tentatively July 22).

wrong use of invoke (some times you should trust your own judgement over phpstorm)

Well, the concept of a middleware is that you can wrap both before and after.

Sure, my point was that previously we had some context from the method name, now we don't and therefore maybe a comment would help. Something like, "Do x before the request is sent."

Sure, my point was that previously we had some context from the method name, now we don't and therefore maybe a comment would help. Something like, "Do x before the request is sent."

I doubt you can do anything without reading the official documentation , so I put a link to it into some places.

• Fixed the tests
• Added some test coverage
• Added some documentation

Forgot to add the new test.

+++ b/core/lib/Drupal/Core/Http/HandlerStackConfigurator.php
@@ -0,0 +1,94 @@
+   * Configures the handler stack using services tagged as http_middleware.

Before I forget: this should be http_client_middleware now

Ah, good eye!

Couple of minor changes then I think this is rtbc - thanks @dawehner

1. +++ b/core/composer.json
   @@ -22,7 +22,7 @@
   +    "guzzlehttp/guzzle": "dev-master",
   

   Can we pin this to ~6.0?

2. +++ b/core/composer.json
   @@ -31,8 +31,8 @@
   +    "behat/mink-goutte-driver": "dev-master",
   

   Can we pin a specific commit here?

3. +++ b/core/composer.lock
   @@ -3446,7 +3414,10 @@
   +        "guzzlehttp/guzzle": 20,
   

   Then these should go right?

4. +++ b/core/lib/Drupal.php
   @@ -403,6 +403,15 @@ public static function httpClient() {
   +   * Returns a factory for guzzle HTTP clients with Drupal drupal options.
   

   Drupal drupal (now I have that damn song in my head)

5. +++ b/core/lib/Drupal/Core/Test/EventSubscriber/HttpRequestSubscriber.php
   @@ -7,36 +7,32 @@
   +use Drupal\Core\Http\StackHandlerRegistrationInterface;
   

   No longer needed

Can we pin this to ~6.0?

Well, yeah not yet, see https://packagist.org/packages/guzzlehttp/guzzle

No longer needed

I think it is still pretty amazing that PHP does not complain about it.

Can we pin this to ~6.0?

Well, yeah not yet, see https://packagist.org/packages/guzzlehttp/guzzle

That says there is a 6.0.2 release?

Yes there was, but it doesn't include yet the on_redirect feature, see https://github.com/guzzle/guzzle/releases/tag/6.0.2 (its 10 days old)

+++ b/core/lib/Drupal/Core/Test/EventSubscriber/HttpRequestSubscriber.php
@@ -7,36 +7,31 @@
   /**
    * {@inheritdoc}
    */
...
+  public function __invoke() {

We still have the inheritdoc here which doesn't relate to anything that can be parsed?

There we go. I don't think its important that its before, but rather what it does: It manipulates the request.

Yeah agreed, that makes sense.

This is looking good to me, couple of small nitpicks and a question for my own benefit then I think RTBC.

1. +++ b/core/lib/Drupal/Core/Http/HandlerStackConfigurator.php
   @@ -0,0 +1,94 @@
   +  /**
   +   * Array of middlewares to add to the handler stack.
   +   *
   +   * @var callable[]
   +   */
   +  protected $middlewares = NULL;
   +
   +  /**
   +   * A list of used middleware service IDs.
   +   *
   +   * @var string[]
   +   */
   +  protected $middlewareIds;
   

   These are both mean to be arrays, one is initialised to NULL, the other is implicitly NULL. Should they both be []?

2. +++ b/core/lib/Drupal/Core/Http/HandlerStackConfigurator.php
   @@ -0,0 +1,94 @@
   +  public function configure(HandlerStack $handler_stack) {
   +    $this->initializeMiddlewares();
   +    foreach ($this->middlewares as $middleware_id => $middleware) {
   

   If you call configure multiple times the middlewares are only initialised once but they're pushed onto the $handler_stack every time. Just asking if that is correct? I couldn't actually find where this was called from.

3. +++ b/core/modules/locale/locale.batch.inc
   @@ -234,15 +237,19 @@ function locale_translation_batch_fetch_finished($success, $results) {
    function locale_translation_http_check($uri) {
   ...
   +      'on_redirect' => function(RequestInterface $request, ResponseInterface $response, UriInterface $uri) use (&$actual_uri) {
   

   Confused me at first, but we have two different variables called $uri in this function. One in the closure, one outside.

Thank you for your review @benjy

These are both mean to be arrays, one is initialised to NULL, the other is implicitly NULL. Should they both be []?

Well, the NULL is indented to use the isset() check below.

If you call configure multiple times the middlewares are only initialised once but they're pushed onto the $handler_stack every time. Just asking if that is correct? I couldn't actually find where this was called from.

Yeah this is called from within the container. The configurator is designed in a way that you don't have to care about it, which is great, as the configurator itself can be really simple.

A compiled container looks like the following:

        $a = \GuzzleHttp\HandlerStack::create();
        call_user_func(array(new \Drupal\Core\Http\HandlerStackConfigurator($this, array()), 'configure'), $a);

        $this->services['http_client_factory'] = $instance = new \Drupal\Core\Http\ClientFactory($a);

        $instance->_serviceId = 'http_client_factory';

        return $instance;

Confused me at first, but we have two different variables called $uri in this function. One in the closure, one outside.

Well, technically this is perfectly fine, but sure, let's rename it.

Awesome, thanks dawehner

Needs a reroll.

Also...

+++ b/core/lib/Drupal.php
@@ -403,6 +403,15 @@ public static function httpClient() {
   /**
+   * Returns a factory for guzzle HTTP clients with Drupal options.
+   *
+   * @return \Drupal\Core\Http\ClientFactory
+   */
+  public static function httpClientFactory() {
+    return static::getContainer()->get('http_client_factory');
+  }

Is this really worth it? Most of the usages are tests and i'd be confused about whether I should be using \Drupal::httpClient() vs \Drupal::httpClientFactory()

/me dreams of a world without dependencies in version control.

/me dreams of a world without dependencies in version control.

Best decision ever to not put that onto priority on drupal.org

dawehner --

Just a quick review.

1. +++ b/core/core.api.php
   @@ -2190,6 +2190,9 @@ function hook_validation_constraint_alter(array &$definitions) {
   + *   middleware, see https://guzzle.readthedocs.org/en/latest/handlers-and-middleware.html
   

   Can we move the link to next line?

2. +++ b/core/lib/Drupal/Core/Http/ClientFactory.php
   @@ -0,0 +1,70 @@
   +class ClientFactory {
   

   Why not add a interface here as well?

Thank you jibran!

Why not add a interface here as well?

Can you describe why we need that? Why would you want to change the behaviour of all other other HTTP clients not part of your module?
If you want to change the behaviour you can also write a middleware.

That's fair enough. Thanks for the explanation. RTBC as per #123.

Thank you jibran!

Nice one getting this green.

One question:

1. +++ b/core/modules/aggregator/src/Plugin/aggregator/fetcher/DefaultFetcher.php
   @@ -70,19 +74,25 @@ public static function create(ContainerInterface $container, array $configuratio
        try {
   -      $response = $this->httpClient->send($request);
   +
   +      /** @var \Psr\Http\Message\UriInterface $actual_uri */
   +      $actual_uri = NULL;
   +      $response = $this->httpClientFactory->fromOptions(['allow_redirects' => [
   +        'on_redirect' => function(RequestInterface $request, ResponseInterface $response, UriInterface $uri) use (&$actual_uri) {
   +        $actual_uri = (string) $uri;
   +      }]])->send($request);
    
   

   This is much, much more verbose - we can't add an extra method to the service to encapsulate?

2. +++ b/core/modules/locale/locale.batch.inc
   @@ -234,15 +237,19 @@ function locale_translation_batch_fetch_finished($success, $results) {
    function locale_translation_http_check($uri) {
      $logger = \Drupal::logger('locale');
      try {
   -    $response = \Drupal::httpClient()->head($uri);
   +    $actual_uri = NULL;
   +    $response = \Drupal::service('http_client_factory')->fromOptions(['allow_redirects' => [
   +      'on_redirect' => function(RequestInterface $request, ResponseInterface $response, UriInterface $request_uri) use (&$actual_uri) {
   +      $actual_uri = (string) $request_uri;
   +    }]])->head($uri);
        $result = array();
    
   

   Almost the same here, I expect any contirb/custom code using guzzle to make requests will end up copy/pasting this over and over again.

The config option passed as the parameter to fromOptions() needs to come last, otherwise the default options and options from Settings will always override what we pass in. We just don't want that :)

Here is a new test that should show the failure, and the fix.

Sorry, pretty much x-posted with catch.

1. +++ b/core/tests/Drupal/Tests/Core/Http/ClientFactoryTest.php
   @@ -7,6 +7,68 @@
   + * @group Http
   + */
   +class ClientFactoryTest extends UnitTestCase {
   

   <3

2. +++ b/core/tests/Drupal/Tests/Core/Http/ClientFactoryTest.php
   @@ -7,6 +7,68 @@
   +    $stack = $this->getMockBuilder('GuzzleHttp\HandlerStack')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +    $this->factory = new ClientFactory($stack);
   

   You could also just create a new HandlerStack , but meh

2. I thought that, but then I would be passing in an empty closure or something to construct it. Which was maybe a bit weird? This makes it more obvious that we don't care about the stack. I could be swayed though :)

2. I thought that, but then I would be passing in an empty closure or something to construct it. Which was maybe a bit weird? This makes it more obvious that we don't care about the stack. I could be swayed though :)

Fair point.

Suggestion. Track the URI in the client itself :(, but then provide something like $client->getLastEffectiveUrl() or so

From the D8 Core EU Criticals Meeting:

- Use the new on_redirect function
- Store the redirects on the response and enumerate them:

X-Drupal-Redirect-Count: 2
X-Drupal-Redirect-1: ...
X-Drupal-Redirect-2: ...

etc.

That keeps the data where it belongs (on the value object) and we and/or upstream can provide a helper function to get the redirects (or first/last redirect) out of the response object.

That is also middleware compatible.

Overall HTTP headers go into the direction of getting more and more used for transferring additional metadata and that is a good thing IMHO.

So this feels like the best option at this moment.

I just tried to implement that directly in guzzle: https://github.com/guzzle/guzzle/issues/1183

Had a look at that and I think the easiest solution is indeed to fix it in guzzle directly. Let's wait, I'd say

Per #143.

.

I'm curious whether we could commit it with the current state and then get the upstream fix in later or improve the API in Drupal if this doesn't work out?

+1 to the idea because it would not have to be a critical.

I think it is fine to commit this now, but I will have to say that the followup would also have to be a critical. I remember reading somewhere that we wouldn't/shouldn't release the RC with composer.json locked to a specific commit (like we have in the patch). That means the followup will be a blocker to release, and hence critical.

The reason I think it is fine to commit now would be to allow reviews for the followup to be carried out on a smaller, more specific patch.

I think it is fine to commit this now, but I will have to say that the followup would also have to be a critical. I remember reading somewhere that we wouldn't/shouldn't release the RC with composer.json locked to a specific commit (like we have in the patch). That means the followup will be a blocker to release, and hence critical.

Well, that would be a minor update, wouldn't it?

the followup will be a blocker to release, and hence critical.

This would be part of the RC1 issue.

I also agree that this issue should not wait on that. This one is done, aside from one DX problem/feature that is out of our hands. The upgrade is needed regardless so we are using a supported Guzzle version. A smaller incremental update can take care of that surely?

I also agree with #150. Setting to NR, but if someone who's reviewed this patch believes it's RTBC, go ahead and set to that.

Those was my only question on #134 when I looked at it, and I'm fine with the answer to that question being a follow-up postponed on the upstream issue.

Thank you @effulgentsia and @catch

Reupload the old issue. It still applied cleanly, so this should be RTBC

It was RTBC in #132. We decided to move #134 to follow up. @dawehner is happy with the changes with #135 so setting it back to RTBC. We just need a follow up here now.

+++ /dev/null
@@ -1,70 +0,0 @@
-    // The entire config array is merged/configurable to allow Guzzle client
-    // options outside of 'defaults' to be changed, such as 'adapter', or
-    // 'message_factory'.
-    $config = NestedArray::mergeDeep(array('defaults' => $default_config), $config, Settings::get('http_client_config', array()));
...
+++ b/core/lib/Drupal/Core/Http/ClientFactory.php
@@ -0,0 +1,70 @@
+    // The entire config array is merged/configurable to allow Guzzle client
+    // options outside of 'defaults' to be changed, such as 'adapter', or
+    // 'message_factory'.
+    $config = NestedArray::mergeDeep($default_config, Settings::get('http_client_config', []), $config);

Is the change from array('defaults' => $default_config) to $default_config intentional? If so, should the comment be updated?

1. +++ b/core/core.services.yml
   @@ -379,10 +379,21 @@ services:
      http_client:
   -    class: Drupal\Core\Http\Client
   +    class: GuzzleHttp\Client
   +    factory: http_client_factory:fromOptions
   

   Should we change class to GuzzleHttp\ClientInterface instead? Is there any reason to disallow swapping the factory with one that returns a ClientInterface implementation that doesn't subclass Guzzle's?

2. +++ b/core/lib/Drupal.php
   @@ -395,7 +395,7 @@ public static function state() {
   -   * @return \GuzzleHttp\ClientInterface
   +   * @return \GuzzleHttp\Client
   

   Same here. #72.2 says we want this because Client has some nice docs, but I think doing this sends the message that the implementation can't be swapped, and if that's not the case, then let's not send that message.

3. +++ b/core/lib/Drupal/Core/Test/EventSubscriber/HttpRequestSubscriber.php
   @@ -7,36 +7,33 @@
   -class HttpRequestSubscriber implements SubscriberInterface {
   +class HttpRequestSubscriber {
   

   Since this is no longer an event subscriber, let's rename it and remove the containing EventSubscriber directory. Perhaps Drupal\Core\Test\TestHttpClientMiddleware?

4. +++ b/core/modules/aggregator/src/Plugin/aggregator/fetcher/DefaultFetcher.php
   @@ -70,19 +74,25 @@ public static function create(ContainerInterface $container, array $configuratio
   -      $response = $this->httpClient->send($request);
   +
   +      /** @var \Psr\Http\Message\UriInterface $actual_uri */
   +      $actual_uri = NULL;
   +      $response = $this->httpClientFactory->fromOptions(['allow_redirects' => [
   +        'on_redirect' => function(RequestInterface $request, ResponseInterface $response, UriInterface $uri) use (&$actual_uri) {
   +        $actual_uri = (string) $uri;
   +      }]])->send($request);
   ...
   <code>
   +++ b/core/modules/locale/locale.batch.inc
   @@ -234,15 +237,19 @@ function locale_translation_batch_fetch_finished($success, $results) {
   -    $response = \Drupal::httpClient()->head($uri);
   +    $actual_uri = NULL;
   +    $response = \Drupal::service('http_client_factory')->fromOptions(['allow_redirects' => [
   +      'on_redirect' => function(RequestInterface $request, ResponseInterface $response, UriInterface $request_uri) use (&$actual_uri) {
   +      $actual_uri = (string) $request_uri;
   +    }]])->head($uri);
   

   Confusing whitespacing. I think it would be clearer to indent the $actual_uri = (string) $request_uri; line and then have the closing } on its own line, matching the indentation of 'on_redirect'.

   Also, can we file the follow-up referenced in #152 / #154, and add @todo links to that issue above these lines.

Also, I think a CR would be valuable for this, mentioning the upgrade in general, and some key highlights about what module developers should know, such as the switch to PSR-7 and the need to use the factory when wanting a client with customized options.

Thank you for your review!

Should we change class to GuzzleHttp\ClientInterface instead? Is there any reason to disallow swapping the factory with one that returns a ClientInterface implementation that doesn't subclass Guzzle's?

Well yeah its a factory, you can put in anything here, even a non existing one :)

Same here. #72.2 says we want this because Client has some nice docs, but I think doing this sends the message that the implementation can't be swapped, and if that's not the case, then let's not send that message.

Well, that is tricky. $client->get() is not guaranteed on the ClientInterface itself, so effectively there are boundaries of swapping things out. Also almost the entire behaviour you want to swap is in handlers/middlewares which you can swap now.
What about doing \GuzzleHttp\ClientInterface|\GuzzleHttp\Client to kind of communicate both things at the same time?

Since this is no longer an event subscriber, let's rename it and remove the containing EventSubscriber directory. Perhaps Drupal\Core\Test\TestHttpClientMiddleware?

Fair point.

Well, I would prefer to do as few as possible and rather link to the official documentation, so people learn where they should actually look for things, but sure, let's create one.

I will work on addressing these reviews when its not 2am in the morning.

$client->get() is not guaranteed on the ClientInterface itself

D'oh. I should have looked and noticed that myself. In that case, ignore #156.1 and #156.2, because we really are then constraining the service to be a full-fledged GuzzleHttp\Client.

Well, I would prefer to do as few as possible and rather link to the official documentation, so people learn where they should actually look for things, but sure, let's create one.

I'm assuming this is referring to my request for a CR. Yeah, I think it can be small. I think it could just mention the adoption of PSR-7, but link to an appropriate docs page for more info. But, I think it should mention explicitly that the client service can still be used when it doesn't need instance-specific options (just like before this change), but that a new client needs to be created from the factory service when any instance-specific options are wanted (a new pattern after this change). And have a before/after code sample to that effect. Such as:

+++ b/core/modules/statistics/src/Tests/StatisticsAdminTest.php
@@ -54,8 +54,8 @@ protected function setUp() {
-    $this->client = \Drupal::httpClient();
-    $this->client->setDefaultOption('config/curl', array(CURLOPT_TIMEOUT => 10));
+    $this->client = \Drupal::service('http_client_factory')
+      ->fromOptions(['config/curl' => [CURLOPT_TIMEOUT => 10]]);

D'oh. I should have looked and noticed that myself. In that case, ignore #156.1 and #156.2, because we really are then constraining the service to be a full-fledged GuzzleHttp\Client.

Yeah the ClientInterface is more of a super PURE http thing.

Here is a feedback addressing #157.3 and #157.4

Writing a CR now.

CR is written.

CR looks good, so removing tag. Adding benjy and iMiksu for commit credit for their substantive comments. Is there anyone else who should be added?

Well yeah, thank you mtdowling for the help on github, sadly you can't add them here.
+1 for the list of people.

Addressed the missing feedback.

He has a d.o profile though https://www.drupal.org/u/mtdowling.

@jibran
I guess though he would have to have a comment on this issue, doesn't he?

Pushed to 8.0.x. Thanks for the great work. I added mtdowling to the commit message manually. See #2474609: Not possible to credit people who didn't comment in an issue for improving d.o. to make that easier.

This issue may have affected #2595927: Update guzzle to fix 100% CPU usage with curl_multi_exec() in GuzzleHttp\Handler\CurlMultiHandler->tick() during install