I've set up the following redirect chain.

scratch-redirect-1 -> 302 to 
scratch-redirect-2 -> 302 to
scratch-redirect-3 -> 302 to
scratch-redirect-final

Fetches below with max_redirect => 10. In all cases, scratch-redirect-final is the final URL that has its content fetched.

Fetching scratch-redirect-3 I get the following abbreviated response object:

[code] => 200
[redirect_code] => 302
[redirect_url] => http://s/core7/scratch-redirect-final

Fetching scratch-redirect-2 I get the following abbreviated response object:

[code] => 200
[redirect_code] => 302
[redirect_url] => http://s/core7/scratch-redirect-3

Fetching scratch-redirect-1 I get the following abbreviated response object:

[code] => 200
[redirect_code] => 302
[redirect_url] => http://s/core7/scratch-redirect-2

API docs claim:

- redirect_code: If redirected, an integer containing the initial response status code.
- redirect_url: If redirected, a string containing the redirection location.

While I'm not sure what "redirection location" actually means, for 1 step redirects it amounts to the TARGET.

Attached patch changes redirect_url to mean the final url.

CommentFileSizeAuthor
#14 1081068-14-drupal_http_request_inconsistent_redirect_url.patch4.29 KBwojtha
#12 1081068-12-drupal_http_request_inconsistent_redirect_url.patch3.67 KBwojtha
#10 1081068-10-drupal_http_request_inconsistent_redirect_url.patch3.67 KBwojtha
#9 1081068-9-drupal_http_request_inconsistent_redirect_url_D8.patch3.67 KBwojtha
#7 1081068-7-drupal_http_request_inconsistent_redirect_url.patch3.67 KBwojtha
#3 1081068-3-drupal_http_request_inconsistent_redirect_url.patch1.45 KBbfroehle
do_drupal_http_request_redirect_url.patch1.09 KBheine

Comments

heine’s picture

heine commented
Status: Active » Needs review
heine’s picture

heine commented
Priority: Normal » Major

Marking major as it blocks #575810: OpenID discovery spec violation - follow redirects..

bfroehle’s picture

bfroehle commented
StatusFileSize
new 1081068-3-drupal_http_request_inconsistent_redirect_url.patch1.45 KB

I've attached a revised patch which breaks the comment at 80 characters. The patch itself is proper and seems ready to me.

wojtha’s picture

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

RTBC
It needs to be backported to D6 too.

heine’s picture

heine commented
Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review

*sigh*

heine’s picture

heine commented
Status: Needs review » Needs work

Probably needs test. Cannot do so atm.

wojtha’s picture

wojtha commented
Status: Needs review » Needs work
StatusFileSize
new 1081068-7-drupal_http_request_inconsistent_redirect_url.patch3.67 KB

Rerolled patch from #3 by Heine and bfroehle. I added the tests for single and for multiple redirects.

wojtha’s picture

wojtha commented
Status: Needs work » Needs review
wojtha’s picture

wojtha commented
Status: Needs work » Needs review
StatusFileSize
new 1081068-9-drupal_http_request_inconsistent_redirect_url_D8.patch3.67 KB

Just changing redirect code from 302 to 301 in the testing function. This code is more appropriate I think.

wojtha’s picture

wojtha commented
StatusFileSize
new 1081068-10-drupal_http_request_inconsistent_redirect_url.patch3.67 KB
dries’s picture

dries commented
Status: Needs review » Needs work 
+++ b/includes/common.inc
@@ -753,7 +753,8 @@ function drupal_access_denied() {
+ *   - redirect_url: If redirected, a string containing the final redirection
+ *     location.

It was not really clear what 'final' meant. I had to read up on the example in the issue to understand what it meant. This is minor. 

+++ b/includes/common.inc
@@ -1007,7 +1008,7 @@ function drupal_http_request($url, array $options = array()) {
+      $result->redirect_url = isset($result->redirect_url) ? $result->redirect_url : $location;

This is written a bit 'wasteful'. If $result->redirect_url is already set, we don't have to overwrite it with itself. We can use a regular if-statement.

wojtha’s picture

wojtha commented
Status: Needs work » Needs review
StatusFileSize
new 1081068-12-drupal_http_request_inconsistent_redirect_url.patch3.67 KB

Reflecting Dries's notices in #11. I changed redirect_url description to:
redirect_url: If redirected, a string containing the URL of the redirect target
... makes more sense now?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +Needs backport to D7 
+++ b/modules/simpletest/tests/system_test.module
@@ -122,6 +129,15 @@ function system_test_redirect($code) {
+function system_test_multiple_redirects($count) {

Can we quickly add a phpDoc here?

Powered by Dreditor.

wojtha’s picture

wojtha commented
StatusFileSize
new 1081068-14-drupal_http_request_inconsistent_redirect_url.patch4.29 KB

@sun sure

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Looks like Dries's feedback has been addressed.

Corrected some minor English things (continuos, destinatination) and committed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

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

Issue tags: +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.