Skip to content

Commit

Permalink
ComponentReflection::combineArgs() throws InvalidArgumentException in…
Browse files Browse the repository at this point in the history 
…stead BadRequestException when incompatible type is object
  • Loading branch information
@dg
dg committed Sep 6, 2016
1 parent 5d211e0 commit 212ec43
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 9 deletions.
5 changes: 3 additions & 2 deletions src/Application/UI/ComponentReflection.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,11 @@ public static function combineArgs(\ReflectionFunctionAbstract $method, $args)
foreach ($method->getParameters() as $i => $param) {
$name = $param->getName();
list($type, $isClass) = self::getParameterType($param);
$exception = $isClass ? Nette\InvalidArgumentException::class : BadRequestException::class;
if (isset($args[$name])) {
$res[$i] = $args[$name];
if (!self::convertType($res[$i], $type, $isClass)) {
throw new BadRequestException(sprintf(
throw new $exception(sprintf(
'Argument $%s passed to %s() must be %s, %s given.',
$name,
($method instanceof \ReflectionMethod ? $method->getDeclaringClass()->getName() . '::' : '') . $method->getName(),
Expand All @@ -143,7 +144,7 @@ public static function combineArgs(\ReflectionFunctionAbstract $method, $args)
} elseif ($type === 'NULL') {
$res[$i] = NULL;
} else {
throw new BadRequestException(sprintf(
throw new $exception(sprintf(
'Missing parameter $%s required by %s()',
$name,
($method instanceof \ReflectionMethod ? $method->getDeclaringClass()->getName() . '::' : '') . $method->getName()
Expand Down
8 changes: 4 additions & 4 deletions tests/UI/ComponentReflection.combineArgs.php7.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,17 +190,17 @@ test(function () {

Assert::exception(function () use ($method) {
Reflection::combineArgs($method, []);
}, BadRequestException::class, 'Missing parameter $req required by MyPresenter::objects()');
}, Nette\InvalidArgumentException::class, 'Missing parameter $req required by MyPresenter::objects()');

Assert::exception(function () use ($method) {
Reflection::combineArgs($method, ['req' => NULL, 'opt' => NULL]);
}, BadRequestException::class, 'Missing parameter $req required by MyPresenter::objects()');
}, Nette\InvalidArgumentException::class, 'Missing parameter $req required by MyPresenter::objects()');

Assert::exception(function () use ($method) {
Reflection::combineArgs($method, ['req' => $method, 'opt' => NULL]);
}, BadRequestException::class, 'Argument $req passed to MyPresenter::objects() must be stdClass, ReflectionMethod given.');
}, Nette\InvalidArgumentException::class, 'Argument $req passed to MyPresenter::objects() must be stdClass, ReflectionMethod given.');

Assert::exception(function () use ($method) {
Reflection::combineArgs($method, ['req' => [], 'opt' => NULL]);
}, BadRequestException::class, 'Argument $req passed to MyPresenter::objects() must be stdClass, array given.');
}, Nette\InvalidArgumentException::class, 'Argument $req passed to MyPresenter::objects() must be stdClass, array given.');
});
6 changes: 3 additions & 3 deletions tests/UI/ComponentReflection.combineArgs.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@ test(function () {

Assert::exception(function () use ($method) {
Reflection::combineArgs($method, []);
}, BadRequestException::class, 'Missing parameter $req required by MyPresenter::objects()');
}, Nette\InvalidArgumentException::class, 'Missing parameter $req required by MyPresenter::objects()');

Assert::exception(function () use ($method) {
Reflection::combineArgs($method, ['req' => NULL, 'opt' => NULL]);
}, BadRequestException::class, 'Missing parameter $req required by MyPresenter::objects()');
}, Nette\InvalidArgumentException::class, 'Missing parameter $req required by MyPresenter::objects()');

Assert::exception(function () use ($method) {
Reflection::combineArgs($method, ['req' => $method, 'opt' => NULL]);
}, BadRequestException::class, 'Argument $req passed to MyPresenter::objects() must be stdClass, ReflectionMethod given.');
}, Nette\InvalidArgumentException::class, 'Argument $req passed to MyPresenter::objects() must be stdClass, ReflectionMethod given.');
});

