I think that drupal_http_request should return an error if it spents max allowed redirects without reaching the target.

CommentFileSizeAuthor
#24 1096890-24_max_allowed_redirects_error.patch2.92 KBmgifford
#20 1096890-19_max_allowed_redirects_error.patch2.26 KBwojtha
#18 1096890-18_max_allowed_redirects_error.patch2.32 KBwojtha
#15 1096890-15_max_allowed_redirects_error.patch2.2 KBwojtha
#9 1096890-9_max_allowed_redirects_error.patch2.38 KBxjm
#4 1096890-2_max_allowed_redirects_error.patch3.45 KBwojtha
#3 1096890-2_max_allowed_redirects_error_D8.patch3.45 KBwojtha
#2 1096890-2_max_allowed_redirects_error_D8.patch3.45 KBwojtha
#1 max_allowed_redirects_error_1096890.patch1.07 KBwojtha
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wojtha’s picture

wojtha CreditAttribution: wojtha commented
Status: Active » Needs review
FileSize
max_allowed_redirects_error_1096890.patch1.07 KB
wojtha’s picture

wojtha CreditAttribution: wojtha commented
Version: 7.x-dev » 8.x-dev
Assigned: Unassigned » wojtha
FileSize
1096890-2_max_allowed_redirects_error_D8.patch3.45 KB

Moving to D8 + adding test.

wojtha’s picture

wojtha CreditAttribution: wojtha commented
FileSize
1096890-2_max_allowed_redirects_error_D8.patch3.45 KB

Rerolling, patch in #2 has still Status: ignored.

wojtha’s picture

wojtha CreditAttribution: wojtha commented
FileSize
1096890-2_max_allowed_redirects_error.patch3.45 KB

Ahh, last two patches were ignored because of the "D8" at the end of the name. Strange.

Dries’s picture

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

This looks reasonable. I think this is RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +Needs backport to D7

Looks backportable as well, remove tag if you disagree. Did not properly review patch.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: -Needs backport to D7

#4: 1096890-2_max_allowed_redirects_error.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 1096890-2_max_allowed_redirects_error.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Reviewed & tested by the community
FileSize
1096890-9_max_allowed_redirects_error.patch2.38 KB

Rerolled against current D8 codebase; should still be RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1096890-9_max_allowed_redirects_error.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

The patch error is:

Output: [PHP Fatal error: Cannot redeclare system_test_multiple_redirects() (previously declared in ./modules/simpletest/tests/system_test.module:154) in ./modules/simpletest/tests/system_test.module on line 170

Looks like part of this already went in?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

I realize #9 is also missing a hunk. I'll take a closer look at #4 against current codebase.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Looks like system_test_multiple_redirects() got added in #1081068: drupal_http_request inconsistent redirect_url (thank you git blame!). Maybe @wojtha can clarify the overlap since he worked on both patches. :) Otherwise, maybe we just need to rewrite the tests based on #1081068: drupal_http_request inconsistent redirect_url.

wojtha’s picture

wojtha CreditAttribution: wojtha commented

@xjm Yeah, that function - the menu callback - is an overlap. But it is needed by both patches which are separate issues.

wojtha’s picture

wojtha CreditAttribution: wojtha commented
Assigned: wojtha » Unassigned
Status: Needs work » Needs review
FileSize
1096890-15_max_allowed_redirects_error.patch2.2 KB

Callback system_test_multiple_redirects() stripped from the patch since it got in already in #1081068: drupal_http_request inconsistent redirect_url.

mikeytown2’s picture

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

error codes should be negative.

hass’s picture

hass CreditAttribution: hass commented

In httprl, too... :-)

wojtha’s picture

wojtha CreditAttribution: wojtha commented
Status: Needs work » Needs review
FileSize
1096890-18_max_allowed_redirects_error.patch2.32 KB

@mikeytown2: fixed

@hass

In httprl, too... :-)

I dont understand that comment.

Status: Needs review » Needs work

The last submitted patch, 1096890-18_max_allowed_redirects_error.patch, failed testing.

wojtha’s picture

wojtha CreditAttribution: wojtha commented
Status: Needs work » Needs review
FileSize
1096890-19_max_allowed_redirects_error.patch2.26 KB

Error code constant HTTP_REQUEST_ALLOWED_REDIRECTS_EXHAUSTED shortened to HTTP_REQUEST_REDIRECTS_EXHAUSTED

wojtha’s picture

wojtha CreditAttribution: wojtha commented

Ahh I see what do you mean with httprl ... #1429284: "2 maximum allowed redirects are exhausted" logged, inconsistent with core.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

So, drupal_http_request() is on it's way out, has anyone already tried how Guzzle re-acts to this?

This is marked as needs backport, so I guess it can just be moved down to 7.x...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Version: 8.x-dev » 7.x-dev

Ok, Guzzle throws a Guzzle\Http\Exception\TooManyRedirectsException. Nice. Moving back to 7.x.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Issue summary: View changes
FileSize
1096890-24_max_allowed_redirects_error.patch2.92 KB

reroll of 1096890-15_max_allowed_redirects_error.patch