The bug with 'http://www.apple.com/qtactivex/qtplugin.cab' is not fixed at all. My debugging shows that it tries several redirects and then it stops processing with 'maximum allowed redirects exhausted'.

This is incorrect behavior. As you can see in the options array I have defined 'max_redirects' => 1. This must cause httprl to STOP processing after the first redirect and give me the first returned redirect code in the responds array. It's not allowed to follow more redirects and it is not correct to tell me 'maximum allowed redirects exhausted'. Compare to core, please.

CommentFileSizeAuthor
#4 httprl-1429284-4-fix-missing-redirect-ulr-when-max-redirects-are-reached.patch1.64 KBmikeytown2

Comments

hass’s picture

hass commented
Priority: Critical » Normal

Uuhh, what is this... maximum allowed redirects is "exhausted" is logic wise logged because of the 'max_redirects' => 1 has been exhausted. This may be highly confusing for users as nothing is wrong, but it sounds like something is wrong and it breaks existing logics in linkchecker as this already require a 200 OK in top level array next to redirect_code. I could add this -2 to linkchecker... or we make httprl consistent with core. I'm for consistency and losing the information, that there are more redirects...

hass’s picture

hass commented
Title: HTTPRL follows redirects until maximum allowed redirects are exhausted, but is configured to follow only *one* redirect » "-2 maximum allowed redirects are exhausted" logged, inconsistent with core
hass’s picture

hass commented
Title: "-2 maximum allowed redirects are exhausted" logged, inconsistent with core » "2 maximum allowed redirects are exhausted" logged, inconsistent with core

it's positive, 2

mikeytown2’s picture

mikeytown2 commented
StatusFileSize
new httprl-1429284-4-fix-missing-redirect-ulr-when-max-redirects-are-reached.patch1.64 KB

As for the error code see #1320222-6: Bring in other drupal patches & #1096890: drupal_http_request should return error if reaches max allowed redirects. Your UTF-8 fix (#371495: Error message from drupal_http_request() not UTF8 encoded) is in one of these issues that never get any attention; I've tried to included patches that seem like a good idea.

You are referencing #1420104-3: Logic is broken with redirects, correct? I see the error. The redirect array doesn't have the 2nd location included. The following patch has been committed.

mikeytown2’s picture

mikeytown2 commented
Status: Active » Fixed

As for the debug code, I've checked and only 2 requests total go through the event loop when max_redirects are set to 1. (debug after the timer has been stopped)

http://www.apple.com/qtactivex/qtplugin.cab
http://qtinstall.apple.com/qtactivex/qtplugin.cab

If you placed the debug code inside of the event loop (while (!empty($streams)) { ... }) you would see a lot of action in there.

Marking this issue as fixed. Let me know if I'm wrong :)

hass’s picture

hass commented

I will test this later. #1320222: Bring in other drupal patches is a very good fix list... It's really bad that this is not going to get fixed in core.

hass’s picture

hass commented
Status: Fixed » Needs work

Ups, sorry for confusing things... the patch you committed is wrong. It should only list *one* redirect as I'm saying max_redirect = 1. Now it contains two redirects - what is invalid. My issue here was more about your custom status code = 2 that requires some extra stuff in linkchecker (and is inconsistent with core), but I'm with you - that we should have a status code for this. Let's try to get the patch #1096890: drupal_http_request should return error if reaches max allowed redirects into core. When I opened the case I expected this status code only after 3 redirects and after more thinking I understood what you are doing there. But I have expected a "200 OK", not a "2 maximum redirects exhausted". No worries I can workaround.

Let's roll back this patch as it's break things.

mikeytown2’s picture

mikeytown2 commented

actually it does fix an important thing.

Initial URL: www.apple.com/qtactivex/qtplugin.cab

With the patch
max_redirects = 0

// HTTPRL
    [redirect_code] => Array (
            [0] => 302
        )
    [redirect_url] => Array (
            [0] => qtinstall.apple.com/qtactivex/qtplugin.cab
        )
// CORE
    [code] => 302
    [redirect_url] => qtinstall.apple.com/qtactivex/qtplugin.cab

max_redirects = 1

// HTTPRL
    [redirect_code] => Array (
            [0] => 302
            [1] => 302
        )
    [redirect_url] => Array (
            [0] => qtinstall.apple.com/qtactivex/qtplugin.cab
            [1] => appldnld.apple.com.edgesuite.net/content.info.apple.com/QuickTime/qtactivex/qtplugin.cab
        )
// CORE
    [redirect_url] => appldnld.apple.com.edgesuite.net/content.info.apple.com/QuickTime/qtactivex/qtplugin.cab
    [redirect_code] => 302

With out the patch
max_redirects = 0

// HTTPRL
    [redirect_code] => Array (
            [0] => 302
        )
    [redirect_url] => Array (
            [0] => qtinstall.apple.com/qtactivex/qtplugin.cab
        )
// CORE
    [code] => 302
    [redirect_url] => qtinstall.apple.com/qtactivex/qtplugin.cab

max_redirects = 1

// HTTPRL
    [redirect_code] => Array (
            [0] => 302
        )
    [redirect_url] => Array (
            [0] => qtinstall.apple.com/qtactivex/qtplugin.cab
        )
// CORE
    [redirect_url] => appldnld.apple.com.edgesuite.net/content.info.apple.com/QuickTime/qtactivex/qtplugin.cab
    [redirect_code] => 302
mikeytown2’s picture

mikeytown2 commented
Status: Needs work » Fixed
hass’s picture

hass commented

Thanks for finding a bug in linkchecker :-)))

Status: Fixed » Closed (fixed)

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