[HttpKernel] Fixed the nullable support for php 7.1 and below #19784
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| } | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function resolve(Request $request, ArgumentMetadata $argument) | ||
| { | ||
| if ($argument->isNullable()) { |
There was a problem hiding this comment.
yield $argument->isNullable() ? null : $argument->getDefaultValue();?
|
IMHO, if we can remove the deprecation, go for 3.1, otherwise, master. |
|
@nicolas-grekas alright, re-opening against the master, I can't avoid the argument it seems |
|
Reopened after removing BC break of #19785 |
| public function __construct() | ||
| { | ||
| $this->supportsVariadic = method_exists('\ReflectionParameter', 'isVariadic'); | ||
| $this->supportsParameterType = method_exists('\ReflectionParameter', 'getType'); |
|
Isn't it something that also needs to fixed on 2.7? |
|
@fabpot in the controller resolver, yes. Probably in the argument resolver as well (but would like to do that in another patch) Once it's fixed in here, I can probably backport a similar solution |
|
@iltar If possible, I would prefer to have the 2.7 patch so that we can merge it first, merge 2.7 into 2.8 then 3.1, and merge this PR immediately to resolve conflicts. |
|
@fabpot sounds like a plan, I'll get it ready when I have some extra time |
|
Thank you. |
This was referenced Sep 1, 2016
|
👍 |
fabpot
added a commit
that referenced
this pull request
Sep 14, 2016
…, 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 @fabpot Commits ------- 9c48756 Fixed the nullable support for php 7.1 and below
symfony-splitter
pushed a commit
to symfony/http-kernel
that referenced
this pull request
Sep 14, 2016
…, 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 | symfony/symfony#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 @fabpot Commits ------- 9c48756 Fixed the nullable support for php 7.1 and below
|
Thank you @iltar. |
fabpot
added a commit
that referenced
this pull request
Sep 14, 2016
…low (iltar) This PR was squashed before being merged into the 3.1 branch (closes #19784). Discussion ---------- [HttpKernel] Fixed the nullable support for php 7.1 and below | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19771 | License | MIT | Doc PR | ~ This PR gives support for for the new php 7.1 and will only work in beta3 or higher. I've had to backport the support to 3.1 because I consider this a bug that it won't work, even though 3.1 won't be supported for much longer. ~~The deprecation I've added in the `ArgumentMetadata` should not be triggered as all framework cases create it with the argument. Just for developers who for some reason implemented this manually, I've added the deprecation.~~ ~~*If needed, I can re-open this against 3.2 and leave 3.1 "broken"*~~ On 7.1 lower than beta3 this will happen but shouldn't affect any higher versions (I hope). ``` There was 1 failure: 1) Symfony\Component\HttpKernel\Tests\ControllerMetadata\ArgumentMetadataFactoryTest::testNullableTypesSignature Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ Array ( 0 => Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata Object (...) 1 => Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata Object ( 'name' => 'bar' - 'type' => 'stdClass' + 'type' => 'Symfony\Component\HttpKernel\Tests\Fixtures\Controller\stdClass' 'isVariadic' => false 'hasDefaultValue' => false 'defaultValue' => null 'isNullable' => true ) 2 => Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata Object (...) ) /home/ivanderberg/projects/symfony/src/Symfony/Component/HttpKernel/Tests/ControllerMetadata/ArgumentMetadataFactoryTest.php:123 ``` Commits ------- 4a1ab6d [HttpKernel] Fixed the nullable support for php 7.1 and below
Merged
fabpot
added a commit
that referenced
this pull request
Oct 4, 2016
This PR was merged into the 3.1 branch. Discussion ---------- [HttpKernel] Fix nullable types handling | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19784 | License | MIT | Doc PR | - Commits ------- 0884518 [HttpKernel] Fix nullable types handling
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR gives support for for the new php 7.1 and will only work in beta3 or higher. I've had to backport the support to 3.1 because I consider this a bug that it won't work, even though 3.1 won't be supported for much longer.
The deprecation I've added in theArgumentMetadatashould not be triggered as all framework cases create it with the argument. Just for developers who for some reason implemented this manually, I've added the deprecation.If needed, I can re-open this against 3.2 and leave 3.1 "broken"On 7.1 lower than beta3 this will happen but shouldn't affect any higher versions (I hope).