14 comments on commit 212ec43

@matej21
Copy link
Contributor

Choose a reason for hiding this comment

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

hi, what is the reason? Now app will show 500 instead of 404 page

@hrach
Copy link
Contributor

@hrach hrach commented on 212ec43 Nov 11, 2016

Choose a reason for hiding this comment

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

It's also a BC break.

@dg
Copy link
Member Author

@dg dg commented on 212ec43 Nov 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

When an incompatible object is passed to presenter, it seems like logic exception, ie 500, not like 404. Or am I wrong?

@JanTvrdik
Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on how you define responsibility of router. Now you're forcing users to increase complexity of router to analyze target presenter via reflection.

@dg
Copy link
Member Author

@dg dg commented on 212ec43 Nov 14, 2016

Choose a reason for hiding this comment

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

Can you give me example?

@JanTvrdik
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh shit, this is still a problem and now we have it in production.

The example is rather obvious. You have a presenter with a class typehint (e.g. Article $article) and you have a general-purpose EntityFilteringRouter (wrapper around another IRouter) which converts parameter article from integer (article id) to an instance of Article without analyzing the target presenter signature via reflection.

IMHO for the same reason the routers are currently not responsible to ensure that parameter article is an integer when presenter has int $article, the should not be responsible to ensure that parameter article is a correct instance, i.e. I don't understand why int and Article typehint should be treated any differently.

@dg
Copy link
Member Author

@dg dg commented on 212ec43 Jan 24, 2017

Choose a reason for hiding this comment

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

Because 4xx errors are usually not logged (ie not logged as logic errors), this commit helps you to discover logic error in code. 👍

@JanTvrdik
Copy link
Contributor

Choose a reason for hiding this comment

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

@dg But this is where we disagree. I don't think this is a logic error. Please read the comment once again. I don't understand why int and Article typehint should be treated any differently.

@dg
Copy link
Member Author

@dg dg commented on 212ec43 Jan 24, 2017

Choose a reason for hiding this comment

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

Because user can fake URL to generate scalar or array parameter, but is unable to fake object.

@dg
Copy link
Member Author

@dg dg commented on 212ec43 Jan 24, 2017

Choose a reason for hiding this comment

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

Object can create only your router, so it is your logic bug.

@JanTvrdik
Copy link
Contributor

@JanTvrdik JanTvrdik commented on 212ec43 Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No, logic bug = the router failed to fulfill his responsibility. But why should router be responsible for verifying type of parameters with class typehints and not for verifying type of parameters with scalar typehints?

If you change ComponentReflection to always throw InvalidArgumentException, it would be very odd, but consistent. It would mean that you have decided to move responsibility of parameter checking from presenter to router. But what you have done is in my view very inconsistent responsibility split. Router used to be more are less independent of presenters. You have suddenly broken this ancient agreement.

@dg
Copy link
Member Author

@dg dg commented on 212ec43 Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Simply: I remember that I commited this fix when I found out that the previous behavior conceal very serious bug in my code. If we are moving towards more strict language and code, it is logical to eliminate things that can silently conceal serious errors.

@JanTvrdik
Copy link
Contributor

@JanTvrdik JanTvrdik commented on 212ec43 Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@dg But this is not about being more or less strict. This is about breaking IRouter API that has been stable for years in a way that introduces inconsistency to Nette overall design.

How about the following compromise (I'm not even sure myself whether it would be actually a good idea). Throw InvalidArgumentException when instance of different class / wrong type is given, throw BadRequestException when the argument is simply missing.

@dg
Copy link
Member Author

@dg dg commented on 212ec43 Jan 26, 2017

Choose a reason for hiding this comment

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

#173

Please sign in to comment.