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

Jump to bottom

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

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

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

weaverryan opened this issue Dec 3, 2016 · 8 comments
Labels
BC Break Bug HttpKernel Status: Needs Review

Comments

@weaverryan
Copy link
Member

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
Copy link
Contributor

linaori commented Dec 4, 2016 •
edited

@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
Copy link

AlkisStamos commented Dec 4, 2016 •
edited

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
Copy link
Contributor

linaori 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
Copy link
Member Author

@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
Copy link
Contributor

linaori commented Dec 5, 2016 via email

@linaori linaori mentioned this issue Dec 5, 2016
Merged
@fabpot
Copy link
Member

fabpot commented Dec 5, 2016

👍 for fixing this as a bug.

fabpot added a commit that referenced this issue Dec 5, 2016
@fabpot
bug #20755 [2.7][HttpKernel] Regression test for missing controller a…
e70dc64 
…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 as completed Dec 5, 2016
@linaori linaori mentioned this issue Dec 5, 2016
Merged
fabpot added a commit that referenced this issue Dec 5, 2016
@fabpot
bug #20756 [3.1][HttpKernel] Regression test for missing controller a…
68d5e0d 
…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
Copy link
Member Author

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

@linaori
Copy link
Contributor

linaori 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
Assignees
No one assigned
Labels
BC Break Bug HttpKernel Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @javiereguiluz @weaverryan @linaori @AlkisStamos @carsonbot