Skip to content
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

Behavior Change: Incorrect controller arguments no longer throw exception #20746

Closed
weaverryan opened this issue Dec 3, 2016 · 8 comments

Comments

Projects
None yet
6 participants
@weaverryan
Copy link
Member

commented Dec 3, 2016

Hi guys!

The commit sha: 9c48756 (cc @iltar) changed some behavior in all versions of Symfony, I think on accident.

Consider the following:

/**
 * @Route("/product/{id}")
 */
public function showAction($id2)
{
}

Previously, this would have thrown the following error:

Controller "SomeController::showAction" requires that you provide a value for the "$id2" argument (because there is no default value or because there is a non optional argument after this one).

But after this commit, it does not throw the error, and instead $id2 is set to null in all cases. The specific problem is this block: 9c48756#diff-8518ab757bc9acd6d5a874a2e2737502R135

Was this expected? It's a step back in DX, as you can now silently mess up your route param without being notified.

Cheers!

@linaori

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2016

@weaverryan I think this is an unintended side-effect. As the method name implies, it does allow null, but this only for cases where no type-hint is given. Is this only on php7 or also on php5?

edit: are 3.1 and 3.2 affected by this as well? They resolve this differently in the default setup

@AlkisStamos

This comment has been minimized.

Copy link

commented Dec 4, 2016

Hello
i've noticed this behavior on 3.1 and 3.2 but not on 3.0. It seems to be coming from the Symfony\Component\HttpKernel\Controller\ArgumentResolver\DefaultValueResolver::resolve method since it yields a value in any case (even null).
Changing the
yield $argument->hasDefaultValue() ? $argument->getDefaultValue() : null;
to
if(!$argument->hasDefaultValue()) { return null; } yield $argument->getDefaultValue();
Seems that it fixes this behavior cause the Symfony\Component\HttpKernel\Controller\ArgumentResolver checks for non \Generator instances.
Note: above fix may work only in php7 versions (https://wiki.php.net/rfc/generator-return-expressions).

@linaori

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2016

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/DefaultValueResolver.php#L30

This line should probably check whether a type is set, as: fooAction($something); allows to call: $controller->fooAction(null);

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2016

@iltar Yep, I'm using 5.6, so it definitely is an issue there.

It's tricky because now that there's been a release with the behavior change, we've sorta already accidentally broken BC. If we're going to unbreak it back, we should probably do it quickly - it's already been included in 2.7.19, 2.7.20 and 2.7.21, for example, for the 2.7 releases.

@iltar are you able to make a patch that does the check you're talking about?

Thanks :)

@linaori

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2016

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

👍 for fixing this as a bug.

fabpot added a commit that referenced this issue Dec 5, 2016

bug #20755 [2.7][HttpKernel] Regression test for missing controller a…
…rguments (iltar)

This PR was merged into the 2.7 branch.

Discussion
----------

[2.7][HttpKernel] Regression test for missing controller arguments

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20746
| License       | MIT
| Doc PR        | ~

This fix should ensure that when an action has a mandatory parameter without a type, it will throw the exception instead of inserting null.

This test was missing when adding nullable support in 2.7 and up (probably has to be added to 3.1 as well).

Commits
-------

d1a7164 Regression test for missing controller arguments

@fabpot fabpot closed this Dec 5, 2016

fabpot added a commit that referenced this issue Dec 5, 2016

bug #20756 [3.1][HttpKernel] Regression test for missing controller a…
…rguments (iltar)

This PR was merged into the 3.1 branch.

Discussion
----------

[3.1][HttpKernel] Regression test for missing controller arguments

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20746
| License       | MIT
| Doc PR        | ~

Same fix as #20755 but for 3.1 and up as the new feature was hit by the same bug.

ping @fabpot

Commits
-------

9e588b8 Regression test for missing controller arguments (3.1)
@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2016

Wow! That was crazy fast fix and merge - thank you @iltar!

@linaori

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2016

@weaverryan I feel responsible for changes that I contribute to and thus find it important to provide some after-care when I can

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.