Krinkle has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/345269 )
Change subject: HttpFunctions: Increase code coverage
......................................................................
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
---
M includes/http/MWHttpRequest.php
M tests/phpunit/includes/http/HttpTest.php
2 files changed, 60 insertions(+), 52 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/69/345269/1
diff --git a/includes/http/MWHttpRequest.php b/includes/http/MWHttpRequest.php
index 8d58ce5..88cc510 100644
--- a/includes/http/MWHttpRequest.php
+++ b/includes/http/MWHttpRequest.php
@@ -609,19 +609,17 @@
}
}
- 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..8a2708b 100644
--- a/tests/phpunit/includes/http/HttpTest.php
+++ b/tests/phpunit/includes/http/HttpTest.php
@@ -66,6 +66,13 @@
* @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 @@
];
}
+ 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 @@
* 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 @@
/**
* 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 ) {
--
To view, visit https://gerrit.wikimedia.org/r/345269
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I775d95965dc23a1e6c4c62ed84f9da64b6c72135
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
