Skip to content
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

Promoted properties missing default value #182

Open
n-valverde opened this issue May 16, 2023 · 0 comments
Open

Promoted properties missing default value #182

n-valverde opened this issue May 16, 2023 · 0 comments
Labels
Bug Something isn't working

Comments

@n-valverde
Copy link

n-valverde commented May 16, 2023

Bug Report

Q A
Version(s) 4.11.0 (all versions affected since 4.5.0)

Summary

Promoted properties support was added by #114
For some reason, this is lacking support for promoted property default value, I am not sure if this was made on purpose or not, but adding this seems pretty straight forward

Full context of the issue: In a Symfony application, when configuring a service as lazy, this is generating a proxy under the hood (using friendsofphp/proxy-manager-lts). Hunting all deprecations on the road of upgrading PHP in my userland application, I've been faced with a notice about optional parameter being treated as required because of the parameter position. Problem is, this notice come from the generated proxy, but my userland code signature is correct. Let's consider this signature

class SomeService
{
    private LoggerInterface $logger;

    public function __construct(private SomeDependency $dependency, ?LoggerInterface $logger = null, private string $someOptionalConfig = "")
    {
        $this->logger = $logger ?? new NullLogger();
    }
}

That signature is a bit ugly but is technically correct, the problem is, when generating a proxy for this class, you end up with

class SomeService
{
    private LoggerInterface $logger;

    public function __construct(private SomeDependency $dependency, ?LoggerInterface $logger = null, private string $someOptionalConfig)
    {
        $this->logger = $logger ?? new NullLogger();
    }
}

$someOptionalConfig does not have its default value anymore, so the argument position becomes wrong because there is this optional $logger which is now treated as required.

Obviously I can change the userland code signature to fix that, but this behaviour feels unexpected

Current behavior

Promoted property default value is basically always ignored

How to reproduce

Slighlty changing the relevant test easily show the failure, see below

namespace LaminasTest\Code\Generator\TestAsset;

final class ClassWithPromotedParameter
{
    public function __construct(private string $promotedParameter = 'foobar') {
    }
}
namespace LaminasTest\Code\Generator;

class ClassGeneratorTest extends TestCase
{
    public function testClassWithPromotedParameterFromReflection(): void
    {
        $classGenerator = ClassGenerator::fromReflection(
            new ClassReflection(ClassWithPromotedParameter::class)
        );

        $expectedOutput = <<<EOS
namespace LaminasTest\Code\Generator\TestAsset;

final class ClassWithPromotedParameter
{
    public function __construct(private string \$promotedParameter = 'foobar')
    {
    }
}

EOS;

        self::assertEquals($expectedOutput, $classGenerator->generate()); // Fails
    }
}

Expected behavior

Generated class does include promoted property default value. Applying the below tiny patch makes the test pass, and ultimately fixes the root issue of generating proxy classes from Symfony config, but again I am not sure if this was initially made on purpose

Index: src/Generator/PromotedParameterGenerator.php
===================================================================
diff --git a/src/Generator/PromotedParameterGenerator.php b/src/Generator/PromotedParameterGenerator.php
--- a/src/Generator/PromotedParameterGenerator.php	(revision 169123b3ede20a9193480c53de2a8194f8c073ec)
+++ b/src/Generator/PromotedParameterGenerator.php	(date 1684241975636)
@@ -28,12 +28,13 @@
         ?string $type = null,
         string $visibility = self::VISIBILITY_PUBLIC,
         ?int $position = null,
-        bool $passByReference = false
+        bool $passByReference = false,
+        mixed $defaultValue = null
     ) {
         parent::__construct(
             $name,
             $type,
-            null,
+            $defaultValue,
             $position,
             $passByReference,
         );
@@ -92,7 +93,8 @@
             $type,
             $visibility,
             $generator->getPosition(),
-            $generator->getPassedByReference()
+            $generator->getPassedByReference(),
+            $generator->getDefaultValue()
         );
     }
 }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant