HttpFunctions: Increase code coverage

* Complete coverage for Http::getProxy().
* Remove bogus @covers tag on data provider, and add the
  relevant MWHttpRequest::getFinalUrl to the test instead.
* Convert test to use dataProvider and add missing test cases
  to increase getFinalUrl() test coverage to 100%.
* Minor clean up in getFinalUrl to consistently use early-return
  for all cases, not just for relative 'domain' and 'isset-host'
  cases. Without this coverage actually couldn't reach 100% due
  to the remainder of the empty else branch never being reached
  (CRAP: "Redundant 'else' after 'return'")

Change-Id: I775d95965dc23a1e6c4c62ed84f9da64b6c72135

diff --git a/includes/http/MWHttpRequest.php b/includes/http/MWHttpRequest.php
index 8d58ce5..88cc510 100644 (file)
--- a/includes/http/MWHttpRequest.php
+++ b/includes/http/MWHttpRequest.php
@@ -609,19 +609,17 @@ class MWHttpRequest implements LoggerAwareInterface {
                                }
                        }
 
-                       if ( $foundRelativeURI ) {
-                               if ( $domain ) {
-                                       return $domain . $locations[$countLocations - 1];
-                               } else {
-                                       $url = parse_url( $this->url );
-                                       if ( isset( $url['host'] ) ) {
-                                               return $url['scheme'] . '://' . $url['host'] .
-                                                       $locations[$countLocations - 1];
-                                       }
-                               }
-                       } else {
+                       if ( !$foundRelativeURI ) {
                                return $locations[$countLocations - 1];
                        }
+                       if ( $domain ) {
+                               return $domain . $locations[$countLocations - 1];
+                       }
+                       $url = parse_url( $this->url );
+                       if ( isset( $url['host'] ) ) {
+                               return $url['scheme'] . '://' . $url['host'] .
+                                       $locations[$countLocations - 1];
+                       }
                }
 
                return $this->url;

diff --git a/tests/phpunit/includes/http/HttpTest.php b/tests/phpunit/includes/http/HttpTest.php
index 036baa8..3693a27 100644 (file)
--- a/tests/phpunit/includes/http/HttpTest.php
+++ b/tests/phpunit/includes/http/HttpTest.php
@@ -66,6 +66,13 @@ class HttpTest extends MediaWikiTestCase {
         * @covers Http::getProxy
         */
        public function testGetProxy() {
+               $this->setMwGlobals( 'wgHTTPProxy', false );
+               $this->assertEquals(
+                       '',
+                       Http::getProxy(),
+                       'default setting'
+               );
+
                $this->setMwGlobals( 'wgHTTPProxy', 'proxy.domain.tld' );
                $this->assertEquals(
                        'proxy.domain.tld',
@@ -140,50 +147,56 @@ class HttpTest extends MediaWikiTestCase {
                ];
        }
 
+       public static function provideRelativeRedirects() {
+               return [
+                       [
+                               'location' => [ 'http://newsite/file.ext', '/newfile.ext' ],
+                               'final' => 'http://newsite/newfile.ext',
+                               'Relative file path Location: interpreted as full URL'
+                       ],
+                       [
+                               'location' => [ 'https://oldsite/file.ext' ],
+                               'final' => 'https://oldsite/file.ext',
+                               'Location to the HTTPS version of the site'
+                       ],
+                       [
+                               'location' => [
+                                       '/anotherfile.ext',
+                                       'http://anotherfile/hoster.ext',
+                                       'https://anotherfile/hoster.ext'
+                               ],
+                               'final' => 'https://anotherfile/hoster.ext',
+                               'Relative file path Location: should keep the latest host and scheme!'
+                       ],
+                       [
+                               'location' => [ '/anotherfile.ext' ],
+                               'final' => 'http://oldsite/anotherfile.ext',
+                               'Relative Location without domain '
+                       ],
+                       [
+                               'location' => null,
+                               'final' => 'http://oldsite/file.ext',
+                               'No Location (no redirect) '
+                       ],
+               ];
+       }
+
        /**
         * Warning:
         *
         * These tests are for code that makes use of an artifact of how CURL
         * handles header reporting on redirect pages, and will need to be
-        * rewritten when T31232 is taken care of (high-level handling of
-        * HTTP redirects).
+        * rewritten when T31232 is taken care of (high-level handling of HTTP redirects).
+        *
+        * @dataProvider provideRelativeRedirects
+        * @covers MWHttpRequest::getFinalUrl
         */
-       public function testRelativeRedirections() {
+       public function testRelativeRedirections( $location, $final, $message = null ) {
                $h = MWHttpRequestTester::factory( 'http://oldsite/file.ext', [], __METHOD__ );
-
-               # Forge a Location header
-               $h->setRespHeaders( 'location', [
-                               'http://newsite/file.ext',
-                               '/newfile.ext',
-                       ]
-               );
-               # Verify we correctly fix the Location
-               $this->assertEquals(
-                       'http://newsite/newfile.ext',
-                       $h->getFinalUrl(),
-                       "Relative file path Location: interpreted as full URL"
-               );
-
-               $h->setRespHeaders( 'location', [
-                               'https://oldsite/file.ext'
-                       ]
-               );
-               $this->assertEquals(
-                       'https://oldsite/file.ext',
-                       $h->getFinalUrl(),
-                       "Location to the HTTPS version of the site"
-               );
-
-               $h->setRespHeaders( 'location', [
-                               '/anotherfile.ext',
-                               'http://anotherfile/hoster.ext',
-                               'https://anotherfile/hoster.ext'
-                       ]
-               );
-               $this->assertEquals(
-                       'https://anotherfile/hoster.ext',
-                       $h->getFinalUrl( "Relative file path Location: should keep the latest host and scheme!" )
-               );
+               // Forge a Location header
+               $h->setRespHeaders( 'location', $location );
+               // Verify it correctly fixes the Location
+               $this->assertEquals( $final, $h->getFinalUrl(), $message );
        }
 
        /**
@@ -201,8 +214,6 @@ class HttpTest extends MediaWikiTestCase {
         * Extension API: 20140829
         *
         * Commented out constants that were removed in PHP 5.6.0
-        *
-        * @covers CurlHttpRequest::execute
         */
        public function provideCurlConstants() {
                return [
@@ -481,9 +492,8 @@ class HttpTest extends MediaWikiTestCase {
 
        /**
         * Added this test based on an issue experienced with HHVM 3.3.0-dev
-        * where it did not define a cURL constant.
+        * where it did not define a cURL constant. T72570
         *
-        * T72570
         * @dataProvider provideCurlConstants
         */
        public function testCurlConstants( $value ) {