From: Timo Tijhof <krinklemail@gmail.com>
Date: Wed, 29 Mar 2017 00:21:15 +0000 (-0700)
Subject: HttpFunctions: Increase code coverage
X-Git-Tag: 1.31.0-rc.0~3675^2
X-Git-Url: http://git.cyclocoop.org/%24href?a=commitdiff_plain;h=e9f577fd6ba219d2bbce4424e38d287bc0da5643;p=lhc%2Fweb%2Fwiklou.git

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 8d58ce5314..88cc510219 100644
--- 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 036baa8c27..3693a277a1 100644
--- 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 ) {