Skip to content

Fix behavior for controller actions with union types #20352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions framework/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Yii Framework 2 Change Log
- Bug #20329: pgsql: Column Schema doesn't recognize PG type cast (arkhamvm)
- Bug #8298: Loading fixtures does not update table sequence for Postgresql database (mtangoo)
- Bug #20347: Fix compatibility with PHP 8.4: remove usage of `session.use_trans_sid` and `session.use_only_cookies` (tehmaestro)
- Bug #20351: Fix behavior for `yii\web\Controller::bindActionParams` around union types (chriscpty)


2.0.52 February 13, 2025
Expand Down
164 changes: 130 additions & 34 deletions framework/web/Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -133,41 +133,15 @@
$name = $param->getName();
if (array_key_exists($name, $params)) {
$isValid = true;
$isArray = ($type = $param->getType()) instanceof \ReflectionNamedType && $type->getName() === 'array';
if ($isArray) {
$params[$name] = (array)$params[$name];
} elseif (is_array($params[$name])) {
$isValid = false;
} elseif (
PHP_VERSION_ID >= 70000
&& ($type = $param->getType()) !== null
&& method_exists($type, 'isBuiltin')
&& $type->isBuiltin()
&& ($params[$name] !== null || !$type->allowsNull())
) {
$typeName = PHP_VERSION_ID >= 70100 ? $type->getName() : (string)$type;

if ($params[$name] === '' && $type->allowsNull()) {
if ($typeName !== 'string') { // for old string behavior compatibility
$params[$name] = null;
}
} else {
switch ($typeName) {
case 'int':
$params[$name] = filter_var($params[$name], FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE);
break;
case 'float':
$params[$name] = filter_var($params[$name], FILTER_VALIDATE_FLOAT, FILTER_NULL_ON_FAILURE);
break;
case 'bool':
$params[$name] = filter_var($params[$name], FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE);
break;
}
if ($params[$name] === null) {
$isValid = false;
}
}
$type = $param->getType();
if ($type instanceof \ReflectionNamedType) {
[$result, $isValid] = $this->filterSingleTypeActionParam($params[$name], $type);
$params[$name] = $result;
} elseif (class_exists('\ReflectionUnionType') && $type instanceof \ReflectionUnionType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think class_exists is needed here.

[$result, $isValid] = $this->filterUnionTypeActionParam($params[$name], $type);
$params[$name] = $result;

Check warning on line 142 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L141-L142

Added lines #L141 - L142 were not covered by tests
}

if (!$isValid) {
throw new BadRequestHttpException(
Yii::t('yii', 'Invalid data received for parameter "{param}".', ['param' => $name])
Expand Down Expand Up @@ -211,6 +185,128 @@
return $args;
}

/**
* The logic for [[bindActionParam]] to validate whether a given parameter matches the action's typing
* if the function parameter has a single named type.
* @param mixed $param The parameter value.
* @param \ReflectionNamedType $type
* @return array{0: mixed, 1: bool} The resulting parameter value and a boolean indicating whether the value is valid.
*/
private function filterSingleTypeActionParam($param, $type)
{
$isArray = $type->getName() === 'array';
if ($isArray) {
return [(array)$param, true];

Check warning on line 199 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L199

Added line #L199 was not covered by tests
}

if (is_array($param)) {
return [$param, false];

Check warning on line 203 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L203

Added line #L203 was not covered by tests
}

if (
PHP_VERSION_ID >= 70000
&& method_exists($type, 'isBuiltin')
&& $type->isBuiltin()
&& ($param !== null || !$type->allowsNull())
) {
$typeName = PHP_VERSION_ID >= 70100 ? $type->getName() : (string)$type;
if ($param === '' && $type->allowsNull()) {
if ($typeName !== 'string') { // for old string behavior compatibility
return [null, true];
}
return ['', true];
}

$filterResult = $this->filterParamByType($param, $typeName);
return [$filterResult, $filterResult !== null];
}
return [$param, true];
}

/**
* The logic for [[bindActionParam]] to validate whether a given parameter matches the action's typing
* if the function parameter has a union type.
* @param mixed $param The parameter value.
* @param \ReflectionUnionType $type
* @return array{0: mixed, 1: bool} The resulting parameter value and a boolean indicating whether the value is valid.
*/
private function filterUnionTypeActionParam($param, $type)

Check warning on line 233 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L233

Added line #L233 was not covered by tests
{
$types = $type->getTypes();
if ($param === '' && $type->allowsNull()) {

Check warning on line 236 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L235-L236

Added lines #L235 - L236 were not covered by tests
// check if type can be string for old string behavior compatibility
foreach ($types as $partialType) {

Check warning on line 238 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L238

Added line #L238 was not covered by tests
if (
$partialType === null
|| !method_exists($partialType, 'isBuiltin')
|| !$partialType->isBuiltin()

Check warning on line 242 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L240-L242

Added lines #L240 - L242 were not covered by tests
) {
continue;

Check warning on line 244 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L244

Added line #L244 was not covered by tests
}
$typeName = PHP_VERSION_ID >= 70100 ? $partialType->getName() : (string)$partialType;
if ($typeName === 'string') {
return ['', true];

Check warning on line 248 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L246-L248

Added lines #L246 - L248 were not covered by tests
}
}
return [null, true];

Check warning on line 251 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L251

Added line #L251 was not covered by tests
}
// if we found a built-in type but didn't return out, its validation failed
$foundBuiltinType = false;

Check warning on line 254 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L254

Added line #L254 was not covered by tests
// we save returning out an array or string for later because other types should take precedence
$canBeArray = false;
$canBeString = false;
foreach ($types as $partialType) {

Check warning on line 258 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L256-L258

Added lines #L256 - L258 were not covered by tests
if (
$partialType === null
|| !method_exists($partialType, 'isBuiltin')
|| !$partialType->isBuiltin()

Check warning on line 262 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L260-L262

Added lines #L260 - L262 were not covered by tests
) {
continue;

Check warning on line 264 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L264

Added line #L264 was not covered by tests
}
$foundBuiltinType = true;
$typeName = PHP_VERSION_ID >= 70100 ? $partialType->getName() : (string)$partialType;
$canBeArray |= $typeName === 'array';
$canBeString |= $typeName === 'string';
if (is_array($param)) {
if ($canBeArray) {
break;

Check warning on line 272 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L266-L272

Added lines #L266 - L272 were not covered by tests
}
continue;

Check warning on line 274 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L274

Added line #L274 was not covered by tests
}

$filterResult = $this->filterParamByType($param, $typeName);
if ($filterResult !== null) {
return [$filterResult, true];

Check warning on line 279 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L277-L279

Added lines #L277 - L279 were not covered by tests
}
}
if (!is_array($param) && $canBeString) {
return [$param, true];

Check warning on line 283 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L282-L283

Added lines #L282 - L283 were not covered by tests
}
if ($canBeArray) {
return [(array)$param, true];

Check warning on line 286 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L285-L286

Added lines #L285 - L286 were not covered by tests
}
return [$param, $canBeString || !$foundBuiltinType];

Check warning on line 288 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L288

Added line #L288 was not covered by tests
}

/**
* Run the according filter_var logic for teh given type.
* @param string $param The value to filter.
* @param string $typeName The type name.
* @return mixed|null The resulting value, or null if validation failed or the type can't be validated.
*/
private function filterParamByType(string $param, string $typeName)
{
switch ($typeName) {
case 'int':
return filter_var($param, FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE);
case 'float':
return filter_var($param, FILTER_VALIDATE_FLOAT, FILTER_NULL_ON_FAILURE);

Check warning on line 303 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L303

Added line #L303 was not covered by tests
case 'bool':
return filter_var($param, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE);
}
return null;

Check warning on line 307 in framework/web/Controller.php

View check run for this annotation

Codecov / codecov/patch

framework/web/Controller.php#L307

Added line #L307 was not covered by tests
}

/**
* {@inheritdoc}
*/
Expand Down
48 changes: 48 additions & 0 deletions tests/framework/web/ControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use RuntimeException;
use Yii;
use yii\base\InlineAction;
use yii\web\BadRequestHttpException;
use yii\web\NotFoundHttpException;
use yii\web\Response;
use yii\web\ServerErrorHttpException;
Expand Down Expand Up @@ -329,5 +330,52 @@ public function testUnionBindingActionParams()
$args = $this->controller->bindActionParams($injectionAction, $params);
$this->assertSame('test', $args[0]);
$this->assertSame(1, $args[1]);

// test that a value PHP parsed to a string but that should be an int becomes one
$params = ['arg' => 'test', 'second' => '1'];

$args = $this->controller->bindActionParams($injectionAction, $params);
$this->assertSame('test', $args[0]);
$this->assertSame(1, $args[1]);
}

public function testUnionBindingActionParamsWithArray()
{
if (PHP_VERSION_ID < 80000) {
$this->markTestSkipped('Can not be tested on PHP < 8.0');
return;
}
// Use the PHP80 controller for this test
$this->controller = new FakePhp80Controller('fake', new \yii\web\Application([
'id' => 'app',
'basePath' => __DIR__,
'components' => [
'request' => [
'cookieValidationKey' => 'wefJDF8sfdsfSDefwqdxj9oq',
'scriptFile' => __DIR__ . '/index.php',
'scriptUrl' => '/index.php',
],
],
]));

$this->mockWebApplication(['controller' => $this->controller]);

$injectionAction = new InlineAction('array-or-int', $this->controller, 'actionArrayOrInt');
$params = ['foo' => 1];

try {
$args = $this->controller->bindActionParams($injectionAction, $params);
$this->assertSame(1, $args[0]);
} catch (BadRequestHttpException $e) {
$this->fail('Failed to bind int param for array|int union type!');
}

try {
$paramsArray = ['foo' => [1, 2, 3, 4]];
$args = $this->controller->bindActionParams($injectionAction, $paramsArray);
$this->assertSame([1, 2, 3, 4], $args[0]);
} catch (BadRequestHttpException $e) {
$this->fail('Failed to bind array param for array|int union type!');
}
}
}
5 changes: 5 additions & 0 deletions tests/framework/web/FakePhp80Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,9 @@ public function actionInjection(int|string $arg, int|string $second)
{

}

public function actionArrayOrInt(array|int $foo)
{

}
}
Loading