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

CommentFileSizeAuthor
#18 aggregator-guzzle-1862524-18.patch1.87 KBParisLiakos
#13 aggregator-guzzle-1862524-13.patch1.87 KBBerdir
#12 aggregator-guzzle-1862524-12.patch2.17 KBSutharsan
#6 aggregator-guzzle-1862524-6.patch2.16 KBBerdir
#1 aggregator-guzzle-1862524-1.patch2.32 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
aggregator-guzzle-1862524-1.patch2.32 KB

Conversion is easy, although the code in there is weird. There is no user facing error message when the opml download fails, just a green "nothing to do" message, but that's not my problem here:)

When doing manual testing, I noticed that we can't rely on $e->getResponse() because that might be NULL. If you enter a non-existing domain, then $e->getResponse()->getReasonPhrase() is a fatal error. Not sure what the recommended error handling is. It does throw an error on 404, at least, that much I've checked.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I suggest we also fix the DefaultFetcher issues in here, there are a lot of follow-ups already...

- Handling of 301 redirected URL
- Improve error handling to prevent fatal errors.

Status: Needs review » Needs work

The last submitted patch, aggregator-guzzle-1862524-1.patch, failed testing.

mtdowling’s picture

mtdowling CreditAttribution: mtdowling commented

Guzzle throws an exception on any response code >= 400, and on curl transfer errors.

4xx errors throw a Guzzle\Http\Exception\ClientErrorResponseException
5xx errors throw a Guzzle\Http\Exception\ServerErrorResponseException
cURL errors throw a Guzzle\Http\Exception\CurlException

Does that help?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Yes. What I'm not sure is what's the best way to handle those errors in a generic way so we that can add a useful log message without having to duplicate a lot of code.

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -353,16 +354,18 @@ function aggregator_form_opml_submit($form, &$form_state) {
+      $error = $e->getResponse() ? $e->getResponse()->getReasonPhrase() : $e->getMessage();
+      watchdog('aggregator', 'HTTP request to @url failed with error: @error', array('@url' => $form_state['values']['remote'], '@error' => $error));

That's what I'm currently doing in this patch. getMessage(), especially in the case of Curl exceptions contain a lot debug information that isn't really useful for logging nor to provide as feedback in a UI.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
aggregator-guzzle-1862524-6.patch2.16 KB

Another one simplified a bit.

rbayliss’s picture

rbayliss CreditAttribution: rbayliss commented

This looks good to me. The only semi-annoying thing is that the CurlException message contains a variable export that gets stuck in watchdog, but I guess that's ok for debugging purposes. If it is a problem, we could set up a cascading catch block and call ->getError() on the CurlException, which returns a more readable string.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Yes, we need to come up with a standard, useful way to handle and log guzzle exceptions.

Having two catch blocks is certainly cleaner than my getResponse() hack, although its a bit more code. Maybe something like $error = $e instanceof CurlException ? $e->getError() : $e->getResponse()->getReasonPhrase() ?

Or we could improve guzzle to provide a useful default exception message with getMessage()... that var_export() stuff on getMessage() is kinda weird anyway, never see anyone else doing that?

rbayliss’s picture

rbayliss CreditAttribution: rbayliss commented

Ha. Funny timing. I just posted a possible solution here: #1875792: Standardize Guzzle exception handling

YesCT’s picture

YesCT CreditAttribution: YesCT commented

#6: aggregator-guzzle-1862524-6.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, aggregator-guzzle-1862524-6.patch, failed testing.

Sutharsan’s picture

Sutharsan CreditAttribution: Sutharsan commented
Status: Needs work » Needs review
FileSize
aggregator-guzzle-1862524-12.patch2.17 KB

Patch rerolled.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
aggregator-guzzle-1862524-13.patch1.87 KB

Re-roll, using the same pattern for adding an actually useful error that is displayed to the user. More or less same code as DefaultFetcher. Which had the reference fixed, so no longer part of the patch.

Status: Needs review » Needs work

The last submitted patch, aggregator-guzzle-1862524-13.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

#13: aggregator-guzzle-1862524-13.patch queued for re-testing.

ParisLiakos’s picture

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

+1 for the return addition..no need to trigger _aggregator_parse_opml() if there are no data
rtbc'ing and i hope bot agrees:)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/aggregator/aggregator.admin.inc
@@ -186,12 +188,22 @@ function aggregator_form_opml_submit($form, &$form_state) {
+      watchdog('aggregator', 'Faled to download OPML file due to "%error".', array('%error' => $response->getStatusCode() . ' ' . $response->getReasonPhrase()), WATCHDOG_WARNING);

Typo: Faled -> Failed

Applies to the other watchdog()s and dsm()s as well.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review
FileSize
aggregator-guzzle-1862524-18.patch1.87 KB

oops:)

Status: Needs review » Needs work

The last submitted patch, aggregator-guzzle-1862524-18.patch, failed testing.

ParisLiakos’s picture

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

EntityTranslationUITest.php line 40
random failure

#18: aggregator-guzzle-1862524-18.patch queued for re-testing.

Berdir’s picture

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

I can confirm the typo is fixed ;)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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