Skip to content

[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

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

agustingomes
Copy link
Contributor

@agustingomes agustingomes commented Sep 30, 2021

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #43260, part of #41552
License MIT
Doc PR -

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.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 1, 2021

Thanks for the PR! I spend some time on the topic this morning and shared our findings with Nikita.
He's proposing php/php-src#7540 and that'd be perfect for our use case!
This also needs to be taken into account for attributes. Note that since 8.1, ReflectionAttribute has a __toString method that we can use to compute their hash. Can you please update the PR accordingly?

@agustingomes
Copy link
Contributor Author

I will add a scenario for the attributes as well. I'll keep an eye on that PR from Nikita.

@agustingomes
Copy link
Contributor Author

agustingomes commented Oct 1, 2021

@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 nicolas-grekas changed the title Allow signature generation when methods use new in initializers [Config] Fix signature generation when methods use new in initializers Oct 2, 2021
@nicolas-grekas nicolas-grekas changed the title [Config] Fix signature generation when methods use new in initializers [Config] Fix signature generation with nested attributes on PHP 8.1 Oct 2, 2021
@nicolas-grekas
Copy link
Member

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) {}'];

@agustingomes agustingomes force-pushed the new-in-initializer-fix branch 2 times, most recently from 946c557 to d2ca566 Compare October 15, 2021 07:41
@agustingomes
Copy link
Contributor Author

@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?

$defaults[$p->name] = $p->getDefaultValue();
try {
$defaults[$p->name] = $p->getDefaultValue();
} catch (\Throwable $e) {
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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

@agustingomes agustingomes force-pushed the new-in-initializer-fix branch from d2ca566 to 15c950b Compare October 15, 2021 10:43
@nicolas-grekas
Copy link
Member

Thank you @agustingomes.

@nicolas-grekas nicolas-grekas merged commit 078df14 into symfony:4.4 Oct 29, 2021
@nicolas-grekas
Copy link
Member

And thank you @nikic!

@agustingomes agustingomes deleted the new-in-initializer-fix branch October 30, 2021 17:38
This was referenced Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants