shipmonk-rnd/phpstan-rules

EnforceReadonlyPublicPropertyRule false positive with property override

Opened this issue · 3 comments

Hi there!

In case of property override that rule fails, but PHP doesn't allow to add readonly modifier to overriden property.

42     Public property `timestamps` not marked as readonly.
         ✏️  Entities/Foo.php

It happened with Laravel Eloquent Model: https://github.com/laravel/framework/blob/ef76b3a4dc0616960a1532c29cad3b8c429f8597/src/Illuminate/Database/Eloquent/Concerns/HasTimestamps.php#L14C31-L14C31

Simple code example:

<?php

declare(strict_types=1);

namespace B2B\TCA;

abstract class VendorClass
{
    public $property = true;
}

final class User extends VendorClass
{
    public $property = false;
}
PHPStan output: 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

Line Foo.php


3 Multiple class/interface/trait is not allowed in single file
✏️ Foo.php
9 Property B2B\TCA\VendorClass::$property has no type specified.
✏️ Foo.php
9 Public property property not marked as readonly.
✏️ Foo.php
14 Property B2B\TCA\User::$property has no type specified.
✏️ Foo.php
14 Public property property not marked as readonly.
✏️ Foo.php


I think you should change the default value in constructor of the child.

But I think we can change the rule not to report when public prop is overridden. It would be reported in the parent anyway (if that one is analysed and is not in vendor).

I found this pretty hard to implement when considering the same issue with traits. I believe it should behave like this:

<?php

namespace EnforceReadonlyPublicPropertyRuleOverride;

trait MyTraitParent {
    public int $parentTraitProperty; // error: Public property `parentTraitProperty` not marked as readonly.
    public int $parentTraitPropertyOverridden; // error: Public property `parentTraitPropertyOverridden` not marked as readonly.
}

trait MyTrait {
    use MyTraitParent;

    public int $traitProperty; // error: Public property `traitProperty` not marked as readonly.
    public int $traitPropertyOverridden; // error: Public property `traitPropertyOverridden` not marked as readonly.
    public int $parentTraitPropertyOverridden;
}

abstract class MyParentClass {
    public int $parentPropertyOverridden; // error: Public property `parentPropertyOverridden` not marked as readonly.
    public int $parentProperty; // error: Public property `parentProperty` not marked as readonly.
}

class MyClass extends MyParentClass
{
    use MyTrait;

    public int $traitPropertyOverridden;
    public int $parentPropertyOverridden;
    public int $ownProperty; // error: Public property `ownProperty` not marked as readonly.
}

But detecting all the cases properly is non-trivial. If you are able to send PR with this testcase green, I'll accept it. But I dont want to invest that much time into this. Thanks for understanding.

I think you should change the default value in constructor of the child.

Unfortunately, I cannot.
It's a part of Laravel Eloquent and constructor cannot be used for that, because it's kinda finalish.
I'm not a big fan of overriding, but the framework force users to do that. 😞

Okay, I'll try to implement it by myself.

Also it would be great if the repository has hacktoberfest topic.