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

Inlining constructor parameters too aggresively when implementing interface #6883

Closed
rvanlaak opened this issue Dec 20, 2021 · 3 comments
Closed
Labels

Comments

@rvanlaak
Copy link

Bug Report

Subject Details
Rector version latest tag and dev-main

For the PHP 8.1 setlist, the constructor properties get inlined too aggressively in combination with readonly.

Minimal PHP Code Causing Issue

I can not reproduce this on the demo, as it does not seem to use the dev-main branch.

Suggestion how to fix

Make sure to check if the parent is an interface before .

Actual Behaviour

Cache warmup fails due to compilation error:

image

Diff that shows problem

  • The promoted DomainEventMetadata::fromArray([]) is no valid property instantiation.
  • Promotion of $metadata is not compatible with interface.
  • Only $occuredOn should have been promoted.
diff --git a/api/src/Domain/Shared/Event/AbstractDomainEvent.php b/api/src/Domain/Shared/Event/AbstractDomainEvent.php
index 1ac3776d..35450925 100644
--- a/api/src/Domain/Shared/Event/AbstractDomainEvent.php
+++ b/api/src/Domain/Shared/Event/AbstractDomainEvent.php
@@ -20,19 +20,11 @@ use Domain\Shared\EventStore\StoredEvent;
  */
 abstract class AbstractDomainEvent implements DomainEventInterface
 {
-    private DomainEventMetadata $metadata;
-    private \DateTimeInterface $occurredOn;
-
     /**
      * @param T $aggregateRootId
      */
-    public function __construct(
-        private $aggregateRootId,
-        ?DomainEventMetadata $metadata = null,
-        ?\DateTimeInterface $occurredOn = null,
-    ) {
-        $this->metadata = $metadata ?? DomainEventMetadata::fromArray([]);
-        $this->occurredOn = $occurredOn ?? new \DateTimeImmutable();
+    public function __construct(private $aggregateRootId, private readonly DomainEventMetadata $metadata = DomainEventMetadata::fromArray([]), private readonly \DateTimeInterface $occurredOn = new \DateTimeImmutable())
+    {
     }

     /**
~
~

Expected code

    # ...

    private DomainEventMetadata $metadata;

    /**
     * @param T $aggregateRootId
     */
    public function __construct(
        private $aggregateRootId,
        ?DomainEventMetadata $metadata = null,
        private readonly \DateTimeInterface $occurredOn = new \DateTimeImmutable(),
    ) {
        $this->metadata = $metadata ?? DomainEventMetadata::fromArray([]);
    }
@rvanlaak rvanlaak added the bug label Dec 20, 2021
@samsonasik
Copy link
Member

Could you create failing fixture for it, for single rule, you can add to https://github.com/rectorphp/rector-src/tree/main/rules-tests/Php81/Rector/ClassMethod/NewInInitializerRector/Fixture

For multi rules related, you can apply to https://github.com/rectorphp/rector-src/tree/main/tests/Issues, you can look at one of it, copy and modify based on your use case, eg: https://github.com/rectorphp/rector-src/tree/main/tests/Issues/ComplexReadOnly

@TomasVotruba
Copy link
Member

Thanks for reporting 👍

It seems the problem is in abstract class. It cannot contain promoted properties:

https://stitcher.io/blog/constructor-promotion-in-php-8#not-allowed-in-abstract-constructors

@samsonasik
Copy link
Member

resolved at rectorphp/rector-src#1581 , feel free to create failing test case if the issue persist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants