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

Required attribute should remove readonly properties errors #360

Closed
SVillette opened this issue Jul 26, 2023 · 8 comments
Closed

Required attribute should remove readonly properties errors #360

SVillette opened this issue Jul 26, 2023 · 8 comments

Comments

@SVillette
Copy link

Description

Currently, PHPStan reports an error when a readonly property is not initialized and another error when not initialized in a constructor.

  • Class App\Foo has an uninitialized readonly property $validator. Assign it in the constructor.
  • Readonly property App\Foo::$validator is assigned outside of the constructor.

But with the attribute #[Required] the DIC automatically call this method to initialize the property.

I think it could be a nice feature to remove these errors in that case.

Example

trait ValidationAwareCommandTrait
{
    private readonly ValidatorInterface $validator;

    #[Required]
    public function setValidator(ValidatorInterface $validator): void
    {
        $this->validator = $validator;
    }
}

Playground

https://phpstan.org/r/2ba9746d-bafd-4433-bf13-bcf3b15fc940

@ondrejmirtes
Copy link
Member

For sure this doesn't work in any class, right? What's the requirement for this to work somewhere? Does the class need to be a DI service?

@SVillette
Copy link
Author

SVillette commented Aug 7, 2023

The service has to be part of the DIC to use the attribute. There are multiple types of injections as the following docs explain (the doc will explain it better than me).

I think the setter injection is the most popular to deal with optional dependencies.

Also, the example was using traits which is not documented in the Symfony docs, but works too.

@ondrejmirtes
Copy link
Member

Yeah, I don't think we need to read the DIC for registered services in the first version, we just need to get rid of the false positives.

Feel free to contribute this, here's the guide: https://phpstan.org/developing-extensions/always-read-written-properties

@SVillette
Copy link
Author

SVillette commented Aug 8, 2023

I think #348 may fix half of the issue (first error).
But is there any extension point to prevent the second error (Readonly property App\Foo::$validator is assigned outside of the constructor.) to be triggered ? As it was made by purpose according to this comment.

@ondrejmirtes
Copy link
Member

But is there any extension point to prevent the second error

There isn't. Just remove the readonly keyword from those properties, it doesn't have any benefit in that case.

@SVillette
Copy link
Author

I disagree on that point. The docs mention that

The setter can be called more than once, also long after initialization, so you cannot be sure the dependency is not replaced during the lifetime of the object (except by explicitly writing the setter method to check if it has already been called).

Checking that the setter has been called or not can be done by setting the property readonly.

@SVillette
Copy link
Author

I guess this one can be closed now @ondrejmirtes. This error is not matched anymore in my project.

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants