Support from Acquia helps fund testing for Drupal Acquia logo

Comments

krishworks’s picture

Status: Active » Needs review
FileSize
1.61 KB

first attempt at changing. not sure if everything is correct. followed the method described here: http://drupal.org/node/1862446

Status: Needs review » Needs work

The last submitted patch, system.module-guzzle-change.patch, failed testing.

Berdir’s picture

The exception handling needs to catch the RequestException as well. See #1875792: Standardize Guzzle exception handling

krishworks’s picture

Thanks Berdir for the helpful link. Here is another patch handling RequestException as well.

krishworks’s picture

Status: Needs work » Needs review
FileSize
1.81 KB

the last patch was partial. Here is a complete patch.

Status: Needs review » Needs work

The last submitted patch, guzzle.patch, failed testing.

Berdir’s picture

You are missing the use statements for these two exception classes, now they don't match and the exception bubbles up.

krishworks’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

added appropriate 'use' statement for Exception handling.

Status: Needs review » Needs work

The last submitted patch, guzzle.patch, failed testing.

Berdir’s picture

+++ b/core/modules/system/system.moduleundefined
@@ -3700,12 +3702,20 @@ function system_retrieve_file($url, $destination = NULL, $managed = FALSE, $repl
+    drupal_set_message(t('HTTP error @errorcode occurred when trying to fetch @remote.', array('@errorcode' => $response->getStatusCode(), '@remote' => $url)), 'error');

You need to call $e->getResponse() to get the response.

+++ b/core/modules/system/system.moduleundefined
@@ -3700,12 +3702,20 @@ function system_retrieve_file($url, $destination = NULL, $managed = FALSE, $repl
+    drupal_set_message(t('HTTP error @errorcode occurred when trying to fetch @remote.', array('@errorcode' => $response->getStatusCode(), '@remote' => $url)), 'error');

And this does not have a response nor http error status. You can use $e->getMessage() to get the generic curl error message, see the linked issue.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.17 KB

Here's a re-roll. Again using similar pattern for the error message as in aggregator DefaultFetcher

ParisLiakos’s picture

+++ b/core/modules/system/system.moduleundefined
@@ -3667,7 +3669,7 @@ function theme_system_compact_link() {
- * Attempts to get a file using drupal_http_request and to store it locally.
+ * Attempts to get a file using Guzzle HTTP client and to store it locally.

maybe instead of mentioning Guzzle would be better to say http client service?

ParisLiakos’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.moduleundefined
@@ -3708,12 +3710,22 @@ function system_retrieve_file($url, $destination = NULL, $managed = FALSE, $repl
-    drupal_set_message(t('HTTP error @errorcode occurred when trying to fetch @remote.', array('@errorcode' => $result->code, '@remote' => $url)), 'error');
...
+    drupal_set_message(t('HTTP error "@error" occurred when trying to fetch @remote.', array('@errorcode' => $response->getStatusCode() . ' ' . $response->getReasonPhrase(), '@remote' => $url)), 'error');
...
+    drupal_set_message(t('HTTP error "@error" occurred when trying to fetch @remote.', array('@errorcode' => $exception->getMessage(), '@remote' => $url)), 'error');

You either need to change the message or fix the placeholders..i have no clue though how to get status code from Request Exception

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
2.09 KB

Better?

As discussed, guzzle it is, it's documented in @return now and without a generic interface, not replacable. And I prefer that over a generic service name.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/system/system.moduleundefined
@@ -3708,12 +3710,22 @@ function system_retrieve_file($url, $destination = NULL, $managed = FALSE, $repl
+    drupal_set_message(t('Failed to fetch file due to HTTP error "%error"', array('%error' => $response->getStatusCode() . ' ' . $response->getReasonPhrase())), 'error');
...
+    drupal_set_message(t('Failed to fetch file due to error "%error"', array('%error' => $exception->getMessage())), 'error');

just for future reviewers, "HTTP" word is left on purpose on the first message cause it will form something like:
Failed to fetch file due to HTTP error "404 not found"

which makes perfect sense:)
RTBC

catch’s picture

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.