Convert drupal_http_request usage in locale.batch.inc to Guzzle

There seems to be a getLocation() method on the Response object, does this return that information?

This feature has been removed in the aggregator implementation, but I actually think that this is incorrect, as I've written in #1447736-187: Adopt Guzzle library to replace drupal_http_request().

Ah right. Because the final response *is* a 200 OK and has no Location header, obviously.

I think that's what getPreviousResponse() is for, you could call that and check if it is a redirect/has a Location.

#6: locale-guzzle-1875614-6.patch queued for re-testing.

+++ b/core/modules/locale/locale.batch.incundefined
@@ -433,39 +436,49 @@ function locale_translation_batch_fetch_finished($success, $results) {
+  catch (CurlException $e) {
+    // Handle connection problems and cURL specific errors.
+    watchdog('locale', 'HTTP request to @url failed with error: @error.', array('@url' => $uri, '@error' => $e->getMessage()));
...
+  catch (RequestException $e) {
+    // Handle other http related errors.
+    watchdog('locale', 'HTTP request to @url failed with error: @error.', array('@url' => $uri, '@error' => $e->getMessage()));

CurlException is a childclass of RequestException, so you don't need the CurlException block unless you actually want to do something different with it.

Otherwise, this is a nice example of when you actually have a case for handling those exceptions differently.

+++ b/core/modules/locale/locale.batch.incundefined
@@ -5,6 +5,9 @@
+use Guzzle\Http\Exception\BadResponseException;
+use Guzzle\Http\Exception\CurlException;

Should have a use for RequestException instead now.

+++ b/core/modules/locale/locale.batch.incundefined
@@ -433,39 +436,46 @@ function locale_translation_batch_fetch_finished($success, $results) {
+  $request = drupal_container()->get('http_default_client')->head($uri);

This should now use Drupal::httpClient().

+++ b/core/modules/locale/locale.batch.incundefined
@@ -433,39 +436,46 @@ function locale_translation_batch_fetch_finished($success, $results) {
  * @param string $uri
  *   URI of remote file.
- * @param array $headers
- *   HTTP request headers.
- * @return stdClass
- *   Result object containing the HTTP request headers, response code, headers,
- *   data, redirect status and updated timestamp.
+ * @return array
+ *   Array containing the last modified timestamp of the translation file.
  *   TRUE if the file is not found.
  *   FALSE if a fault occured.

looks like the true false lines are inaccurate now.

(also missing newline before @return)

I tried to address #15
and looked at the sample code in other guzzle conversions and the change notice:
http://drupal.org/node/1862446

But I'm still not sure.

Also, I didn't know if, like the change notice sample, if the setup should happen before the try {} and then in the try, do the ->send.

+++ b/core/modules/locale/locale.batch.incundefined
@@ -436,16 +436,17 @@ function locale_translation_batch_fetch_finished($success, $results) {
-    $response = $request->send();
+    $response = Drupal::httpClient()
+      ->get('http_default_client')
+      ->head($uri)

get() was a method on the drupal_container(), this just needs Drupal::httpClient()->head()->send().

This should work.

That's how we did in all other places where we logged the reason. It's an untranslated string anyway (e.g. "403 Access Denied") so LTR/RTL shouldn't really matter?

I don't see how getReasonPhrase could contain the body? getMessage() doesn't do anything with the reason phrase and if you look at getRawHeaders(), it's done in a very similar way there.

+++ b/core/modules/locale/locale.batch.incundefined
@@ -433,39 +436,46 @@ function locale_translation_batch_fetch_finished($success, $results) {
- *   TRUE if the file is not found.
- *   FALSE if a fault occured.

why are we removing those from docblock? i see the behavior is the same after the patch

+++ b/core/modules/locale/locale.batch.incundefined
@@ -433,39 +436,46 @@ function locale_translation_batch_fetch_finished($success, $results) {
+ *   Array containing the last modified timestamp of the translation file.

maybe add: and the file location if a redirect occurs

+++ b/core/modules/locale/locale.batch.incundefined
@@ -433,39 +436,46 @@ function locale_translation_batch_fetch_finished($success, $results) {
+    watchdog('locale', 'HTTP request to @url failed with error: @error.', array('@url' => $uri, '@error' => $response->getStatusCode() . ' ' . $response->getReasonPhrase()));

Sorry but I still don't get what you mean.

The status code and reason phrase *is* a t() argument that is injected into a placeholder.

So a RTL language would display it as "403 Access denied rorre htiw deliaf lru@ ot tseuqer PTTH" (Yes, I actually reversed the english string ;)), if the placeholder is on the left side. So the only thing we could do is split it up and have to placeholders as @errorcode @errormessage so that they could reverse that too. But given that were talking about a string that is very likely in English, I'm not sure that makes sense? If you think it does, feel free to change :)

@Sutharsan: Great, can you explain what's the deal with those @return documentation changes pointed out in #22? Once that's updated then we can get this in, this is one of two remaining conversion issues now and the last that's not RTBC :)

looks good to go now, thanks:)

All right, let's get these out of the way so we can close a major!

Committed and pushed to 8.x. Thanks!