Page MenuHomePhabricator

Require first-class callable syntax to be used for callbacks in MediaWiki code
Open, Needs TriagePublic

Description

First-class callable syntax $callable = $foo->bar( ... );, compared to expressing callables as strings Foo::bar or arrays [ $foo, 'bar' ], is nicer to read, better for static analysis, and makes it easier to find uses of a method using simpler tools like grep.

I think we should enforce a coding convention for this using Phan. I hope it's possible to make it warn when a function/property/etc. that takes callable is given a string or array (even if that string or array is callable).

We could also just document these things with e.g. @param Closure instead of @param callable, which would have a similar effect. However, Closure as a native type annotation may cause problems with the callables that come from JSON data, and thus have to be strings/arrays. They can be converted with Closure::fromCallable(), but that may be too much. I just don't want to see the syntax [ $foo, 'bar' ] in PHP code, not necessarily forbid having callable arrays around, but it doesn't seem feasible to forbid at the syntax level (with PHPCS) without having lots of false positives/negatives.

Event Timeline

The code would have to be updated mostly by hand, and I guess it remains to be seen whether this would be worth it. Example change updating some simple cases, mostly [ $this, 'foo' ]: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1154989

There are some cases where the new syntax is not the same:

  • The callback method may not be accessible from the current scope, only from the caller's scope
  • The caller method may treat the parameter as a string/array and not an "opaque" callable – this seems rare in our code, but it happens sometimes, e.g. HookContainer::callableToString()
  • The caller method may serialize the parameter – use of PHP serialization is increasingly uncommon in our code, but not exactly rare yet, and it's hard to prove it never happens to a particular class
  • PHPUnit assertEquals() considers all closures equal (lol), so your tests may pass despite issues

Change #1160388 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/tools/phan@master] [WIP] Add FirstClassCallableRecommendPlugin

https://gerrit.wikimedia.org/r/1160388

I tried running my plugin's automated fixer on MediaWiki core, the result is here: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1160800

Feedback welcome. In particular, I wonder if it's helpful to replace references to built-in PHP functions like 'strlen' with the strlen( ... ) syntax – these ended up being slightly more than 50% of changes. I'm not sure if it's an improvement in readability, and several of them resulted in spurious Phan failures, since it now "knows too much" about the types of the callback parameters and return values and thinks they're wrong.

Change #1160800 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1160800

Change #1211148 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Increase minimum PHP version from 8.1.0 to 8.1.4

https://gerrit.wikimedia.org/r/1211148

Change #1160800 merged by jenkins-bot:

[mediawiki/core@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1160800

Change #1211148 merged by jenkins-bot:

[mediawiki/core@master] Increase minimum PHP version from 8.1.0 to 8.1.4

https://gerrit.wikimedia.org/r/1211148

This is now waiting for the Phan plugin/config patch to be merged and then released.

In particular, I wonder if it's helpful to replace references to built-in PHP functions like 'strlen' with the strlen( ... ) syntax – these ended up being slightly more than 50% of changes. I'm not sure if it's an improvement in readability, and several of them resulted in spurious Phan failures, since it now "knows too much" about the types of the callback parameters and return values and thinks they're wrong.

I ended up disabling the warnings/autofixes for this case by default in the config.

Change #1226214 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1226214

Change #1228545 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Cite@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1228545

Change #1228552 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Wikibase@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1228552

Change #1229171 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/CentralAuth@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1229171

Change #1228545 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1228545

Change #1229171 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1229171

Change #1226214 merged by jenkins-bot:

[mediawiki/core@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1226214

Change #1229606 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Use shorter fn() syntax for identity functions and such

https://gerrit.wikimedia.org/r/1229606

Change #1229606 merged by jenkins-bot:

[mediawiki/core@master] Use shorter fn() syntax for identity functions and such

https://gerrit.wikimedia.org/r/1229606

Change #1228552 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Use first class callable syntax in more places

https://gerrit.wikimedia.org/r/1228552