From 1531659d252cd85c6f625a79c7e1f6e5f847fe3f Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Tue, 25 Oct 2016 21:08:14 -0700 Subject: [PATCH] Clean up http classes a bit * added integration tests. We probably don't want automated tests to make external requests but these make manual testing more convenient. Documented some oddities discovered by testing. * made ::$status, ::proxySetup() and ::getHeaderList() protected; they were not referenced in any gerrit-hosted extension and they provide no useful functionality to external callers. Similarly, marked ::read() and ::errorHandler() as internal (these are used as callbacks so can't be protected) * removed inheritance abuse in ::execute() * documented ::execute() as returning a StatusValue (but keep returning a Status for now) * changed setCookie argument defaults to ones that make sense * replaced MWException * moved unit tests to the correct location * fixed some code style issues Change-Id: I5852fc75badc5d475ae30ec2c9376bde7024bd95 --- RELEASE-NOTES-1.29 | 2 + includes/http/CurlHttpRequest.php | 9 +- includes/http/Http.php | 2 +- includes/http/MWHttpRequest.php | 51 +++-- includes/http/PhpHttpRequest.php | 10 +- includes/libs/CookieJar.php | 4 + tests/common/TestsAutoLoader.php | 4 +- .../includes/http/CurlHttpRequestTest.php | 5 + .../includes/http/MWHttpRequestTestCase.php | 208 ++++++++++++++++++ .../includes/http/PhpHttpRequestTest.php | 5 + .../phpunit/includes/{ => http}/HttpTest.php | 4 +- 11 files changed, 273 insertions(+), 31 deletions(-) create mode 100644 tests/integration/includes/http/CurlHttpRequestTest.php create mode 100644 tests/integration/includes/http/MWHttpRequestTestCase.php create mode 100644 tests/integration/includes/http/PhpHttpRequestTest.php rename tests/phpunit/includes/{ => http}/HttpTest.php (99%) diff --git a/RELEASE-NOTES-1.29 b/RELEASE-NOTES-1.29 index ab52544fc6..21a94c5d4a 100644 --- a/RELEASE-NOTES-1.29 +++ b/RELEASE-NOTES-1.29 @@ -56,6 +56,8 @@ changes to languages because of Phabricator reports. SearchEngineFactory::getSearchEngineClass() instead. * $wgSessionsInMemcached (deprecated in 1.20) was removed. No replacement is required as all sessions are stored in Object Cache now. +* MWHttpRequest::execute() should be considered to return a StatusValue; the + Status return type is deprecated. == Compatibility == diff --git a/includes/http/CurlHttpRequest.php b/includes/http/CurlHttpRequest.php index f58c3a9a5b..7fd3e835c4 100644 --- a/includes/http/CurlHttpRequest.php +++ b/includes/http/CurlHttpRequest.php @@ -38,11 +38,10 @@ class CurlHttpRequest extends MWHttpRequest { } public function execute() { - - parent::execute(); + $this->prepare(); if ( !$this->status->isOK() ) { - return $this->status; + return Status::wrap( $this->status ); // TODO B/C; move this to callers } $this->curlOptions[CURLOPT_PROXY] = $this->proxy; @@ -102,7 +101,7 @@ class CurlHttpRequest extends MWHttpRequest { $curlHandle = curl_init( $this->url ); if ( !curl_setopt_array( $curlHandle, $this->curlOptions ) ) { - throw new MWException( "Error setting curl options." ); + throw new InvalidArgumentException( "Error setting curl options." ); } if ( $this->followRedirects && $this->canFollowRedirects() ) { @@ -140,7 +139,7 @@ class CurlHttpRequest extends MWHttpRequest { $this->parseHeader(); $this->setStatus(); - return $this->status; + return Status::wrap( $this->status ); // TODO B/C; move this to callers } /** diff --git a/includes/http/Http.php b/includes/http/Http.php index 43ae2d0e8f..a68a63b7f0 100644 --- a/includes/http/Http.php +++ b/includes/http/Http.php @@ -74,7 +74,7 @@ class Http { } else { $errors = $status->getErrorsByType( 'error' ); $logger = LoggerFactory::getInstance( 'http' ); - $logger->warning( $status->getWikiText( false, false, 'en' ), + $logger->warning( Status::wrap( $status )->getWikiText( false, false, 'en' ), [ 'error' => $errors, 'caller' => $caller, 'content' => $req->getContent() ] ); return false; } diff --git a/includes/http/MWHttpRequest.php b/includes/http/MWHttpRequest.php index 08883ae44f..a42b6d0991 100644 --- a/includes/http/MWHttpRequest.php +++ b/includes/http/MWHttpRequest.php @@ -46,9 +46,11 @@ class MWHttpRequest implements LoggerAwareInterface { protected $reqHeaders = []; protected $url; protected $parsedUrl; + /** @var callable */ protected $callback; protected $maxRedirects = 5; protected $followRedirects = false; + protected $connectTimeout; /** * @var CookieJar @@ -60,7 +62,8 @@ class MWHttpRequest implements LoggerAwareInterface { protected $respStatus = "200 Ok"; protected $respHeaders = []; - public $status; + /** @var StatusValue */ + protected $status; /** * @var Profiler @@ -98,9 +101,9 @@ class MWHttpRequest implements LoggerAwareInterface { } if ( !$this->parsedUrl || !Http::isValidURI( $this->url ) ) { - $this->status = Status::newFatal( 'http-invalid-url', $url ); + $this->status = StatusValue::newFatal( 'http-invalid-url', $url ); } else { - $this->status = Status::newGood( 100 ); // continue + $this->status = StatusValue::newGood( 100 ); // continue } if ( isset( $options['timeout'] ) && $options['timeout'] != 'default' ) { @@ -161,7 +164,7 @@ class MWHttpRequest implements LoggerAwareInterface { * @param string $url Url to use * @param array $options (optional) extra params to pass (see Http::request()) * @param string $caller The method making this request, for profiling - * @throws MWException + * @throws DomainException * @return CurlHttpRequest|PhpHttpRequest * @see MWHttpRequest::__construct */ @@ -169,7 +172,7 @@ class MWHttpRequest implements LoggerAwareInterface { if ( !Http::$httpEngine ) { Http::$httpEngine = function_exists( 'curl_init' ) ? 'curl' : 'php'; } elseif ( Http::$httpEngine == 'curl' && !function_exists( 'curl_init' ) ) { - throw new MWException( __METHOD__ . ': curl (http://php.net/curl) is not installed, but' . + throw new DomainException( __METHOD__ . ': curl (http://php.net/curl) is not installed, but' . ' Http::$httpEngine is set to "curl"' ); } @@ -186,7 +189,7 @@ class MWHttpRequest implements LoggerAwareInterface { return new CurlHttpRequest( $url, $options, $caller, Profiler::instance() ); case 'php': if ( !wfIniGetBool( 'allow_url_fopen' ) ) { - throw new MWException( __METHOD__ . ': allow_url_fopen ' . + throw new DomainException( __METHOD__ . ': allow_url_fopen ' . 'needs to be enabled for pure PHP http requests to ' . 'work. If possible, curl should be used instead. See ' . 'http://php.net/curl.' @@ -194,7 +197,7 @@ class MWHttpRequest implements LoggerAwareInterface { } return new PhpHttpRequest( $url, $options, $caller, Profiler::instance() ); default: - throw new MWException( __METHOD__ . ': The setting of Http::$httpEngine is not valid.' ); + throw new DomainException( __METHOD__ . ': The setting of Http::$httpEngine is not valid.' ); } } @@ -222,7 +225,7 @@ class MWHttpRequest implements LoggerAwareInterface { * * @return void */ - public function proxySetup() { + protected function proxySetup() { // If there is an explicit proxy set and proxies are not disabled, then use it if ( $this->proxy && !$this->noProxy ) { return; @@ -300,7 +303,7 @@ class MWHttpRequest implements LoggerAwareInterface { * Get an array of the headers * @return array */ - public function getHeaderList() { + protected function getHeaderList() { $list = []; if ( $this->cookieJar ) { @@ -333,12 +336,14 @@ class MWHttpRequest implements LoggerAwareInterface { * bytes are reported handled than were passed to you, the HTTP fetch * will be aborted. * - * @param callable $callback - * @throws MWException + * @param callable|null $callback + * @throws InvalidArgumentException */ public function setCallback( $callback ) { - if ( !is_callable( $callback ) ) { - throw new MWException( 'Invalid MwHttpRequest callback' ); + if ( is_null( $callback ) ) { + $callback = [ $this, 'read' ]; + } elseif ( !is_callable( $callback ) ) { + throw new InvalidArgumentException( __METHOD__ . ': invalid callback' ); } $this->callback = $callback; } @@ -350,6 +355,7 @@ class MWHttpRequest implements LoggerAwareInterface { * @param resource $fh * @param string $content * @return int + * @internal */ public function read( $fh, $content ) { $this->content .= $content; @@ -359,9 +365,14 @@ class MWHttpRequest implements LoggerAwareInterface { /** * Take care of whatever is necessary to perform the URI request. * - * @return Status + * @return StatusValue + * @note currently returns Status for B/C */ public function execute() { + throw new LogicException( 'children must override this' ); + } + + protected function prepare() { $this->content = ""; if ( strtoupper( $this->method ) == "HEAD" ) { @@ -371,7 +382,7 @@ class MWHttpRequest implements LoggerAwareInterface { $this->proxySetup(); // set up any proxy as needed if ( !$this->callback ) { - $this->setCallback( [ $this, 'read' ] ); + $this->setCallback( null ); } if ( !isset( $this->reqHeaders['User-Agent'] ) ) { @@ -494,6 +505,8 @@ class MWHttpRequest implements LoggerAwareInterface { /** * Tells the MWHttpRequest object to use this pre-loaded CookieJar. * + * To read response cookies from the jar, getCookieJar must be called first. + * * @param CookieJar $jar */ public function setCookieJar( $jar ) { @@ -519,14 +532,18 @@ class MWHttpRequest implements LoggerAwareInterface { * Set-Cookie headers. * @see Cookie::set * @param string $name - * @param mixed $value + * @param string $value * @param array $attr */ - public function setCookie( $name, $value = null, $attr = null ) { + public function setCookie( $name, $value, $attr = [] ) { if ( !$this->cookieJar ) { $this->cookieJar = new CookieJar; } + if ( $this->parsedUrl && !isset( $attr['domain'] ) ) { + $attr['domain'] = $this->parsedUrl['host']; + } + $this->cookieJar->setCookie( $name, $value, $attr ); } diff --git a/includes/http/PhpHttpRequest.php b/includes/http/PhpHttpRequest.php index 2af000fac0..d8a9949d2f 100644 --- a/includes/http/PhpHttpRequest.php +++ b/includes/http/PhpHttpRequest.php @@ -87,6 +87,7 @@ class PhpHttpRequest extends MWHttpRequest { * is completely useless (something like "fopen: failed to open stream") * so normal methods of handling errors programmatically * like get_last_error() don't work. + * @internal */ public function errorHandler( $errno, $errstr ) { $n = count( $this->fopenErrors ) + 1; @@ -94,8 +95,7 @@ class PhpHttpRequest extends MWHttpRequest { } public function execute() { - - parent::execute(); + $this->prepare(); if ( is_array( $this->postData ) ) { $this->postData = wfArrayToCgi( $this->postData ); @@ -227,12 +227,12 @@ class PhpHttpRequest extends MWHttpRequest { . ': error opening connection: {errstr1}', $this->fopenErrors ); } $this->status->fatal( 'http-request-error' ); - return $this->status; + return Status::wrap( $this->status ); // TODO B/C; move this to callers } if ( $result['timed_out'] ) { $this->status->fatal( 'http-timed-out', $this->url ); - return $this->status; + return Status::wrap( $this->status ); // TODO B/C; move this to callers } // If everything went OK, or we received some error code @@ -253,6 +253,6 @@ class PhpHttpRequest extends MWHttpRequest { } fclose( $fh ); - return $this->status; + return Status::wrap( $this->status ); // TODO B/C; move this to callers } } diff --git a/includes/libs/CookieJar.php b/includes/libs/CookieJar.php index 910a7ca82d..8f5700abb9 100644 --- a/includes/libs/CookieJar.php +++ b/includes/libs/CookieJar.php @@ -19,7 +19,11 @@ * @ingroup HTTP */ +/** + * Cookie jar to use with MWHttpRequest. Does not handle cookie unsetting. + */ class CookieJar { + /** @var Cookie[] */ private $cookie = []; /** diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index 66df315e10..b67c9abfee 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -29,11 +29,13 @@ $wgAutoloadClasses += [ # tests/common 'TestSetup' => "$testDir/common/TestSetup.php", + # tests/integration + 'MWHttpRequestTestCase' => "$testDir/integration/includes/http/MWHttpRequestTestCase.php", + # tests/parser 'DbTestPreviewer' => "$testDir/parser/DbTestPreviewer.php", 'DbTestRecorder' => "$testDir/parser/DbTestRecorder.php", 'DjVuSupport' => "$testDir/parser/DjVuSupport.php", - 'TestRecorder' => "$testDir/parser/TestRecorder.php", 'MultiTestRecorder' => "$testDir/parser/MultiTestRecorder.php", 'ParserTestMockParser' => "$testDir/parser/ParserTestMockParser.php", 'ParserTestRunner' => "$testDir/parser/ParserTestRunner.php", diff --git a/tests/integration/includes/http/CurlHttpRequestTest.php b/tests/integration/includes/http/CurlHttpRequestTest.php new file mode 100644 index 0000000000..04f80f434f --- /dev/null +++ b/tests/integration/includes/http/CurlHttpRequestTest.php @@ -0,0 +1,5 @@ +oldHttpEngine = Http::$httpEngine; + Http::$httpEngine = static::$httpEngine; + + try { + $request = MWHttpRequest::factory( 'null:' ); + } catch ( DomainException $e ) { + $this->markTestSkipped( static::$httpEngine . ' engine not supported' ); + } + + if ( static::$httpEngine === 'php' ) { + $this->assertInstanceOf( PhpHttpRequest::class, $request ); + } else { + $this->assertInstanceOf( CurlHttpRequest::class, $request ); + } + } + + public function tearDown() { + parent::tearDown(); + Http::$httpEngine = $this->oldHttpEngine; + } + + // -------------------- + + public function testIsRedirect() { + $request = MWHttpRequest::factory( 'http://httpbin.org/get' ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertFalse( $request->isRedirect() ); + + $request = MWHttpRequest::factory( 'http://httpbin.org/redirect/1' ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertTrue( $request->isRedirect() ); + } + + public function testgetFinalUrl() { + $request = MWHttpRequest::factory( 'http://httpbin.org/redirect/3' ); + if ( !$request->canFollowRedirects() ) { + $this->markTestSkipped( 'cannot follow redirects' ); + } + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertNotSame( 'http://httpbin.org/get', $request->getFinalUrl() ); + + $request = MWHttpRequest::factory( 'http://httpbin.org/redirect/3', [ 'followRedirects' + => true ] ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertSame( 'http://httpbin.org/get', $request->getFinalUrl() ); + $this->assertResponseFieldValue( 'url', 'http://httpbin.org/get', $request ); + + $request = MWHttpRequest::factory( 'http://httpbin.org/redirect/3', [ 'followRedirects' + => true ] ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertSame( 'http://httpbin.org/get', $request->getFinalUrl() ); + $this->assertResponseFieldValue( 'url', 'http://httpbin.org/get', $request ); + + if ( static::$httpEngine === 'curl' ) { + $this->markTestIncomplete( 'maxRedirects seems to be ignored by CurlHttpRequest' ); + return; + } + + $request = MWHttpRequest::factory( 'http://httpbin.org/redirect/3', [ 'followRedirects' + => true, 'maxRedirects' => 1 ] ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertNotSame( 'http://httpbin.org/get', $request->getFinalUrl() ); + } + + public function testSetCookie() { + $request = MWHttpRequest::factory( 'http://httpbin.org/cookies' ); + $request->setCookie( 'foo', 'bar' ); + $request->setCookie( 'foo2', 'bar2', [ 'domain' => 'example.com' ] ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertResponseFieldValue( 'cookies', [ 'foo' => 'bar' ], $request ); + } + + public function testSetCookieJar() { + $request = MWHttpRequest::factory( 'http://httpbin.org/cookies' ); + $cookieJar = new CookieJar(); + $cookieJar->setCookie( 'foo', 'bar', [ 'domain' => 'httpbin.org' ] ); + $cookieJar->setCookie( 'foo2', 'bar2', [ 'domain' => 'example.com' ] ); + $request->setCookieJar( $cookieJar ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertResponseFieldValue( 'cookies', [ 'foo' => 'bar' ], $request ); + + $request = MWHttpRequest::factory( 'http://httpbin.org/cookies/set?foo=bar' ); + $cookieJar = new CookieJar(); + $request->setCookieJar( $cookieJar ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertHasCookie( 'foo', 'bar', $request->getCookieJar() ); + + $this->markTestIncomplete( 'CookieJar does not handle deletion' ); + return; + + $request = MWHttpRequest::factory( 'http://httpbin.org/cookies/delete?foo' ); + $cookieJar = new CookieJar(); + $cookieJar->setCookie( 'foo', 'bar', [ 'domain' => 'httpbin.org' ] ); + $cookieJar->setCookie( 'foo2', 'bar2', [ 'domain' => 'httpbin.org' ] ); + $request->setCookieJar( $cookieJar ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertNotHasCookie( 'foo', $request->getCookieJar() ); + $this->assertHasCookie( 'foo2', 'bar2', $request->getCookieJar() ); + } + + public function testGetResponseHeaders() { + $request = MWHttpRequest::factory( 'http://httpbin.org/response-headers?Foo=bar' ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $headers = array_change_key_case( $request->getResponseHeaders(), CASE_LOWER ); + $this->assertArrayHasKey( 'foo', $headers ); + $this->assertSame( $request->getResponseHeader( 'Foo' ), 'bar' ); + } + + public function testSetHeader() { + $request = MWHttpRequest::factory( 'http://httpbin.org/headers' ); + $request->setHeader( 'Foo', 'bar' ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertResponseFieldValue( [ 'headers', 'Foo' ], 'bar', $request ); + } + + public function testGetStatus() { + $request = MWHttpRequest::factory( 'http://httpbin.org/status/418' ); + $status = $request->execute(); + $this->assertFalse( $status->isOK() ); + $this->assertSame( $request->getStatus(), 418 ); + } + + public function testSetUserAgent() { + $request = MWHttpRequest::factory( 'http://httpbin.org/user-agent' ); + $request->setUserAgent( 'foo' ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertResponseFieldValue( 'user-agent', 'foo', $request ); + } + + public function testSetData() { + $request = MWHttpRequest::factory( 'http://httpbin.org/post', [ 'method' => 'POST' ] ); + $request->setData( [ 'foo' => 'bar', 'foo2' => 'bar2' ] ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $this->assertResponseFieldValue( 'form', [ 'foo' => 'bar', 'foo2' => 'bar2' ], $request ); + } + + public function testSetCallback() { + if ( static::$httpEngine === 'php' ) { + $this->markTestIncomplete( 'PhpHttpRequest does not use setCallback()' ); + return; + } + + $request = MWHttpRequest::factory( 'http://httpbin.org/ip' ); + $data = ''; + $request->setCallback( function ( $fh, $content ) use ( &$data ) { + $data .= $content; + return strlen( $content ); + } ); + $status = $request->execute(); + $this->assertTrue( $status->isGood() ); + $data = json_decode( $data, true ); + $this->assertInternalType( 'array', $data ); + $this->assertArrayHasKey( 'origin', $data ); + } + + // -------------------- + + protected function assertResponseFieldValue( $key, $expectedValue, MWHttpRequest $response ) { + $this->assertSame( 200, $response->getStatus(), 'response status is not 200' ); + $data = json_decode( $response->getContent(), true ); + $this->assertInternalType( 'array', $data, 'response is not JSON' ); + $keyPath = ''; + foreach ( (array)$key as $keySegment ) { + $keyPath .= ( $keyPath ? '.' : '' ) . $keySegment; + $this->assertArrayHasKey( $keySegment, $data, $keyPath . ' not found' ); + $data = $data[$keySegment]; + } + $this->assertSame( $expectedValue, $data ); + } + + protected function assertHasCookie( $expectedName, $expectedValue, CookieJar $cookieJar ) { + $cookieJar = TestingAccessWrapper::newFromObject( $cookieJar ); + $cookies = array_change_key_case( $cookieJar->cookie, CASE_LOWER ); + $this->assertArrayHasKey( strtolower( $expectedName ), $cookies ); + $cookie = TestingAccessWrapper::newFromObject( + $cookies[strtolower( $expectedName )] ); + $this->assertSame( $expectedValue, $cookie->value ); + } + + protected function assertNotHasCookie( $name, CookieJar $cookieJar ) { + $cookieJar = TestingAccessWrapper::newFromObject( $cookieJar ); + $this->assertArrayNotHasKey( strtolower( $name ), + array_change_key_case( $cookieJar->cookie, CASE_LOWER ) ); + } +} + diff --git a/tests/integration/includes/http/PhpHttpRequestTest.php b/tests/integration/includes/http/PhpHttpRequestTest.php new file mode 100644 index 0000000000..d0222a5e76 --- /dev/null +++ b/tests/integration/includes/http/PhpHttpRequestTest.php @@ -0,0 +1,5 @@ +