-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Config] Fix signature generation with nested attributes on PHP 8.1 #43267
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
[Config] Fix signature generation with nested attributes on PHP 8.1 #43267
Conversation
38cfaef
to
c480435
Compare
c480435
to
1a728dd
Compare
Thanks for the PR! I spend some time on the topic this morning and shared our findings with Nikita. |
I will add a scenario for the attributes as well. I'll keep an eye on that PR from Nikita. |
@nicolas-grekas given the proposal of Nikita was merged, I'll wait for the next RC to come out and I'll validate if any implementation changes are still needed. If not, I'll still add the test cases for completeness. Thank you for looking into it! |
We should make this pass: --- a/src/Symfony/Component/Config/Tests/Resource/ReflectionClassResourceTest.php
+++ b/src/Symfony/Component/Config/Tests/Resource/ReflectionClassResourceTest.php
@@ -126,6 +126,10 @@ EOPHP;
yield [true, 0, '#[Foo]'];
}
+ if (\PHP_VERSION_ID >= 80100) {
+ yield [true, 0, '#[Foo(new MissingClass)]'];
+ }
+
yield [true, 1, 'abstract class %s'];
yield [true, 1, 'final class %s'];
yield [true, 1, 'class %s extends Exception'];
@@ -162,6 +166,10 @@ EOPHP;
yield [false, 9, 'private string $priv;'];
}
+ if (\PHP_VERSION_ID >= 80100) {
+ yield [true, 17, 'public function ccc($bar = new MissingClass()) {}'];
+ }
+
yield [true, 17, 'public function ccc($bar = 187) {}'];
yield [true, 17, 'public function ccc($bar = ANOTHER_ONE_THAT_WILL_NEVER_BE_DEFINED_CCCCCCCCC) {}']; |
946c557
to
d2ca566
Compare
@nicolas-grekas I validated that the php/php-src#7540 fixes the initially reported behavior, and I added changes to account for the scenarios you mentioned. However, the 7.4 tests are failing because something in the Core component. seems unrelated to my changes at first. Can you advice how to proceed? |
src/Symfony/Component/Config/Resource/ReflectionClassResource.php
Outdated
Show resolved
Hide resolved
$defaults[$p->name] = $p->getDefaultValue(); | ||
try { | ||
$defaults[$p->name] = $p->getDefaultValue(); | ||
} catch (\Throwable $e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here also a cast to string is the way to go on PHP 8.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately here this won't work.
In the test case where we expect line 13 to change:
- from
protected function prot($a = []) {}
- to
protected function prot($a = [123]) {}
it fails because the string casting loses the value of the array item, being it's output like this: Parameter #0 [ <optional> $a = Array ]
.
The string casting in the other places does work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is unexpected to me /cc @nikic wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for RC5 :)
See php/php-src@fb5cff1
d2ca566
to
15c950b
Compare
15c950b
to
6300a17
Compare
Thank you @agustingomes. |
And thank you @nikic! |
This fix allows the new PHP 8.1 syntax of new in initializers
It allows the method signature to be inferred from the object that is the default parameter value.