NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[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.

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

Jump to bottom

Closed

linaori wants to merge2 commits intosymfony:masterfromlinaori:feature/nullable-arguments

Closed

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

linaori wants to merge2 commits intosymfony:masterfromlinaori:feature/nullable-arguments

Conversation

Copy link

Contributor

linaori commentedAug 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 theArgumentMetadata 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::testNullableTypesSignatureFailed 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 (...) )

carsonbot added Status: Needs Review HttpKernel Feature Deprecation labels

Aug 30, 2016

Iltar van der Berg added2 commits

August 30, 2016 10:55

Fixed the nullable support for php 7.1 and below

38b0239

Fixed deprecations in SecurityBundle

d4466c0

GuilhemN reviewed

Aug 30, 2016

View reviewed changes

src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php

		* @return bool
		*/
		privatefunctionisNullable(\ReflectionParameter$parameter)
		{

Copy link

Contributor

GuilhemNAug 30, 2016

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

linaoriAug 30, 2016

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

GuilhemNAug 30, 2016

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 (seethis comment) and as we don't know how they are gonna support php7.1 features, i think it is safer to usemethod_exists.

Copy link

ContributorAuthor

linaoriAug 30, 2016

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

GuilhemNAug 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

nicolas-grekasAug 31, 2016

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

ContributorAuthor

linaoriAug 31, 2016

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-grekasAug 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

ContributorAuthor

linaoriAug 31, 2016

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

GuilhemNAug 31, 2016

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.

Copy link

Member

fabpot commentedAug 30, 2016

👍

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

HeahDudeAug 30, 2016

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?. TheRFC for now don't use any space.