Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 3.7.2

Bug #19481 Generic.CodeAnalysis.UselessOverridingMethod does not provide type hints
Submitted: 2012-06-24 01:40 UTC
From: andygrunwald Assigned: squiz
Status: Wont fix Package: PHP_CodeSniffer (version 1.3.4)
PHP Version: 5.3.12 OS: MacOSX
Roadmaps: (Not assigned)    
Subscription  


 [2012-06-24 01:40 UTC] andygrunwald (Andy Grunwald)
Description: ------------ The sniff Generic.CodeAnalysis.UselessOverridingMethod dows not provide type hints. A method can overwrite his parent by just determine new type hints. One example was found in the TYPO3 source code. The two methods can be found here: Original code: http://git.typo3.org/TYPO3v4/Core.git? a=blob;f=t3lib/cache/frontend/class.t3lib_cache_frontend_abstr actfrontend.php;h=be574c8fb667f747aebc02a4dac892e33c2ad0 97;hb=HEAD#l57 Overwritten code: http://git.typo3.org/TYPO3v4/Core.git? a=blob;f=t3lib/cache/frontend/class.t3lib_cache_frontend_phpfr ontend.php;h=08066bb2e34e290bd8c5373bba06ec02f8510266; hb=HEAD#l44 Test script: --------------- class t3lib_cache_frontend_AbstractFrontend { public function __construct($identifier, t3lib_cache_backend_Backend $backend) { // Some code } } public function __construct($identifier, t3lib_cache_backend_PhpCapableBackend $backend) { parent::__construct($identifier, $backend); } Expected result: ---------------- The expected result will be, that no warning will be thrown, because the method overriding is not useless, due to other (specialized) type hints. Actual result: -------------- The actual result is that a warning will be thrown: FILE: ...src- git/t3lib/cache/frontend/class.t3lib_cache_frontend_phpfrontend. php -------------------------------------------------------------------------- ------ FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S) -------------------------------------------------------------------------- ------ 44 | WARNING | Useless method overriding detected -------------------------------------------------------------------------- ------

Comments

 [2012-06-28 06:34 UTC] squiz (Greg Sherwood)
-Status: Open +Status: Wont fix -Assigned To: +Assigned To: squiz
This is a valid bug, but the sniff cannot determine this, especially with classes across multiple files. When you see the code: public function __construct($identifier, t3lib_cache_backend_PhpCapableBackend $backend) { parent::__construct($identifier, $backend); } you obviously can't determine that $backend has a different type hint in the parent. So you either don't ever warn for anything because they might have a different type hint, or you just warn and let people ignore it if it is incorrect. I think in this case, the sniff will just continue to throw a warning as it was originally written and you just need to ignore this particular case. So you might want to consider either removing the sniff from your standard if you think it gives too many false positives, or change the severity of the message so it can be ignored more easily. <rule ref="Generic.CodeAnalysis.UselessOverridingMethod.Found"> <severity>1</severity> </rule> These will be hidden by default, but can be shown by running PHPCS like this: phpcs --warning-severity=1 ... These extra warnings can then be viewed during a code review rather than in automated tools. Or maybe you have 2 runs (one with reduced severity) that developers understand needs a proper review.
 [2012-07-02 00:34 UTC] andygrunwald (Andy Grunwald)
Okay. This can be a solution. But there can be a solution, if this sniff implements PHP_CodeSniffer_MultiFileSniff interface. In the process method, it can be possible to "store" all methods and type hints in a global storage and compare them. This solution is not very "clean", but it can be work. And in bigger projects there are much memory needed. Do you think this fix idea is bad?
 [2012-07-02 04:15 UTC] squiz (Greg Sherwood)
Yes, a multifile sniff would have a better chance of trying to figure out what classes extend/implement others, but the memory requirements of storing all that data would make it unusable. I'm definitely not going to change the existing sniff into a multifile sniff.