10000 [HttpKernel] Added nullable support for php 7.1 and below by linaori · Pull Request #19785 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] Added nullable support for php 7.1 and below #19785

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.

Sign up for GitHub

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
Closed
wants to merge 2 commits into from
Closed

[HttpKernel] Added nullable support for php 7.1 and below #19785

wants to merge 2 commits into from

Conversation

linaori
Copy link
Contributor
@linaori linaori commented Aug 30, 2016
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #19771
License MIT
Doc PR ~

This PR gives support for for the new php 7.1 nullables and will only work in beta3 or higher. 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.

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 (...)
 )

GuilhemN
GuilhemN reviewed Aug 30, 2016
View reviewed changes
src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php
*/
private function isNullable(\ReflectionParameter $parameter)
{
if (PHP_VERSION_ID >= 70000) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you use method_exists here ? Because this constant may differ on hhvm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't hhvm supposed to correspond with a certain php version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarilly, for example hhvm can support some php7 features and set this constant to php5 (see this comment) and as we don't know how they are gonna support php7.1 features, i think it is safer to use method_exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/iltar/symfony/blob/d4466c0d58c41d9da4fd118f1e8973f375d6bc97/src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php#L52
https://github.com/iltar/symfony/blob/d4466c0d58c41d9da4fd118f1e8973f375d6bc97/src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php#L105

Already doing here as well. Tbh I rather don't make hacks for hhvm.

Copy link
Contributor
@GuilhemN GuilhemN Aug 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that's a hack as it actually checks that the feature you need is available (it's imo less hacky than depending on a version). For the same reason, I think we should update the examples you're pointing as it could create unexpected behaviors.

But I'm not using hvvm so it won't impact me anyway, I just think it would be weird if it doesn't work sometimes when it should.
Moreover the solution to this is not complicated: class_exists($parameter, 'getType').

Maybe we could ask someone of the hvvm team if it's safe to depend on this constant for this feature?

Copy link
Member

Choose a reason for hiding this comment

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

method_exists is fast, let's use it imho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I update this for the other cases in this class as well while I'm at it? We do a lose a bit of verbosity of why this check is being done. The version fallbacks should definitely be removed in the future. How would you suggest it should be traced back?

I'm not familiar with HHVM or how they version it. Maybe this is a generic problem which should be solved framework wide?

Copy link
Member
@nicolas-grekas nicolas-grekas Aug 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

It's on a case by case basis I'd say. Logically, HHVM should expose a consistent value for PHP_VERSION_ID when it is compatible enough with it. But this is a moving target and sometimes (like here) we can use feature tests for better portability. So yes, let's check the existing places and see how this applies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been poking around on #hhvm and basically what I was told:

HHVM recommends doing feature detection, not version sniffing

I would really love to see a generic feature detection system which is determined either during container compilation or kernel boot-up

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean? This checks are only needed for php7 features and higher but we can't have something generic as hhvm allows to enable some features but not necessary all of them.

@fabpot
Copy link
Member
fabpot commented Aug 30, 2016

👍

HeahDude
HeahDude reviewed Aug 30, 2016
View reviewed changes
src/Symfony/Component/HttpKernel/Tests/Fixtures/Controller/NullableController.php

class NullableController
{
public function action(? string $foo, ? \stdClass $bar, ? string $baz = 'value', $mandatory)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for my minor comment but I don't know if a syntax has been defined yet for this, because I don't really like having a space after the ?. The RFC for now don't use any space.

Copy link
Member

Choose a reason for hiding this comment

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

Intuitively, I tend to agree with not having a space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's exactly how I had it, but this was fixed by fabbot.io :(

Copy link
Member

Choose a reason for hiding this comment

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

then fabbot needs a fix and we should remove the space here

@linaori linaori mentioned this pull request Aug 31, 2016
Closed
@linaori
Copy link
Contributor Author
linaori commented Aug 31, 2016

Closed in favor of #19784

@linaori linaori closed this Aug 31, 2016
@linaori linaori mentioned this pull request Sep 1, 2016
Merged
@linaori linaori deleted the feature/nullable-arguments branch February 8, 2019 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Deprecation Feature HttpKernel Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@linaori @fabpot @nicolas-grekas @ogizanagi @GuilhemN @HeahDude @carsonbot
0