class classNameHere { public function funcNameHere() { $wgFoo = 'bar'; } }
Here, $wgFoo does not refer to a global variable, but rather a local one, and should not be named with a $wg prefix
Description
Description
Details
Details
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Add a sniff for `$wg*` variables that aren't globals | mediawiki/tools/codesniffer | master | +109 -0 |
Event Timeline
Comment Actions
Change 593617 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/tools/codesniffer@master] Add a sniff for $wg* variables that aren't globals
Comment Actions
Note this is not yet an "accepted rule change". I, personally, think it should be one. But let's please wait for a more official statement.
Comment Actions
@Daimona @thiemowmde @Jdforrester-WMF Based on the discussion regarding how to handle $GLOBALS, would it make sense add a sniff to warn when $GLOBALS is used, separate from the sniff for variable names?
Comment Actions
@thiemowmde This seems fine at glance. Do you have and/or could you gather some basic data to see whether this has existing violations? They might help reveal use cases we've missed or unfortunate name clashes we haven't foreseen.
Also, for the initial version, perhaps only sniff for local variables named wg[A-Z] not all wg*. Specifcally so that e.g. wg, wg2, wgea remain acceptable for now.
Comment Actions
Note sure about the data, but https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/593279/ fixed an example in core, and I just uploaded https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/598143/ to fix another. I'll look for more
- https://gerrit.wikimedia.org/g/mediawiki/services/parsoid/+/a79ff15a0ed4c6e07eab65b947020bf9740c943d/src/Wt2Html/TT/ParagraphWrapper.php has static class variables prefixed with $wg
- https://gerrit.wikimedia.org/g/mediawiki/extensions/Flow/+/9eb1ec71e127db99462843a98a9530a21862d404/includes/Exception/FlowException.php uses $wgOutBkp as a local variable (yes, its set to a copy of $wgOut and then used to reset $wgOut after its modified, but it isn't an actual global variable)
- https://gerrit.wikimedia.org/g/mediawiki/extensions/MassMessage/+/d040bb59e0b0c9550363cd91d22b5c1d0a16f517/tests/phpunit/MassMessageTestCase.php uses a local variable named $wgConf
- https://gerrit.wikimedia.org/g/mediawiki/extensions/UploadWizard/+/5ab0e9e53ef6dbe77b052074a93c4b69afe58180/tests/phpunit/ApiFlickrBlacklistTest.php uses a local variable named $wgUploadWizardConfig
While in many cases these variables hold the intended value for a global, they are not themselves globals
Comment Actions
Although $GLOBALS can lead to smelly code, I think there are cases when using $GLOBALS is wanted [I personally don't like it, but personal taste should be left out in this discussion, I believe]. At any rate, I think that should be discussed separately, i.e. not on this task.
Comment Actions
As argued on Gerrit I'm convinced this sniff should report all $wg… variables that have not been declared via a global $wg… statement before. Even if the variable does indeed reference a global, e.g. via $wg… = &$GLOBALS['wg…']. Such code is smelly and unnecessary and should be replaced with global $wg….
Discussing the use of $GLOBAL is misplaced here, in my opinion. I don't see a particular problem with code using $GLOBAL. It's not worse or better than usages of the global $… syntax. Both is a code smell, yes (every global is a code smell). But $GLOBAL and global are very easy to identify in code reviews. We don't need to actively block them.
… except we reach a state in which globals don't exist any more in the MediaWiki codebase. Maybe in a few years. ;-)
Comment Actions
The same should also apply to function parameters with $wg* names - the actual object passed may be the same as the global object, but that is up to the caller and the function shouldn't know about it (see, eg, https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/JsonConfig/+/604231/ )
Comment Actions
Change 593617 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Add a sniff for $wg* variables that aren't globals