From f338645527fc9a4eb51ecfa0d355adc283f55c59 Mon Sep 17 00:00:00 2001 From: Umherirrender Date: Tue, 5 Jan 2021 00:53:01 +0100 Subject: [PATCH] Create FauxRequestUpload to fake uploads in tests And use it in core Avoid direct use of super global $_FILES This can breaks all FauxRequest relaying on $_FILES in tests or production code via FauxRequest::getUpload. Falls back to $_FILES for the moment Bug: T48163 Change-Id: I7392acc9bb682ec6b7025dbed0734c142f45c91a --- autoload.php | 1 + includes/DerivativeRequest.php | 4 ++ includes/FauxRequest.php | 52 +++++++++++++++++++ includes/FauxRequestUpload.php | 47 +++++++++++++++++ includes/WebRequest.php | 9 ++-- includes/WebRequestUpload.php | 2 + tests/phpunit/includes/api/ApiTestCase.php | 12 ++++- .../includes/api/ApiUploadTestCase.php | 20 +++++-- .../ApiParamValidatorCallbacksTest.php | 16 +++--- 9 files changed, 147 insertions(+), 16 deletions(-) create mode 100644 includes/FauxRequestUpload.php diff --git a/autoload.php b/autoload.php index 93df91642bb..6bff119fda7 100644 --- a/autoload.php +++ b/autoload.php @@ -491,6 +491,7 @@ $wgAutoloadLocalClasses = [ 'FallbackContentHandler' => __DIR__ . '/includes/content/FallbackContentHandler.php', 'FatalError' => __DIR__ . '/includes/exception/FatalError.php', 'FauxRequest' => __DIR__ . '/includes/FauxRequest.php', + 'FauxRequestUpload' => __DIR__ . '/includes/FauxRequestUpload.php', 'FauxResponse' => __DIR__ . '/includes/FauxResponse.php', 'FauxSearchResult' => __DIR__ . '/includes/search/FauxSearchResult.php', 'FauxSearchResultSet' => __DIR__ . '/includes/search/FauxSearchResultSet.php', diff --git a/includes/DerivativeRequest.php b/includes/DerivativeRequest.php index c607db4784b..2dd474a0300 100644 --- a/includes/DerivativeRequest.php +++ b/includes/DerivativeRequest.php @@ -90,6 +90,10 @@ class DerivativeRequest extends FauxRequest { return $this->base->getProtocol(); } + public function getUpload( $key ) { + return $this->base->getUpload( $key ); + } + public function getElapsedTime() { return $this->base->getElapsedTime(); } diff --git a/includes/FauxRequest.php b/includes/FauxRequest.php index 4004969fb94..0cfb829ac67 100644 --- a/includes/FauxRequest.php +++ b/includes/FauxRequest.php @@ -36,6 +36,8 @@ class FauxRequest extends WebRequest { private $wasPosted = false; private $requestUrl; protected $cookies = []; + /** @var array */ + private $uploadData = []; /** * @stable to call @@ -147,6 +149,56 @@ class FauxRequest extends WebRequest { } } + /** + * Set fake upload data for all files + * + * @since 1.37 + * @param (array|WebRequestUpload)[] $uploadData + */ + public function setUploadData( $uploadData ) { + foreach ( $uploadData as $key => $data ) { + $this->setUpload( $key, $data ); + } + } + + /** + * Set fake upload data for one file with specific key + * + * @since 1.37 + * @param string $key + * @param array|WebRequestUpload $data + */ + public function setUpload( $key, $data ) { + if ( $data instanceof WebRequestUpload ) { + // cannot reuse WebRequestUpload, because it contains the original web request object + $data = [ + 'name' => $data->getName(), + 'type' => $data->getType(), + 'tmp_name' => $data->getTempName(), + 'size' => $data->getSize(), + 'error' => $data->getError(), + ]; + } + // Check if everything is provided + if ( !is_array( $data ) || + array_diff( WebRequestUpload::REQUIRED_FILEINFO_KEYS, array_keys( $data ) ) !== [] + ) { + throw new MWException( __METHOD__ . ' got bogus data' ); + } + $this->uploadData[$key] = $data; + } + + /** + * Return a FauxRequestUpload object corresponding to the key + * + * @param string $key + * @return FauxRequestUpload + */ + public function getUpload( $key ) { + // $_FILES is used for backward compatibility + return new FauxRequestUpload( $this->uploadData + $_FILES, $this, $key ); + } + /** * @since 1.25 * @param string $url diff --git a/includes/FauxRequestUpload.php b/includes/FauxRequestUpload.php new file mode 100644 index 00000000000..11c98d4ffde --- /dev/null +++ b/includes/FauxRequestUpload.php @@ -0,0 +1,47 @@ + value pairs, the + * fake (whole) FILES values + * @param FauxRequest $request The associated faux request + * @param string $key name of upload param + */ + public function __construct( $data, $request, $key ) { + $this->request = $request; + $this->doesExist = isset( $data[$key] ); + if ( $this->doesExist ) { + $this->fileInfo = $data[$key]; + } + } + +} diff --git a/includes/WebRequest.php b/includes/WebRequest.php index 541f4a3dd35..eb2b3a1f810 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -1038,8 +1038,7 @@ class WebRequest { * @return string|null String or null if no such file. */ public function getFileTempname( $key ) { - $file = new WebRequestUpload( $this, $key ); - return $file->getTempName(); + return $this->getUpload( $key )->getTempName(); } /** @@ -1049,8 +1048,7 @@ class WebRequest { * @return int */ public function getUploadError( $key ) { - $file = new WebRequestUpload( $this, $key ); - return $file->getError(); + return $this->getUpload( $key )->getError(); } /** @@ -1065,8 +1063,7 @@ class WebRequest { * @return string|null String or null if no such file. */ public function getFileName( $key ) { - $file = new WebRequestUpload( $this, $key ); - return $file->getName(); + return $this->getUpload( $key )->getName(); } /** diff --git a/includes/WebRequestUpload.php b/includes/WebRequestUpload.php index a492ae9be82..1fa9c6198b1 100644 --- a/includes/WebRequestUpload.php +++ b/includes/WebRequestUpload.php @@ -31,6 +31,8 @@ use MediaWiki\MediaWikiServices; * @ingroup HTTP */ class WebRequestUpload { + /** All keys a fileinfo has to specific to work with this class */ + public const REQUIRED_FILEINFO_KEYS = [ 'name', 'size', 'tmp_name', 'type', 'error', ]; /** @var WebRequest */ protected $request; /** @var bool */ diff --git a/tests/phpunit/includes/api/ApiTestCase.php b/tests/phpunit/includes/api/ApiTestCase.php index 6920204dc67..6107dfc3746 100644 --- a/tests/phpunit/includes/api/ApiTestCase.php +++ b/tests/phpunit/includes/api/ApiTestCase.php @@ -105,7 +105,7 @@ abstract class ApiTestCase extends MediaWikiLangTestCase { )->toString(); } - $wgRequest = new FauxRequest( $params, true, $sessionObj ); + $wgRequest = $this->buildFauxRequest( $params, $sessionObj ); RequestContext::getMain()->setRequest( $wgRequest ); RequestContext::getMain()->setUser( $contextUser ); MediaWiki\Auth\AuthManager::resetCache(); @@ -132,6 +132,16 @@ abstract class ApiTestCase extends MediaWikiLangTestCase { return $results; } + /** + * @since 1.37 + * @param array $params + * @param MediaWiki\Session\Session|array|null $session + * @return FauxRequest + */ + protected function buildFauxRequest( $params, $session ) { + return new FauxRequest( $params, true, $session ); + } + /** * Convenience function to access the token parameter of doApiRequest() * more succinctly. diff --git a/tests/phpunit/includes/api/ApiUploadTestCase.php b/tests/phpunit/includes/api/ApiUploadTestCase.php index ea2a91b7825..6aef464fd37 100644 --- a/tests/phpunit/includes/api/ApiUploadTestCase.php +++ b/tests/phpunit/includes/api/ApiUploadTestCase.php @@ -6,6 +6,13 @@ use MediaWiki\MediaWikiServices; * Abstract class to support upload tests */ abstract class ApiUploadTestCase extends ApiTestCase { + + /** + * @since 1.37 + * @var array Used to fake $_FILES in tests and given to FauxRequest + */ + protected $requestDataFiles = []; + /** * Fixture -- run before every test */ @@ -115,7 +122,7 @@ abstract class ApiUploadTestCase extends ApiTestCase { throw new Exception( "couldn't stat $tmpName" ); } - $_FILES[$fieldName] = [ + $this->requestDataFiles[$fieldName] = [ 'name' => $fileName, 'type' => $type, 'tmp_name' => $tmpName, @@ -139,7 +146,7 @@ abstract class ApiUploadTestCase extends ApiTestCase { throw new Exception( "couldn't stat $tmpName" ); } - $_FILES[$fieldName] = [ + $this->requestDataFiles[$fieldName] = [ 'name' => $fileName, 'type' => $type, 'tmp_name' => $tmpName, @@ -148,10 +155,17 @@ abstract class ApiUploadTestCase extends ApiTestCase { ]; } + /** @inheritDoc */ + protected function buildFauxRequest( $params, $session ) { + $request = parent::buildFauxRequest( $params, $session ); + $request->setUploadData( $this->requestDataFiles ); + return $request; + } + /** * Remove traces of previous fake uploads */ public function clearFakeUploads() { - $_FILES = []; + $this->requestDataFiles = []; } } diff --git a/tests/phpunit/includes/api/Validator/ApiParamValidatorCallbacksTest.php b/tests/phpunit/includes/api/Validator/ApiParamValidatorCallbacksTest.php index 2c24e9d759f..fff991d8433 100644 --- a/tests/phpunit/includes/api/Validator/ApiParamValidatorCallbacksTest.php +++ b/tests/phpunit/includes/api/Validator/ApiParamValidatorCallbacksTest.php @@ -92,7 +92,7 @@ class ApiParamValidatorCallbacksTest extends ApiUploadTestCase { $filePath = $this->filePath( 'yuv420.jpg' ); $this->fakeUploadFile( 'file', $fileName, $mimeType, $filePath ); - $_FILES['file2'] = [ + $this->requestDataFiles['file2'] = [ 'name' => '', 'type' => '', 'tmp_name' => '', @@ -100,7 +100,7 @@ class ApiParamValidatorCallbacksTest extends ApiUploadTestCase { 'error' => UPLOAD_ERR_NO_FILE, ]; - $_FILES['file3'] = [ + $this->requestDataFiles['file3'] = [ 'name' => 'xxx.png', 'type' => '', 'tmp_name' => '', @@ -112,10 +112,12 @@ class ApiParamValidatorCallbacksTest extends ApiUploadTestCase { public function testHasUpload() : void { $this->setupUploads(); - [ $callbacks, $main ] = $this->getCallbacks( new FauxRequest( [ + $request = new FauxRequest( [ 'foo' => '1', 'bar' => '', - ] ) ); + ] ); + $request->setUploadData( $this->requestDataFiles ); + [ $callbacks, $main ] = $this->getCallbacks( $request ); $this->assertFalse( $callbacks->hasUpload( 'foo', [] ) ); $this->assertFalse( $callbacks->hasUpload( 'bar', [] ) ); @@ -133,10 +135,12 @@ class ApiParamValidatorCallbacksTest extends ApiUploadTestCase { public function testGetUploadedFile() : void { $this->setupUploads(); - [ $callbacks, $main ] = $this->getCallbacks( new FauxRequest( [ + $request = new FauxRequest( [ 'foo' => '1', 'bar' => '', - ] ) ); + ] ); + $request->setUploadData( $this->requestDataFiles ); + [ $callbacks, $main ] = $this->getCallbacks( $request ); $this->assertNull( $callbacks->getUploadedFile( 'foo', [] ) ); $this->assertNull( $callbacks->getUploadedFile( 'bar', [] ) );