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

EnforceReadonlyPublicPropertyRule false positive with property override #162

Open
zlodes opened this issue Sep 21, 2023 · 3 comments
Open

Comments

@zlodes
Copy link

zlodes commented Sep 21, 2023

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


@janedbal
Copy link
Member

janedbal commented Oct 2, 2023

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).

@janedbal
Copy link
Member

janedbal commented Oct 3, 2023

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.

@zlodes
Copy link
Author

zlodes commented Oct 29, 2023

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.

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

No branches or pull requests

2 participants