Track redirect HTTP status codes by mallardduck · Pull Request #1711 · guzzle/guzzle

mallardduck · 2016-12-31T03:12:39Z

Using a similar technique to the current Redirect History feature this PR adds support for tracking the HTTP status codes in a new: 'X-Guzzle-Redirect-Status-History' header.

This PR was created in response to #1710

sagikazarmark · 2016-12-31T04:25:32Z

                $promise,
-                (string) $nextRequest->getUri()
+                (string) $nextRequest->getUri(),
+                (int) $response->getStatusCode()


Is there any added value in casting it to int? It will become string anyway. If ou just followed the pattern: the URI needs to be casted so that the URI object becomes string.

sagikazarmark · 2016-12-31T04:27:25Z

+                $statusHeader = $response->getHeader(self::STATUS_HISTORY_HEADER);
+                array_unshift($historyHeader, $uri);
+                array_unshift($statusHeader, $statusCode);
+                return $response->withHeader(self::HISTORY_HEADER, $historyHeader)->withHeader(self::STATUS_HISTORY_HEADER, $statusHeader);


Can you split this line to multiple ones?

sagikazarmark · 2016-12-31T04:30:13Z

Okay, you bought me.

Can you please add some tests?

mallardduck · 2016-12-31T04:39:27Z

Sure thing, I can totally add tests!
I'll work up some tests tonight and get an updated PR sometime over the weekend.

...simple enough, this add the requested tests for the new redirect status code history header.

mallardduck · 2016-12-31T21:43:53Z

@sagikazarmark I think this should be good to go as I've addressed your feedback and created tests. Let me know if you'd like any other adjustments to be made first!

sagikazarmark · 2017-01-05T06:28:18Z

                $promise,
-                (string) $nextRequest->getUri()
+                (string) $nextRequest->getUri(),
+                (string) $response->getStatusCode()


I am failing to see why we need to do any casting here. As said getUri returns an object which MUST be casted to string, but the status code is scalar.

Sorry about that, I guess I was a little confused initially. Totally get your point now tho.

Update PR incoming....

sagikazarmark · 2017-01-05T06:30:25Z

I have one last comment, apart from that good to go.

sagikazarmark · 2017-01-09T08:08:07Z

Okay, the code looks good to me, there is only one final thought: Right now we store status code, and next URL pairs, which means the status code belongs to the previous request, which returned the appropriate new location.

Not sure if it can be confusing or we should document it (since the middlewares are not documented, somewhere in the code).

WDYT?

mallardduck · 2017-01-09T16:56:16Z

That's an excellent point!

As you've suggested I think adding comments in the code could be a good option. Alternatively I was thinking this could be added to the FAQ doc?

I'm not sure how common of a question "Why can't I track redirects with Guzzle?" actually is, but maybe this information could fit there? If not that exact question maybe one similar to it?

I also just realized that I should definitely update the request-options file as it contains info on the currently provided header.

I'll update that file and add some comments to the code; I may even include my example of how I track the full redirect path. From there we can evaluate if that's enough information!

mallardduck · 2017-01-09T17:53:31Z

I've updated the relevant request-options section in the documents, please take a look and let me know your thoughts!

I've also included an example in the FAQ file that's useful for users looking to consume both headers as a single item. This can be used to merge HTTP status codes with their respective request URIs.

sagikazarmark

Apart from a few wording comments, perfect 👍

sagikazarmark · 2017-01-11T07:43:45Z

    $client = new GuzzleHttp\Client(['expect' => false]);
+
+How can I track a redirected requests HTTP codes?
+============================================


Should be as long as the title

Didn't realize that, I'll get that updated!

sagikazarmark · 2017-01-11T07:44:40Z

+How can I track a redirected requests HTTP codes?
+============================================
+
+You can enable the `track_redirects` option which will enable Guzzle to track


This first sentence with double "enabled" sounds a little bit weird...

Maybe say "You can enable tracking the redirected URL and the status code via the ... option"

sagikazarmark · 2017-01-11T07:45:09Z

+
+You can enable the `track_redirects` option which will enable Guzzle to track
+any redirected URI and status code. Each redirected URI will be stored in the
+``X-Guzzle-Redirect-History`` header and each Response Code will be stored in


Let's say status code instead of Response Code

sagikazarmark · 2017-01-11T07:47:54Z

+``X-Guzzle-Redirect-History`` header and each Response Code will be stored in
+the ``X-Guzzle-Redirect-Status-History`` header.
+
+The ``X-Guzzle-Redirect-History`` header will exclude the initial request's URI


Instead of the header names we could say "the URL history" and "the status code history". Just to avoid duplication. But I am open to other suggestions.

This sounds good to me and seems to make it easier to read.

sagikazarmark · 2017-01-11T07:48:11Z

+
+The ``X-Guzzle-Redirect-History`` header will exclude the initial request's URI
+and the ``X-Guzzle-Redirect-Status-History`` header will exclude the final
+response code. With this in mind you should easily be able to track a requests


status code

sagikazarmark · 2017-01-11T07:50:52Z

+    $redirectCodeHistory = $response->getHeader('X-Guzzle-Redirect-Status-History'); // retrieve Redirects HTTP Status history
+
+    // Add the initial URI requested to the URI history
+    $reversedUriHistory = array_reverse($redirectUriHistory); // First we reverse the array items


How about using array_unshift instead?

xD yep should have thought about that.

lately I've been using collections a lot and converted that logic into this. (there's no unshift for the collecitons i've been using)

sagikazarmark · 2017-01-11T07:52:22Z

+    // (Optional) Combine the items of each array into a single result set
+    $fullRedirectReport = [];
+    foreach ($redirectUriHistory as $key => $value) {
+        $fullRedirectReport[$key] = ['location' => $value, 'code' => $redirectCodeHistory[$key]];


What should be the first item here? IMO it should be the first URL if any, but currently it's the last.

I guess I'm a little unsure on this to be honest. I read the comments in the middleware about using array_unshift there due to ordering of codes....however when I execute this code I am provided the same order of redirects as I see in curl.

Using the faq code with this patch I am provided with:

[{"location":"https:\/\/grandmascookieblog.com\/cloud.php","code":302},{"location":"http:\/\/www.liquidweb.com\/cloudsites\/","code":301},{"location":"https:\/\/www.liquidweb.com\/cloudsites\/","code":200}]

You can test with this URL: https://grandmascookieblog.com/cloud.php

Via curl I can see:

[ guzzle (track_redirect_codes)]$ curl -IL https://grandmascookieblog.com/cloud.php HTTP/1.1 302 Found Date: Thu, 12 Jan 2017 23:59:52 GMT Server: Apache Location: http://www.liquidweb.com/cloudsites/ Cache-Control: max-age=0 Expires: Thu, 12 Jan 2017 23:59:52 GMT X-Cache: MISS from grandmascookieblog.com Content-Type: text/html; charset=UTF-8 HTTP/1.1 301 Moved Permanently Server: nginx/1.10.2 Date: Thu, 12 Jan 2017 23:59:53 GMT Content-Type: text/html Content-Length: 185 Connection: keep-alive Location: https://www.liquidweb.com/cloudsites/ X-Frame-Options: SAMEORIGIN HTTP/1.1 200 OK Server: nginx/1.10.2 Date: Thu, 12 Jan 2017 23:59:53 GMT Content-Type: text/html; charset=ISO-8859-1 Content-Length: 38913 Connection: keep-alive Set-Cookie: production_www_sessid=1223adaee9e822dff41d9be79dd793a2; path=/ X-ua-compatible: IE=edge,chrome=1 X-Frame-Options: SAMEORIGIN

Hm, looking at the code again, it should be fine now.

sagikazarmark · 2017-01-11T07:52:46Z

- track_redirects: (bool) When set to ``true``, each redirected URI encountered
-  will be tracked in the ``X-Guzzle-Redirect-History`` header in the order in
-  which the redirects were encountered.
+- track_redirects: (bool) When set to ``true``, each redirected URI and Response


status code

sagikazarmark · 2017-01-11T07:53:14Z

+- track_redirects: (bool) When set to ``true``, each redirected URI and Response
+  Code encountered will be tracked in the ``X-Guzzle-Redirect-History`` and 
+  ``X-Guzzle-Redirect-Status-History`` headers respectively. All URIs and
+  Response Codes will be stored in the order in which the redirects were


status code

sagikazarmark · 2017-01-11T07:53:22Z

+
+  Note: When tracking redirects the ``X-Guzzle-Redirect-History`` header will
+  exclude the initial request's URI and the ``X-Guzzle-Redirect-Status-History``
+  header will exclude the final response code.


status code

Corrects some minor grammar issues.

sagikazarmark

Can you please also check the status code-response code thing?

sagikazarmark · 2017-01-13T08:38:21Z

+    $redirectCodeHistory = $response->getHeader('X-Guzzle-Redirect-Status-History'); // retrieve Redirects HTTP Status history
+
+    // Add the initial URI requested to the (beginning of) URI history
+    array_unshift($reversedUriHistory, $initialRequest); // then we add the initial URL on the end


Is $reversedUriHistory correct here?

sagikazarmark · 2017-01-13T08:38:44Z

+    // (Optional) Combine the items of each array into a single result set
+    $fullRedirectReport = [];
+    foreach ($redirectUriHistory as $key => $value) {
+        $fullRedirectReport[$key] = ['location' => $value, 'code' => $redirectCodeHistory[$key]];


Hm, looking at the code again, it should be fine now.

sagikazarmark · 2017-01-13T20:34:53Z

@mallardduck I would like to release this in 6.3 in a few weeks. Can you check the remaining comments? Thanks

mallardduck · 2017-01-13T22:58:46Z

@sagikazarmark absolutely! I was hoping to get this done today at work however I ran out of time. I'll work on this over the weekend and shoot an updated PR your way.

I really appreciate the followup on this and wanted to let you know it's been awesome to collaborate and give back.

sagikazarmark · 2017-01-15T09:46:00Z

@mallardduck thanks, I think this is ready. I changed some small wordings, also changed the FAQ example (removed some request options) to make it simpler. I think those weren't important for this feature anyway. If they were, please let me know, so we can put them back.

mallardduck added 2 commits December 30, 2016 21:24

Track redirect status codes

03054a9

..oh I see now. Status Code headers working!

e824f34

sagikazarmark requested changes Dec 31, 2016

View reviewed changes

mallardduck added 2 commits December 31, 2016 03:43

Add tests for new status code header

317ff3b

...simple enough, this add the requested tests for the new redirect status code history header.

Updates per feedback

37ff74a

sagikazarmark requested changes Jan 5, 2017

View reviewed changes

Don't cast the status code at all.

6922eb6

Update docs for redirect code tracking PR

67c0262

sagikazarmark requested changes Jan 11, 2017

View reviewed changes

Minor copy edit for docs

305fe36

Corrects some minor grammar issues.

sagikazarmark requested changes Jan 13, 2017

View reviewed changes

sagikazarmark modified the milestone: 6.3.0 Jan 13, 2017

mallardduck and others added 2 commits January 14, 2017 20:13

Adjust documentation for new header

b8580db

Change some wordings, simplify FAQ example

4a801af

sagikazarmark approved these changes Jan 15, 2017

View reviewed changes

sagikazarmark merged commit 77a16c8 into guzzle:master Jan 15, 2017

thinsoldier pushed a commit to thinsoldier/guzzle that referenced this pull request Mar 2, 2017

Track redirect HTTP status codes (guzzle#1711)

0f3597c

Uh oh!

Conversation

mallardduck commented Dec 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Dec 31, 2016

Uh oh!

mallardduck commented Dec 31, 2016

Uh oh!

mallardduck commented Dec 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Jan 5, 2017

Uh oh!

sagikazarmark commented Jan 9, 2017

Uh oh!

mallardduck commented Jan 9, 2017

Uh oh!

mallardduck commented Jan 9, 2017

Uh oh!

sagikazarmark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Jan 13, 2017

Uh oh!

mallardduck commented Jan 13, 2017

Uh oh!

sagikazarmark commented Jan 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development