Seen in r660861. The sniff fires without any mercy on inherited methods:
interface RecentChange_saveHook { public function onRecentChange_save( $recentChange ); } class RecentChangeSaveHandler implements RecentChange_saveHook { public function onRecentChange_save( $recentChange ) { // Function name "onRecentChange_save" should use lower camel case } }
Like, I know, but there's nothing I can do to avoid that ¯\_(ツ)_/¯
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| LowerCamelFunctionsNameSniff: Ignore hook methods | mediawiki/tools/codesniffer | master | +131 -2 |
Change 681376 had a related patch set uploaded (by DannyS712; author: DannyS712):
[mediawiki/tools/codesniffer@master] LowerCamelFunctionsNameSniff: ignore hook methods
@Daimona the patch I sent fixes this for the code that implements these hooks, do we also want to suppress the warning on the interfaces themselves? Or should we leave it to make sure that new hooks being introduced don't have underscores in them?
I recognize that I haven't been sufficiently clear in the task description, but this issue is not just for hooks. The real point is that it's an inherited method. As such, the sniff should:
Its impossible for phpcs to know (in the general case) if a method is inherited on not, its just for hooks where the naming is simple that its possible to tell
Then I would argue that phpcs is not the right tool for enforcing style of method names. Perhaps it might be always disabled if the current class is a subclass of anything?
I think that in the absence of a replacement (eg with a Phan plugin) phpcs as is is better than nothing. If we disabled the sniff for all classes that were subclasses of anything, or implemented any interfaces, it would be far less useful, since (just a guess, no actual numbers) I assume a majority of classes in core and extensions are subclasses or implement interfaces. Is fixing this just for hooks good enough, even if its not perfect? What other inherited methods are there that don't follow the naming conventions?
I agree that it would be less useful. I don't have hard data about which inherited methods other than hooks aren't using camelcase though. Doing that for hooks is probably better than nothing, but I should also note that a phan plugin for this would be trivial, so it might be worth doing that as well.
Change 681376 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] LowerCamelFunctionsNameSniff: Ignore hook methods