Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
38cfaef toc480435Comparec480435 to1a728ddComparenicolas-grekas commentedOct 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the PR! I spend some time on the topic this morning and shared our findings with Nikita. |
agustingomes commentedOct 1, 2021
I will add a scenario for the attributes as well. I'll keep an eye on that PR from Nikita. |
agustingomes commentedOct 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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! |
nicolas-grekas commentedOct 2, 2021
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 tod2ca566Compareagustingomes commentedOct 15, 2021
@nicolas-grekas I validated that thephp/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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| $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 :)
Seephp/php-src@fb5cff1
d2ca566 to15c950bCompare15c950b to6300a17Comparenicolas-grekas commentedOct 29, 2021
Thank you@agustingomes. |
nicolas-grekas commentedOct 29, 2021
And thank you@nikic! |
Uh oh!
There was an error while loading.Please reload this page.
This fix allows the new PHP 8.1 syntax ofnew in initializers
It allows the method signature to be inferred from the object that is the default parameter value.