Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Fixed the nullable support for php 7.1 and below (2.7, 2.8, 3.0)#19811
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| * @param callable $controller | ||
| * @param \ReflectionParameter[] $parameters | ||
| * | ||
| * @return mixed[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@return array The values to use when calling the controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds good to me, is there any reasonarray is preferred overmixed[]?
edit:The arguments to use instead ofThe values to use, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
more common to me :)
linaori commentedSep 1, 2016
@dunglashttps://3v4l.org/3Vb4g if this is how you would expect it to work, I'd say it's nullable as default as long as it doesn't have a default value. |
dunglas commentedSep 1, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@iltar looks good:https://3v4l.org/rn5gR |
linaori commentedSep 1, 2016
@dunglas the problem is that it's not really nullable anymore. For the old 2.7, 2.8 and 3.0 it's fine, but for the new variant (argument value resolver) this is not a good solution imo. If I ask if something is nullable, I refer to the |
dunglas commentedSep 1, 2016
@iltar but it will never happen because ofhttps://github.com/symfony/symfony/pull/19811/files#diff-8518ab757bc9acd6d5a874a2e2737502R133 or am I missing something? |
linaori commentedSep 1, 2016
@dunglas no, for this case it's fine, I'm referring tohttps://github.com/symfony/symfony/pull/19784/files#diff-d298199ac90f557ade8fffe47ab3f755R98 While not directly related to this PR, it's a small detail difference. |
| $controller =array(newNullableController(),'action'); | ||
| $this->assertEquals(array('foo',new \stdClass(),'value','mandatory'),$resolver->getArguments($request,$controller)); | ||
| } | ||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing blank line before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is fabbot.io skipping test files?
fabpot commentedSep 7, 2016
👍 |
fabpot commentedSep 14, 2016
Thank you@iltar. |
…, 3.0) (iltar)This PR was merged into the 2.7 branch.Discussion----------Fixed the nullable support for php 7.1 and below (2.7, 2.8, 3.0)| Q | A| ------------- | ---| Branch? | 2.7, 2.8, 3.0| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19784 (comment)| License | MIT| Doc PR | ~Fixes the nullable support for 2.7, 2.8 and 3.0, can probably be partially merged into 3.1 but not 100% sure./ping@fabpotCommits-------9c48756 Fixed the nullable support for php 7.1 and below
Fixes the nullable support for 2.7, 2.8 and 3.0, can probably be partially merged into 3.1 but not 100% sure.
/ping@fabpot