Problem/Motivation

Due to Guzzle update (from 5.0.2 to 6.0.2) fetch web resource is broken.
Change record: Guzzle updated to version 6

Proposed resolution

Update FetchWebResource with the changes.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 fix_fetch_web_resource-2537500-11-interdiff.txt1.02 KBmbovan
#11 fix_fetch_web_resource-2537500-11.patch5.26 KBmbovan
#7 fix_fetch_web_resource-2537500-7-interdiff.txt6.01 KBmbovan
#7 fix_fetch_web_resource-2537500-7.patch5.29 KBmbovan
#5 fix_fetch_web_resource-2537500-5-interdiff.txt1.13 KBmbovan
#5 fix_fetch_web_resource-2537500-5.patch6.47 KBmbovan
#3 fix_fetch_web_resource-2537500-3-interdiff.txt3.2 KBmbovan
#3 fix_fetch_web_resource-2537500-3.patch6.59 KBmbovan
#1 fix_fetch_web_resource-2537500-1.patch3.39 KBmbovan

Comments

mbovan’s picture

mbovan commented
Status: Active » Needs review
StatusFileSize
new fix_fetch_web_resource-2537500-1.patch3.39 KB

It seems that createRequest() doesn't return RequestInterface anymore, ResponseInterface is returned instead.

I had to create a Request object and pass it as a parameter to the send() method on the http client. Also, request headers are not returned in the same "structure" as response headers which required adapting one to another in order to parse it correctly by the FetchUrlPlugin.

Status: Needs review » Needs work

The last submitted patch, 1: fix_fetch_web_resource-2537500-1.patch, failed testing.

mbovan’s picture

mbovan commented
Status: Needs work » Needs review
StatusFileSize
new fix_fetch_web_resource-2537500-3.patch6.59 KB
new fix_fetch_web_resource-2537500-3-interdiff.txt3.2 KB

And some CollectClientQueueWorker changes and tests fixes. Let's try with this...

Status: Needs review » Needs work

The last submitted patch, 3: fix_fetch_web_resource-2537500-3.patch, failed testing.

mbovan’s picture

mbovan commented
Status: Needs work » Needs review
StatusFileSize
new fix_fetch_web_resource-2537500-5.patch6.47 KB
new fix_fetch_web_resource-2537500-5-interdiff.txt1.13 KB

It seems they changed the way how to add headers to the request...

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/client/src/Plugin/QueueWorker/CollectClientQueueWorker.php
@@ -231,7 +230,7 @@ class CollectClientQueueWorker extends QueueWorkerBase implements ContainerFacto
-        $response = $this->httpClient->post($url, array_merge($item_values, $options));
+        $response = $this->httpClient->request('POST', $url, array_merge($item_values, $options));

    is there a reason to not use ->post(), it still exists 

  2. +++ b/client/src/Plugin/QueueWorker/CollectClientQueueWorker.php
@@ -282,8 +281,12 @@ class CollectClientQueueWorker extends QueueWorkerBase implements ContainerFacto
+      if (!$response->getBody()) {
+        return FALSE;
+      }

    you could store the variable so if (!$body = $response->getBody()) 

  3. +++ b/src/FetchWebResource.php
@@ -53,15 +54,15 @@ class FetchWebResource {
+  public function __construct(ClientFactory $http_client_factory, EntityManagerInterface $entity_manager, ModelManagerInterface $model_manager) {
+    $this->httpClientFactory = $http_client_factory;
     $this->entityManager = $entity_manager;

@@ -83,18 +84,17 @@ class FetchWebResource {
+    // Get a http client.
+    $http_client = $this->httpClientFactory->fromOptions();

    You don't need to inject the factory in case you don't want to have specific options

mbovan’s picture

mbovan commented
StatusFileSize
new fix_fetch_web_resource-2537500-7.patch5.29 KB
new fix_fetch_web_resource-2537500-7-interdiff.txt6.01 KB

@dawehner, thanks for the review.

1. As discussed on irc, the reason was that my PHPStorm didn't find post() method and that we have an Unit test where we mock Guzzle Http Client, but mocked post() was not there. :)
Anyway, when we mocking an object we can specify the methods which will be mocked too (as second parameter of getMock()) , so reverted the changes.

2. Fixed.

3. Changed to \GuzzleHttp\ClientInterface.

edurenye’s picture

edurenye commented

Seems ok for me.

juanse254’s picture

juanse254 commented
Status: Needs review » Needs work 
  1. +++ b/client/src/Plugin/QueueWorker/CollectClientQueueWorker.php
@@ -282,8 +281,12 @@ class CollectClientQueueWorker extends QueueWorkerBase implements ContainerFacto
+

+++ b/src/FetchWebResource.php
@@ -83,17 +84,15 @@ class FetchWebResource {
-
...
-

    Unrelated empty lines 

  2. +++ b/src/FetchWebResource.php
@@ -53,14 +54,14 @@ class FetchWebResource {
+   *   A Guzzle client object.

    Delete extra empty space.

juanse254’s picture

juanse254 commented 
  1. +++ b/client/src/Plugin/QueueWorker/CollectClientQueueWorker.php
@@ -282,8 +281,12 @@ class CollectClientQueueWorker extends QueueWorkerBase implements ContainerFacto
+

    Empty Line 

  2. +++ b/src/FetchWebResource.php
@@ -83,17 +84,15 @@ class FetchWebResource {
-

    Remove unrelated empty line

Didnt notice they were not being shown, my bad.

mbovan’s picture

mbovan commented
Status: Needs work » Needs review
StatusFileSize
new fix_fetch_web_resource-2537500-11.patch5.26 KB
new fix_fetch_web_resource-2537500-11-interdiff.txt1.02 KB

Re #9 (1), #10 (1):

When you highlight only one (empty) line, it's hard to find which one you thought about. Also, you can edit your comment because #10 is the same as #9 (1). :)
Reverted the changes.

Re #9 (2):

Actually, one empty space is missing here. Added it.

  • Berdir committed a9edcaf on 8.x-1.x authored by mbovan 
    Issue #2537500 by mbovan: Fix fetch web resource service after Guzzle...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.