Merge "param-settings: Remove backward compatibility code from default"

4 files changed, 87 insertions, 105 deletions

diff --git a/includes/Rest/Handler.php b/includes/Rest/Handler.php
index 60c706e2ae4a..86f622b59874 100644
--- a/includes/Rest/Handler.php
+++ b/includes/Rest/Handler.php
@@ -553,13 +553,11 @@ abstract class Handler {
 	 * the request is to contain the parameter.
 	 *
 	 * Can be used for the request body as well, by setting self::PARAM_SOURCE
-	 * to "post" or "body". Note that the values of "body" parameters will become
-	 * accessible through getValidatedBody(), while the values of "post"
-	 * parameters will be accessible through getValidatedParams(). "post"
-	 * parameters are used with form data (application/x-www-form-urlencoded or
-	 * multipart/form-data).
+	 * to "post". Note that the values of "post" parameters will be accessible
+	 * through getValidatedParams(). "post" parameters are used with
+	 * form data (application/x-www-form-urlencoded or multipart/form-data).
 	 *
-	 * For "query" and "body" parameters, a PARAM_REQUIRED setting of "false" means the caller
+	 * For "query" parameters, a PARAM_REQUIRED setting of "false" means the caller
 	 * does not have to supply the parameter. For "path" parameters, the path matcher will always
 	 * require the caller to supply all path parameters for a route, regardless of the
 	 * PARAM_REQUIRED setting. However, "path" parameters may be specified in getParamSettings()
@@ -580,10 +578,6 @@ abstract class Handler {
 	 * by this method are used to validate the request body. The parameter
 	 * values will become available through getValidatedBody().
 	 *
-	 * The default implementation will call getParamSettings() and filter the
-	 * return value to only include settings for parameters that have
-	 * self::PARAM_SOURCE set to 'body'.
-	 *
 	 * Subclasses may override this method to specify what fields they support
 	 * in the request body. All parameter settings returned by this method must
 	 * have self::PARAM_SOURCE set to 'body'.
@@ -591,11 +585,7 @@ abstract class Handler {
 	 * @return array[]
 	 */
 	public function getBodyParamSettings(): array {
-		return array_filter( $this->getParamSettings(),
-			static function ( array $settings ) {
-				return ( $settings[self::PARAM_SOURCE] ?? false ) === 'body';
-			}
-		);
+		return [];
 	}
 
 	/**
diff --git a/tests/phpunit/structure/RestStructureTest.php b/tests/phpunit/structure/RestStructureTest.php
index 7e2dc8560946..08c7cc9cc0a9 100644
--- a/tests/phpunit/structure/RestStructureTest.php
+++ b/tests/phpunit/structure/RestStructureTest.php
@@ -191,38 +191,42 @@ class RestStructureTest extends MediaWikiIntegrationTestCase {
 		$request = new RequestData( [ 'method' => $method ] );
 		$handler = $module->getHandlerForPath( $path, $request, false );
 
-		$paramSettings = $handler->getParamSettings();
 		$bodySettings = $handler->getBodyParamSettings();
 
-		$bodySettingsFromParams = array_filter(
-			$paramSettings,
-			static function ( array $settings ) {
-				return $settings[ Handler::PARAM_SOURCE ] === 'body';
-			}
-		);
-
-		if ( !$bodySettings && !$bodySettingsFromParams ) {
+		if ( !$bodySettings ) {
 			$this->addToAssertionCount( 1 );
 			return;
 		}
 
-		if ( $bodySettingsFromParams ) {
-			$this->assertSame(
-				$bodySettingsFromParams,
-				$bodySettings,
-				'If getParamSettings and getBodyParamSettings both return body ' .
-				'parameters, they must return the same body parameters.' . "\n" .
-				'When overriding getBodyParamSettings, do not return body ' .
-				'parameters from getParamSettings.'
-			);
-		}
-
 		foreach ( $bodySettings as $settings ) {
 			$this->assertArrayHasKey( Handler::PARAM_SOURCE, $settings );
 			$this->assertSame( 'body', $settings[Handler::PARAM_SOURCE] );
 		}
 	}
 
+	/**
+	 * @dataProvider provideRoutes
+	 */
+	public function testBodyParametersNotInParamSettings( string $moduleName, string $method, string $path ): void {
+		$router = $this->getTestRouter();
+		$module = $router->getModule( $moduleName );
+
+		$request = new RequestData( [ 'method' => $method ] );
+		$handler = $module->getHandlerForPath( $path, $request, false );
+
+		$paramSettings = $handler->getParamSettings();
+
+		if ( !$paramSettings ) {
+			$this->addToAssertionCount( 1 );
+			return;
+		}
+
+		foreach ( $paramSettings as $settings ) {
+			$this->assertArrayHasKey( Handler::PARAM_SOURCE, $settings );
+			$this->assertNotSame( 'body', $settings[Handler::PARAM_SOURCE] );
+		}
+	}
+
 	public function provideModules(): Iterator {
 		$router = $this->getRouterForDataProviders();
 
diff --git a/tests/phpunit/unit/includes/Rest/Handler/HandlerTest.php b/tests/phpunit/unit/includes/Rest/Handler/HandlerTest.php
index 607ab0c26625..74139a5b9029 100644
--- a/tests/phpunit/unit/includes/Rest/Handler/HandlerTest.php
+++ b/tests/phpunit/unit/includes/Rest/Handler/HandlerTest.php
@@ -206,7 +206,7 @@ class HandlerTest extends MediaWikiUnitTestCase {
 	}
 
 	public static function provideValidate() {
-		yield 'empty' => [ [], new RequestData(), [], [] ];
+		yield 'empty' => [ [], [], new RequestData(), [], [] ];
 
 		yield 'query parameter' => [
 			[
@@ -216,12 +216,14 @@ class HandlerTest extends MediaWikiUnitTestCase {
 					Handler::PARAM_SOURCE => 'query',
 				]
 			],
+			[],
 			new RequestData( [ 'queryParams' => [ 'foo' => 'kittens' ] ] ),
 			[ 'foo' => 'kittens' ],
 			[]
 		];
 
 		yield 'body parameter' => [
+			[],
 			[
 				'foo' => [
 					ParamValidator::PARAM_TYPE => 'string',
@@ -233,33 +235,15 @@ class HandlerTest extends MediaWikiUnitTestCase {
 			[],
 			[ 'foo' => 'kittens' ]
 		];
-
-		yield 'body parameter with type coercion' => [
-			[
-				'foo' => [
-					ParamValidator::PARAM_TYPE => 'integer',
-					ParamValidator::PARAM_REQUIRED => true,
-					Handler::PARAM_SOURCE => 'body',
-				]
-			],
-			// Form data automatically enabled type coercion
-			new RequestData( [
-				'headers' => [
-					'Content-Type' => RequestInterface::FORM_URLENCODED_CONTENT_TYPE
-				],
-				'parsedBody' => [ 'foo' => '1234' ]
-			] ),
-			[],
-			[ 'foo' => 1234 ]
-		];
 	}
 
 	/**
 	 * @dataProvider provideValidate
 	 */
-	public function testValidate( $paramSettings, $request, $expectedParams, $expectedBody ) {
-		$handler = $this->newHandler( [ 'getParamSettings' ] );
+	public function testValidate( $paramSettings, $bodyParamSettings, $request, $expectedParams, $expectedBody ) {
+		$handler = $this->newHandler( [ 'getParamSettings', 'getBodyParamSettings' ] );
 		$handler->method( 'getParamSettings' )->willReturn( $paramSettings );
+		$handler->method( 'getBodyParamSettings' )->willReturn( $bodyParamSettings );
 
 		$this->initHandler( $handler, $request );
 		$this->validateHandler( $handler );
@@ -306,25 +290,6 @@ class HandlerTest extends MediaWikiUnitTestCase {
 				'failureCode' => 'missingparam'
 			]
 		];
-
-		yield 'body parameter without type coercion' => [
-			[
-				'foo' => [
-					ParamValidator::PARAM_TYPE => 'integer',
-					ParamValidator::PARAM_REQUIRED => true,
-					Handler::PARAM_SOURCE => 'body',
-				]
-			],
-			[ // JSON doesn't enable type coercion
-				'headers' => [ 'Content-Type' => 'application/json', ],
-				'parsedBody' => [ 'foo' => '1234' ]
-			],
-			[],
-			[
-				'error' => 'parameter-validation-failed',
-				'failureCode' => 'badinteger-type'
-			]
-		];
 	}
 
 	/**
@@ -368,24 +333,37 @@ class HandlerTest extends MediaWikiUnitTestCase {
 					ParamValidator::PARAM_TYPE => 'string',
 					ParamValidator::PARAM_REQUIRED => true,
 					Handler::PARAM_SOURCE => 'body',
-				],
-				'pathfoo' => [
-					ParamValidator::PARAM_TYPE => 'string',
-					ParamValidator::PARAM_REQUIRED => false,
-					Handler::PARAM_SOURCE => 'path',
-				],
+				]
 			],
 			new RequestData( [ 'parsedBody' => [ 'foo' => 'kittens' ] ] ),
 			[ 'foo' => 'kittens' ]
 		];
+
+		yield 'body parameter with type coercion' => [
+			[
+				'foo' => [
+					ParamValidator::PARAM_TYPE => 'integer',
+					ParamValidator::PARAM_REQUIRED => true,
+					Handler::PARAM_SOURCE => 'body',
+				]
+			],
+			new RequestData( [
+				'headers' => [
+					// Form data automatically enabled type coercion
+					'Content-Type' => RequestInterface::FORM_URLENCODED_CONTENT_TYPE
+				],
+				'parsedBody' => [ 'foo' => '1234' ]
+			] ),
+			[ 'foo' => 1234 ]
+		];
 	}
 
 	/**
 	 * @dataProvider provideValidateBodyParams
 	 */
 	public function testValidateBodyParams( $paramSettings, $request, $expected ) {
-		$handler = $this->newHandler( [ 'getParamSettings' ] );
-		$handler->method( 'getParamSettings' )->willReturn( $paramSettings );
+		$handler = $this->newHandler( [ 'getBodyParamSettings' ] );
+		$handler->method( 'getBodyParamSettings' )->willReturn( $paramSettings );
 
 		$this->initHandler( $handler, $request );
 		$this->validateHandler( $handler );
@@ -395,28 +373,19 @@ class HandlerTest extends MediaWikiUnitTestCase {
 	}
 
 	public function testGetBodyParamSettings() {
-		$paramSettings = [
-			'pathfoo' => [
-				ParamValidator::PARAM_TYPE => 'string',
-				Handler::PARAM_SOURCE => 'path',
-			],
+		$bodyParamSettings = [
 			'bodyfoo' => [
 				ParamValidator::PARAM_TYPE => 'string',
 				Handler::PARAM_SOURCE => 'body',
-			],
-			'queryfoo' => [
-				ParamValidator::PARAM_TYPE => 'string',
-				Handler::PARAM_SOURCE => 'query',
-			],
+			]
 		];
+		$handler = $this->newHandler( [ 'getBodyParamSettings' ] );
+		$handler->method( 'getBodyParamSettings' )->willReturn( $bodyParamSettings );
 
-		$handler = $this->newHandler( [ 'getParamSettings' ] );
-		$handler->method( 'getParamSettings' )->willReturn( $paramSettings );
-
-		$bodyParamSettings = $handler->getBodyParamSettings();
+		$bodyParams = $handler->getBodyParamSettings();
 
 		$expected = [
-			'bodyfoo' => $paramSettings['bodyfoo']
+			'bodyfoo' => $bodyParamSettings['bodyfoo']
 		];
 
 		$this->assertSame( $expected, $bodyParamSettings );
@@ -531,14 +500,30 @@ class HandlerTest extends MediaWikiUnitTestCase {
 			),
 			'badtimestamp'
 		];
+
+		yield 'body parameter without type coercion' => [
+			[
+				'foo' => [
+					ParamValidator::PARAM_TYPE => 'integer',
+					ParamValidator::PARAM_REQUIRED => true,
+					Handler::PARAM_SOURCE => 'body',
+				]
+			],
+			new RequestData( [
+				// JSON doesn't enable type coercion
+				'headers' => [ 'Content-Type' => 'application/json', ],
+				'parsedBody' => [ 'foo' => '1234' ]
+			] ),
+			'badinteger-type'
+		];
 	}
 
 	/**
 	 * @dataProvider provideValidateBodyParams_invalid
 	 */
 	public function testValidateBodyParams_invalid( $paramSettings, $request, $expectedError ) {
-		$handler = $this->newHandler( [ 'getParamSettings' ] );
-		$handler->method( 'getParamSettings' )->willReturn( $paramSettings );
+		$handler = $this->newHandler( [ 'getBodyParamSettings' ] );
+		$handler->method( 'getBodyParamSettings' )->willReturn( $paramSettings );
 
 		try {
 			$this->initHandler( $handler, $request );
@@ -1203,8 +1188,8 @@ class HandlerTest extends MediaWikiUnitTestCase {
 			'application/x-www-form-urlencoded'
 		];
 
-		$handler = $this->newHandler( [ 'getParamSettings', 'getSupportedRequestTypes' ] );
-		$handler->method( 'getParamSettings' )->willReturn( $paramSettings );
+		$handler = $this->newHandler( [ 'getBodyParamSettings', 'getSupportedRequestTypes' ] );
+		$handler->method( 'getBodyParamSettings' )->willReturn( $paramSettings );
 		$handler->method( 'getSupportedRequestTypes' )->willReturn( $supportedTypes );
 
 		// Should accept form data
diff --git a/tests/phpunit/unit/includes/Rest/RouterTest.php b/tests/phpunit/unit/includes/Rest/RouterTest.php
index e2a4de70b42d..e4942513d938 100644
--- a/tests/phpunit/unit/includes/Rest/RouterTest.php
+++ b/tests/phpunit/unit/includes/Rest/RouterTest.php
@@ -718,8 +718,11 @@ class RouterTest extends MediaWikiUnitTestCase {
 	}
 
 	public function testFormDataSupported() {
+		// See T362850
+		$this->expectDeprecationAndContinue( '/The "post" source is deprecated/' );
+
 		$request = new RequestData( [
-			'uri' => new Uri( '/rest/mock/v1/RouterTest/form-handler' ),
+			'uri' => new Uri( '/rest/mock/v1/RouterTest/echo_form_data' ),
 			'method' => 'POST',
 			'postParams' => [ 'foo' => 'bar' ],
 			'headers' => [ "content-type" => 'application/x-www-form-urlencoded' ]
@@ -732,7 +735,7 @@ class RouterTest extends MediaWikiUnitTestCase {
 		$body = $response->getBody();
 		$body->rewind();
 		$data = json_decode( $body->getContents(), true );
-		$this->assertSame( [ 'foo' => 'bar' ], $data );
+		$this->assertSame( [ 'foo' => 'bar' ], $data[ 'parsedBody' ] );
 	}
 
 	public function testJsonBody() {