Skip to content

Conversation

@samsonasik
Copy link
Member

In PHP 7.4 feature set, given the following code with new class extends usage:

final class AnonymousExtendsExistingClassInUnion
{
    private $x;

    public function __construct()
    {
        if (rand(0,1)) {
            $this->x = new \DateTime('now');
        } else {
            $this->x = new class extends \DateTime { };
        }
    }
}

it currently produce diff:

+    /**
+     * @var \DateTime|object|null
+     */
     private $x;

ref https://getrector.org/demo/7be14431-6643-4dd6-8edb-98f533cf9002

which the object is extends DateTime, which can be removed and use:

+    /**
+    * @var \DateTime|null
+    */

instead. This PR try to handle it.

@samsonasik
Copy link
Member Author

Fixed 🎉

}

return null;
}
Copy link
Member

@TomasVotruba TomasVotruba Dec 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

This rule should only change the strict type declarations and avoid docblocks.
Also this can report false positive change in case of docblock changed by another previous rule.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it called when only change docblock, see in line 131, no type changed, only change docblock, then check PhpDocInfo is changed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently stil mark DateTime|null as union type in UnionTypeMapper, not Nullable, so then we use docblock in PHP 7.4 feature set.

That need separate PR to make it nullable compatible DateTime|null marked as nullable type in PHP 7.4, ref https://3v4l.org/4HTQT#v7.4.33

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added another fixture for php 8.x feature enabled to remove existing docblock d12ba31

  • The default test is using php 7.4 feature, so it uses docblock ( union nullable still marked as union, not nullable)
  • when we use php 8.x feature on FixtureComplexTypes directory, it uses fully php 8.x feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to avoid changing docblocks in anyway, as they can be used later as relly on strict-type declarations.
Instead the code on PHP 7.4- for union types should be skipped, and strict union type added only in PHP 8.0.

That way we always add types that are known and strict 👍

@TomasVotruba
Copy link
Member

I'll merge this as a fix for extra object type 👍 Yet we should avoid the decorating docblocks if we can in the future.

@TomasVotruba TomasVotruba merged commit d3abaa1 into main Dec 6, 2022
@TomasVotruba TomasVotruba deleted the handle-anonymous-extends-existing-class-in-union branch December 6, 2022 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants