8000 PHP 8 support issue in handling of current() factories in DI · Issue #39737 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

PHP 8 support issue in handling of current() factories in DI #39737

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
Seldaek opened this issue Jan 6, 2021 · 5 comments
Closed

PHP 8 support issue in handling of current() factories in DI #39737

Seldaek opened this issue Jan 6, 2021 · 5 comments
Labels
Bug DependencyInjection Status: Reviewed

Comments

@Seldaek
Copy link
Member
Seldaek commented Jan 6, 2021

Symfony version(s) affected: 4.4.18

Description

symfony/src/Symfony/Component/DependencyInjection/Exception/InvalidParameterTypeException.php

Line 28 in 9b719ab

parent::__construct(sprintf('Invalid definition for service "%s": argument %d of "%s::%s()" accepts "%s", "%s" passed.', $serviceId, 1 + $parameter->getPosition(), $parameter->getDeclaringClass()->getName(), $parameter->getDeclaringFunction()->getName(), $acceptedType, $type));
does not support ReflectionParameter which are on a function and not a class. $parameter->getDeclaringClass()->getName() fails because getDeclaringClass returns null.

This only happens on PHP 8 because

symfony/src/Symfony/Component/HttpKernel/DependencyInjection/RegisterControllerArgumentLocatorsPass.php

Line 155 in fe91b86

->setFactory('current')
using current in PHP 8 will result in a parameter type hint being array|object, while PHP 7 did not have a type hint: https://3v4l.org/YKHtO and apparently Symfony also does not support union types as per @nicolas-grekas.

How to reproduce
I could not reduce it to a minimal repro case sorry
@Seldaek Seldaek added the Bug label Jan 6, 2021
@derrabus derrabus mentioned this issue Jan 7, 2021
Merged
@derrabus
Copy link
Member
derrabus commented Jan 7, 2021

#39746

@derrabus
Copy link
Member
derrabus commented Jan 7, 2021

apparently Symfony also does not support union types

I'd call it "passive support" for union types: If you can make Symfony 4.4 explode by using a union type in your application, this is a bug we need to fix. 😃

@derrabus
Copy link
Member
derrabus commented Jan 7, 2021 •
edited
Loading

#39746 fixes the exception constructor, but you said the exception should not be raised in the first place. A reproducer would really be appreciated here. The exception in question is raised by CheckTypeDeclarationsPass and I did fix this pass for union types in #39251. But that fix should've shipped with the 4.4.18 release.

@derrabus
Copy link
Member
derrabus commented Jan 7, 2021 •
edited
Loading

Forget what I just wrote. It's late and I should go to bed. 😉 The pass raises the exception internally and drops it if one of the types of the union matches. So #39746 should actually fix your problem.

nicolas-grekas added a commit that referenced this issue Jan 7, 2021
@nicolas-grekas
bug #39746 [DependencyInjection] Fix InvalidParameterTypeException fo…
a147705 
…r function parameters (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Fix InvalidParameterTypeException for function parameters

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #39737
| License       | MIT
| Doc PR        | N/A

Commits
-------

1854543 [DependencyInjection] Fix InvalidParameterTypeException for function parameters
@Seldaek
Copy link
Member Author
Seldaek commented Jan 7, 2021

Thanks for the quick fix @derrabus, I can confirm it works for me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DependencyInjection Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@Seldaek @nicolas-grekas @derrabus @xabbuh @carsonbot
0