Skip to content

[PHP 7.1] Latest master is crashing in ArgumentResolver #19677

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

Closed
mvrhov opened this issue Aug 19, 2016 · 26 comments
Closed

[PHP 7.1] Latest master is crashing in ArgumentResolver #19677

mvrhov opened this issue Aug 19, 2016 · 26 comments

Comments

@mvrhov
Copy link

mvrhov commented Aug 19, 2016

RuntimeException in ArgumentResolver.php line 77: Controller "AppBundle\Controller\DefaultController::indexAction()" requires that you provide a value for the "$request" argument (because there is no default value or because there is a non optional argument after this one).

@mvrhov mvrhov changed the title Latest master is crashing b/c of Argument resolver Latest master is crashing in ArgumentResolver Aug 19, 2016
@dunglas
Copy link
Member

dunglas commented Aug 19, 2016

Can you post a code snippet to reproduce the problem?

@mvrhov
Copy link
Author

mvrhov commented Aug 20, 2016

No need. Just use symfony-standard master.

P.S. It seems that this one is also PHP 7.1 specific

@mvrhov mvrhov changed the title Latest master is crashing in ArgumentResolver Latest master is crashing in ArgumentResolver PHP 7.1 Aug 20, 2016
@mvrhov mvrhov changed the title Latest master is crashing in ArgumentResolver PHP 7.1 [PHP 7.1] Latest master is crashing in ArgumentResolver Aug 20, 2016
@wouterj
Copy link
Member

wouterj commented Aug 20, 2016

I can't reproduce this on PHP 7.1.0-beta2 on Windows.

Status: works for me

@mvrhov
Copy link
Author

mvrhov commented Aug 20, 2016

I have beta3. Unfortunately I cannot run it through debugger as the upcoming xdebug 2.5 segfaults for me.

@wouterj
Copy link
Member

wouterj commented Aug 20, 2016

status: reviewed

I can reproduce this with PHP 7.1-beta3. However, this is probably a bug in PHP, as it's working as expected in all PHP versions except 7.1-beta3?

@mvrhov
Copy link
Author

mvrhov commented Aug 20, 2016

It might be. I have no crash if I switch back to 7.0. However symfony might do something that's not allowed anymore.

@wouterj
Copy link
Member

wouterj commented Aug 20, 2016

The problem is that in PHP 7.1-beta3, ReflectionParameter::getType() is returning a FQCN (with leading \), while we're === comparing it with Request::class, which does not include the leading \.

Reported the bug at PHP, to see if this can be reverted or at least more explicitely documented: https://bugs.php.net/bug.php?id=72906

@dunglas
Copy link
Member

dunglas commented Aug 20, 2016

The API Platform test suite also fails for other reasons with the latest 7.1 (it was green before the beta 3). PHP even segfault. The last beta looks buggy, I think we should only allow failure for 7.1 until it becomes more stable.

@mvrhov
Copy link
Author

mvrhov commented Aug 20, 2016

Take a look at the last comment from @nikic in php/php-src#2068 The BC seems to be intentional to support nullable types.

@wouterj
Copy link
Member

wouterj commented Aug 20, 2016

The feature will be reverted in PHP: https://bugs.php.net/bug.php?id=72906#1471710916

For now, please downgrade your PHP version to PHP 7.1.0-beta2 or PHP 7.0.9. This issue can be closed.

@mvrhov
Copy link
Author

mvrhov commented Aug 20, 2016

while we're === comparing it with Request::class, which does not include the leading

I'm going to take just this one out of the context as it probably should be solved in 3.2. The new nullable types would probably need to be supported in argumetresolver and through the framework as I imagine a lot of ppl will now use them. That's the main reason I'm even running the betas.

@linaori
Copy link
Contributor

linaori commented Aug 20, 2016

This class should probably receive a compatibility update for the nullable: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadataFactory.php

My guess is that it should give a default value with null, meaning hasDefaultValue() should do more than checking just return $parameter->isDefaultValueAvailable();. However, I expect PHP to keep this backwards compatible in 7.1.

@wouterj
Copy link
Member

wouterj commented Aug 20, 2016

@iltar thing is, ReflectionType#__toString() will prefix the FQCN with ? when the class is nullable. So the === check doesn't not make much sense anymore. I'm not sure if they are going to revert only the \ change, or also the ? change. If both are reverted, there is no work to do in Symfony. If only \ is reverted, support for ?Symfony\Component\HttpFoundation\Request should be added (e.g. rtrim($parameter->getType(), '?')).

This also applies to other places using ReflectionType#__toString().

@linaori
Copy link
Contributor

linaori commented Aug 20, 2016

I will try to find a 7.1-beta3 monday if I can so I can build a patch for it (please do remind me if you can)

@mvrhov
Copy link
Author

mvrhov commented Aug 21, 2016

This took unexpected turn. It seems that I wasn't clear enough. If they do revert the patch there is nothing to do from the symfony side, you are correct.
What I meant when I quoted the part of message is it would still be nice if symfony would support nullable types in 3.2 in "all" resolvers/container builder.... e.g. ?Foo $foo woudn't throw exception but null would be injected if necessary.

@wouterj
Copy link
Member

wouterj commented Aug 21, 2016

Everything has been reverted: php/php-src@8855a2c this can now be closed

@linaori
Copy link
Contributor

linaori commented Aug 21, 2016

@mvrhov can you open a new issue for that? I think this is something that has to be merged into 3.1 and 3.2.

@trowski
Copy link
Contributor

trowski commented Aug 21, 2016

As noted by @wouterj, I've reverted the changes so that ReflectionType::__toString() is now identical to 7.0, including not prepending a ? for nullable types. The method is now just an alias of ReflectionNamedType::getName().

ReflectionType::__toString() should be discouraged for code generation going forward, as it seems there's just not a way to add type features in a BC way. My attempt to incorporate nullable types in a way that would allow for even more complex types such as callable(?\Type\Name, ?bool) just caused too many problems. Instead of relying on __toString(), use the other ReflectionType methods and ReflectionNamedType::getName() to build syntax-valid code that supports nullable types.

@wouterj
Copy link
Member

wouterj commented Aug 28, 2016

@symfony/deciders this can be closed

@teohhanhui
Copy link
Contributor

Note that the revert commit has been reverted: php/php-src@f4e68a3

This should be reopened. And we should add tests for this.

@linaori
Copy link
Contributor

linaori commented Sep 5, 2016

@teohhanhui #19811 #19784

@wouterj
Copy link
Member

wouterj commented Sep 5, 2016

Only the ? nullable prefix is added back, the \ is still reverted (which was the bug reported in this issue)

@teohhanhui
Copy link
Contributor

PropertyInfo needs fixing too (ReflectionExtractor). Is there already a PR for that?

@dunglas
Copy link
Member

dunglas commented Sep 5, 2016

@teohhanhui PropertyInfo doesn't need a fix. It is already compatible with 7.1 because it uses ReflectionParameter::allowsNull: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php#L164-L166

@dunglas
Copy link
Member

dunglas commented Sep 5, 2016

@teohhanhui but it indeed needs an update for the return type.

@teohhanhui
Copy link
Contributor

@dunglas Please take a look at #19853

fabpot added a commit that referenced this issue Sep 14, 2016
…flectionType changes in PHP 7.1 (teohhanhui)

This PR was merged into the 2.8 branch.

Discussion
----------

[PropertyInfo] Make ReflectionExtractor compatible with ReflectionType changes in PHP 7.1

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19677 (comment)
| License       | MIT
| Doc PR        | N/A

Commits
-------

4a041e9 Make ReflectionExtractor compatible with ReflectionType changes in PHP 7.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants