feat: add ConvertStaticToSelfRector

[calebdw]

Hello!

Summary

• Consolidated two similar Rector rules into a single ConvertStaticToSelfRector
• Added support for property access
• Enhanced rule logic to handle final classes, private members, and final members correctly
• Deprecated old rules with @deprecated, and delegate to new rule
• Enhanced conversion logic:
  • Final classes: Convert all static::* → self::*
  • Non-final classes: Convert only private static members and final static members

Thanks!

[TomasVotruba]

Hey, what's the difference to previous you've worked on?

[calebdw]

There's a big difference---there's several concerns going on here. I would have probably put most of these into a single rule, but the other classes were already created and I didn't want to make breaking changes, however, I can refactor existing rules if you like:

• static class access (static::class -> self::class)
• static method call (static::foo() -> self::foo())
• static constant access (static::FOO -> self::FOO)
• (this) static property access (static::$foo -> self::$foo)
  • it appears that there was never a rule created for this case

[TomasVotruba]

I see. So we have similar rule for class names, constants and method calls, but not for properties, right?

In that case ✔️

[TomasVotruba]

Is there any reason to have 3 different rules for these or would you recommend using 1 to ease the maintenance?

[calebdw]

If it was me I would put all of these into the same rule especially since the target the same node Class_ and have basically the same structure

[samsonasik]

@TomasVotruba static::class -> self::class require php version check (php 5.5) since that php 5.5 rule https://getrector.com/rule-detail/static-to-self-on-final-class-rector

[TomasVotruba]

@samsonasik I meant property, method call and class constants.

[calebdw]

@TomasVotruba, all of those rules (beside the class constants) only handle final classes, however, non-final classes with private methods/properties can also be updated

[TomasVotruba]

@calebdw I see. Would you recommend to use 1 rule? It's pretty confusing to me to have 3 rules that do 95 % the same checks.

[calebdw]

Yes, I would recommend 1 rule, that will both make it easier to maintain and easier for users to discover.

Would you like me to do that? Is it okay to remove/rename a rule on a minor version?

[samsonasik]

Imo, separare rules as for now is better, shared service that allow change based on type target is fine, but keep separate rules, the reason is user will allow step by step apply change that easier to review and merge.

[calebdw]

Personally, I think it's better to just review all the static:: to self:: changes in a class at once instead of wondering why some instances were changed and not others

[samsonasik]

The goal of levelling is to allow gradual step, and I prefer shared service, different rules, that easy to make upgrade code quality, easy to propose PR and merge, improve next level, If there is a bug, change the shared service only.

But of course, I will let @TomasVotruba decide.

[TomasVotruba]

Would you like me to do that? Is it okay to remove/rename a rule on a minor version?

@calebdw Yes, that would make more sense. That's why I was confused by this PR in the first place. Should be one rule to think about this logic :)

Go for it. Keep old rule class for BC and mark it with @deprecated annotation with link to a new rule.

[calebdw]

@TomasVotruba, @samsonasik this has been refactored into a single rule 👍

[TomasVotruba]

Awesome 😎

[samsonasik]

I prefer to not call rector rule (as service) in other rector rule.

Instead, use shared service so it correctly detected as "already called" as rule apply duplication no need verification.

This way, refresh scope and rule apply duplication become blurry and once we have bug in the future because of it, it will hard to verify.

[TomasVotruba]

I think we'll deprecate this rule soon enough with warning error instead.

Should be used as part of set anyway, so no need to add temporary complexity we'd revert.

[samsonasik]

The new rule already registered at set

https://github.com/rectorphp/rector-src/pull/7161/files#diff-13717e603efd6359b875e4e07c168ce004e8e9434fef929887c734014c018e01

just ensuring user that manually register old rules not got invalid apply rule duplication check because call rector rule as a service.

[samsonasik]

deprecate message is better imo, call rector from another rector seems dangerous since it is a visitor itself

[TomasVotruba]

@samsonasik Indeed, you're right. Could you add it?

Like we have in other rules:

[samsonasik]

sure, here the PR:

• Throw Deprecatation on StaticToSelfStaticMethodCallOnFinalClassRector and ConvertStaticPrivateConstantToSelfRector #7165

[TomasVotruba]

Thank you, let's ship this 👌

[github-actions]

This pull request has been automatically locked because it has been closed for 150 days. Please open a new PR if you want to continue the work.