r94881 MediaWiki - Code Review archive

http://borntoolatevintage.com/ is a real life example, it has 2 redirects:

a) first: http://www.artfire.com/
b) second: /ext/shop/studio/BornTooLateVintage

so final URL is http://www.artfire.com/ext/shop/studio/BornTooLateVintage

This code uses parse_url(), which is subtly broken in a few ways and throws PHP warnings for invalid input. You might be better off using wfParseUrl() instead, see its documentation in GlobalFunctions.php

Good to know. However, one function above (parseCookies) uses getFinalUrl and parse_url so this function was already used in the flow.

Does this have or need unit tests?

http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/tests/phpunit/includes/HttpTest.php Can't see related unit tests but perhaps it's a good idea to create them.

Catrope: using wfParseUrl rise a notice: "Undefined index: scheme" when passed URL has only relative part of the path (eg. "/some/folder/foo") so I'll stick with parse_url() - "Location" should not have protocols like "news:" so it should be safe to leave PHP native function.

Looks good to me. PHP Tests added by r106948.

This feels wrong; inline documentation seems to indicate that $this->respHeaders in a MWHttpRequest will hold response headers from a single response, using headers in case some values exist multiple times.

After following a redirect then, we should only have one 'Location' header from the latest request. Multiple Location headers are not allowed as far as I know, so looping over the single Location header shouldn't give you much information.

Is something merging the returned headers from every followed request when doing redirects or something? This could end up giving some weird bogus results in general...

s/using headers/using sub-arrays/

I can't remember the site causing troubles but I've fixed that functions because of a real case. I got multiple Location headers from getResponseHeaders() and some of them were relative.

Found it - the site that at that time caused multiple redirections was: http://borntoolatevintage.com/

I can see it still does 2 redirects http://borntoolatevintage.com/ => http://www.artfire.com/users/BornTooLateVintage => http://www.artfire.com/ext/shop/studio/BornTooLateVintage though haven't checked if the latter is still relative or was changed into absolute.

By setting CURLOPT_HEADERFUNCTION we can pass any header to a callback:

       $this->curlOptions[CURLOPT_HEADERFUNCTION] = array( $this, "readHeader" );

The callback just append received header one after the other in a huge string. It is then exploded by \r\n to an array.

   protected function readHeader( $fh, $content ) {
       $this->headerText .= $content;
                        ^^^^
       return strlen( $content );
   }

So internally, the Location: header content is really an array of any Location: lines curl might have encountered while following redirections.

I have setup a testing env on my computer using existing virtual hosts and just creating dummy PHP redirection files.

http://trunk.dev/redir.php redirects to http://mw18.dev/redir.php http://mw18.dev/redir.php redirects to /landing.php

h = MWHttpRequest::factory( 'http://trunk.dev/redir.php', array( 'followRedirects'=>true) ); $h->execute(); print $h->getFinalUrl();

The header representation for 'location' is:

   [location] => Array
       (
           [0] => http://mw18.dev/redir.php
           [1] => /landing.php
       )

So previously we would receive /landing.php as a Location and one could assume it is relative to the requested URL host (trunk.dev) whereas it is relative to the host of the previous URL having a host.

Ugh, that looks like a bug/misfeature of the CURL stuff!

bug 29232 (rewriting the redirect handling at a higher level) would avoid this. I'll add a comment about the case that this rev is for...

CURL just give us whatever new headers he received after following a location and pass that to our callback. I guess the issue is in our callback that should not just blindly append the headers but should update them instead. i.e. we probably have to rewrite all that stuff to better support Location :-)

"we probably have to rewrite all that stuff to better support Location" => sounds like my commit was a part of that "rewrite" process ;)

Yes totally :-) I just think it should be made sooner in the workflow though, but your code is fine nonetheless. Thanks!

Added an explanatory comment in r107121 -- the multiple headers are a CURL artifact.