Sub-task of #1862398: [meta] Replace drupal_http_request() with Guzzle.

CommentFileSizeAuthor
#11 update-fetch-guzzle-1862538-11.patch1.21 KBBerdir
#9 update-fetch-guzzle-1862538-9.patch1.21 KBBerdir
#3 update-guzzle-1862538-3.patch1.41 KBBerdir
#1 update-guzzle-1862538-1.patch2.36 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
update-guzzle-1862538-1.patch2.36 KB

Another easy conversion, haven't run the tests yet.

Status: Needs review » Needs work

The last submitted patch, update-guzzle-1862538-1.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
update-guzzle-1862538-3.patch1.41 KB

This should pass the tests and is more like the original code.

rbayliss’s picture

rbayliss CreditAttribution: rbayliss commented

Looks good to me.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

#3: update-guzzle-1862538-3.patch queued for re-testing.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Status: Needs review » Reviewed & tested by the community

I've got nothing to add. It's RTBC to me.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Reviewed & tested by the community » Needs review

Needs to be re-rolled to use the exception handling pattern from the standardize issue. Which should probably wait until that is commited.

Otherwise, this will result in an uncatched exception bubbling up all the way if e.g. the network or d.o is down.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

here is the standardize issue @Berdir suggests going in first: #1875792: Standardize Guzzle exception handling

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
update-fetch-guzzle-1862538-9.patch1.21 KB

Re-roll. Looks better now :)

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs review » Needs work

just one thing

+++ b/core/modules/update/update.fetch.incundefined
@@ -149,12 +151,13 @@ function _update_process_fetch_task($project) {
+    } catch (RequestException $exception) {

should be in a newline after closing }

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
update-fetch-guzzle-1862538-11.patch1.21 KB

There you go. Was too lazy for an interdiff ;)

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

ready to fly, thanks:)

twistor’s picture

twistor CreditAttribution: twistor commented

And what about BadResponseException?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

That's a subclass of RequestException. We only need to differentiate if we want to log different errors otherwise re-act differently on them. I think the default getMessage() of either exception is fine for this.

twistor’s picture

twistor CreditAttribution: twistor commented

Ahh, right.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 28f278b and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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