Skip to content

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Dec 14, 2021

@TomasVotruba TomasVotruba changed the title remove private property only, to keep other rules work separately [TypedPropertyRector] Remove private property only, to keep other rules work separately Dec 14, 2021
@samsonasik
Copy link
Member

@TomasVotruba any other rule that handle it?

@samsonasik
Copy link
Member

@TomasVotruba I think it is handy to update code like in CodeIgniter4 codeigniter4/CodeIgniter4#5462 that want to update private property only.

@TomasVotruba
Copy link
Member Author

This rule was grouping 7 type infererrers together. It's too complex operation to hold them together, so they'll be split to 1 responsibility each in https://github.com/rectorphp/rector-src/tree/main/rules/TypeDeclaration/Rector/Property

  • TypedPropertyFromStrictConstructorRector
  • TypedPropertyFromStrictGetterMethodReturnTypeRector

The 2 doctrine ones will be moved to https://github.com/rectorphp/rector-doctrine

@TomasVotruba
Copy link
Member Author

@TomasVotruba I think it is handy to update code like in CodeIgniter4 codeigniter4/CodeIgniter4#5462 that want to update private property only.

If possible, we should cover this behavior inside the rules itself, without having an option for it.
E.g. strict type should not be added to public property by Rector, as it's pure guessing.

@samsonasik
Copy link
Member

If that needs to apply, then protected modifier should not be changed as well, as it can cause BC break with the following condition:

class A
{
    protected array $data = [];
}

class B extends A
{
    public function reset()
    {
        $this->data = null;
    }
}

(new B())->reset();

that will cause Fatal error:

Fatal error: Uncaught TypeError: Cannot assign null to property A::$data of type array in /in/EDR6K:12
Stack trace:
#0 /in/EDR6K(16): B->reset()

ref https://3v4l.org/EDR6K

@TomasVotruba
Copy link
Member Author

If that needs to apply, then protected modifier should not be changed as well, as it can cause BC break with the following condition:

Yes, that is also dangerous 👍
I've updated test fixute to allow final class + protected property, and skip everything else.

@TomasVotruba
Copy link
Member Author

@samsonasik Feel free to review & merge. I'm going out for a bit

@samsonasik samsonasik merged commit 790889c into main Dec 14, 2021
@samsonasik samsonasik deleted the tv-strict-decompose-3 branch December 14, 2021 16:48
@samsonasik
Copy link
Member

Thank you @TomasVotruba

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.

3 participants