Prohibit Rector from making Closures static

[HenkPoley]

Since Laravel tends to depend on lots of behind the scene magic, using static on calls to functions and closures tends not give what you want. Rector should not introduce these things.

I propose adding these somewhere sensible (maybe to all the LARAVEL_number and LARAVEL_CODE_QUALITY):

    $rectorConfig->skip([
        \Rector\Php70\Rector\StaticCall\StaticCallOnNonStaticToInstanceCallRector::class,
        \Rector\CodingStyle\Rector\Closure\StaticClosureRector::class,
        \Rector\CodingStyle\Rector\ArrowFunction\StaticArrowFunctionRector::class,
        \Rector\Php70\Rector\MethodCall\ThisCallOnStaticMethodToStaticCallRector::class,
    ]]);

[driftingly]

Links to rules:

https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#staticcallonnonstatictoinstancecallrector

https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#staticclosurerector

https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#staticarrowfunctionrector

https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#thiscallonstaticmethodtostaticcallrector

I'm open to something like this. Can you provide an example of where this has caused an issue?

[HenkPoley]

Oof, I don't think I have seen it not give problems.

So.. I suppose it is always when you do something with Laravel Eloquent Models inside a closure?

I think I've read Rector (?similar tool?) removing most of the automatic to static conversions in a recent or upcoming update.

[GeniJaho]

@driftingly @peterfox The rules mentioned above are part of sets in the core Rector repo, the Php 70, and the Coding Style sets. I found this old issue which probably has the same problem https://github.com/rectorphp/rector/issues/8087.

Their solution is to skip the rule for a file or the entire project.

Digging deeper, there are these two rules (StaticArrowFunctionRector and StaticClosureRector) that prefix the arrow functions and closures with static whenever possible. That is, if a closure does not need the $this reference, it can be made static. They occur quite frequently, at least in Laravel codebases. However, the RFC notes that we probably don't need them and get little benefit.

Static closures are rarely used: They're mainly used to prevent $this cycles, which make GC behavior less predictable. Most code need not concern itself with this.

For that reason, I suggest we skip these two rules in the Readme of the project and/or add a section with suggestions on rules to avoid in general. What do you think?

[peterfox]

Funnily enough, this is something I've already looked into a little recently and PHPStan will soon be able to detect when this is happening so actually, we'd just need to check for the param.

https://github.com/phpstan/phpstan/discussions/9302#discussioncomment-8924977

I agree though that there shouldn't be any rules we write in this repo that make the closures static. That's for other rules to do the transformation.