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

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new 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
commented
Status: Needs work » Needs review
StatusFileSize
new update-guzzle-1862538-3.patch1.41 KB

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

rbayliss’s picture

rbayliss commented

Looks good to me.

yesct’s picture

yesct commented

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

sutharsan’s picture

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
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 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
commented
StatusFileSize
new update-fetch-guzzle-1862538-9.patch1.21 KB

Re-roll. Looks better now :)

ParisLiakos’s picture

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
commented
Status: Needs work » Needs review
StatusFileSize
new update-fetch-guzzle-1862538-11.patch1.21 KB

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

ParisLiakos’s picture

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

ready to fly, thanks:)

twistor’s picture

twistor commented

And what about BadResponseException?

berdir’s picture

berdir
Primary language German
Location Switzerland
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 commented

Ahh, right.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
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.