PossiblyStaticClosure should sniff only inside classes? about php-coding-standards HOT 5 CLOSED

@AlexP11223 closures outside of classes can still be bound to objects if not static (see https://3v4l.org/ASFRq).

That is normally not really relevant: considering those closures should not contain $this, even if they are bound the result of calling them will be the same.

However, if you make it static, there are still a few (minor) benefits.

• static explicitly states that the closure must not be bound. Being explicit is always a good thing.
• Adding "static" to an existing closure is BC break. Imagine you have a closure without static. Someone decides to bind it for any reason. You later add static. The other person code breaks. Removing static, on the other hand, is not a BC break. So creating a closure with static offers a tiny benefit in terms of future compatibility.
• It prevents one to make some mistakes. Imagine that because of copy & paste there's accidentally a $this inside a closure. PHP will not complain at compile time, but will complain at runtime. If that closure is static, static analyzers, like PHPCS or Psalm or even the IDE, are able to discover that sort of mistakes before the code is executed.
• To bind a closure prevents the bound object to be garbage collected. Making it static, if is meant to be static, that just can't happen.

As I said, those are minor benefits, but it is also true that typing static is not a big deal either. This is why the sniff is triggered also for closures ouside classes: if one gets that warning from PHPCS, they can type static without thinking too much and forget about it.

That said, what's above is my opinion, but these are company's coding styles. So if there're consensus about not suggesting static for closures outside classes, let's just do it. If that is your opinion please ask our colleagues to express theirs, and I can take care of doing what majority of devs want (unless someone else doesn't want to take care also of the implementation).

from php-coding-standards.

To me does make sense to keep it despite PHPCompatibility since it sounds more our own rule and we shouldn't rely on a sort of inheritance.

It also make sense to keep it and the rare cases where it's not really needed we can simply use phpcs:ignore or phpcs:disable.
My reason is that after almost 3 years we are discussing it now. Just to point out the rarity of the exception.

from php-coding-standards.

Thank you for the enlightening writeup

from php-coding-standards.

If that closure is static, static analyzers, like PHPCS

It detects without static too, e.g. in my example above

- $plugin = $f->createPluginFromFilePath($mainFile);
+ $plugin = $this->createPluginFromFilePath($mainFile);

WARNING | Closures / anonymous functions only have access to $this if used within a class or when bound to an object using bindTo().
    |         | Please verify. (PHPCompatibility.FunctionDeclarations.NewClosure.ThisFoundOutsideClass)

from php-coding-standards.

I'm trying to ge ta clean status of issues for a stable release, so I'm closing this because no activity since months.

from php-coding-standards.