Closed (fixed)
Project:
Coder
Version:
6.x-2.x-dev
Component:
Review/Rules
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
21 Sep 2008 at 01:21 UTC
Updated:
25 Aug 2010 at 21:50 UTC
Occasionally when I'm working on a module, Coder gives me errors that are not accurate. This usually happens with either PHP's string functions (which, in many cases, are faster than the drupal_ versions) or when dynamically building SQL queries (and a safe string is inserted into a query using %sinstead of '%s').
Those errors are annoying, and get in the way when I'm trying to look at everything else. So it would be nice if there was a way to tell coder "don't check this line." It would also be a nice way to check sections of code as you're working on them if you still have some to-do items.
To do this, I propose a comment-based mechanism:
//If this appears immediately above a line of code OR on the same line as other code, don't check that line for rule violations. CL stands for "coder line."
//!CL
//This code can wrap around a block of code to tell Coder "don't check any of the code between these two lines." CB stands for "coder block" and -s and -e stand for "start" and "end," respectively.
//!CB-s
//!CB-e
So for example:
//This line would not be caught as a rule violation by Coder for using substr instead of drupal_substr.
$string1 = substr($string0, 1, 2); //!CL
//The same applies here, but the structure is different.
//!CL
$string1 = substr($string0, 1, 2);
//None of the code below would be validated by Coder.
//!CB-s
$string1 = substr($string0, 1, 2);
$string2 = substr($string3, 1, 2);
//!CB-e
I think there should be some exceptions to the rules that are not applied to code with these tags, however. Having the right number of spaces for each indent is one example.
Comments
Comment #1
douggreen commentedBecause php is an interpretive language, I'm really leary of "coder" directives, even in comments. I'd prefer to have a separate file that wasn't read by php, that contained these directives. I think that adding a button to coder, that allowed you to select acceptable warnings, save these in the database, and ultimately write this to a separate file that could ship with the module, would be ideal.
I've changed the version to 6.x, because 5.x is feature frozen.
Comment #2
icecreamyou commentedI considered a method where clicking on the error/warning icon would save that error in the database (and would thereafter not show up) but I thought that would end up causing a lot of overhead. Your point is well-taken though.
Comment #3
icecreamyou commentedI just installed Coder 2.x and was appalled by the "else if" to "elseif" enforcement, namely because the reasoning for preferring "elseif" is due to a syntax error in PHP that only occurs when using a structure that is against pre-existing and more solidified coding standards. It made me re-think the way this feature request should be approached.
I think it should be possible to enable or disable each of the rules that Coder runs. So rather than having minor/normal/critical levels for rules, each rule could be individually turned on or off in the coder settings. In theory it might also be nice to be able to group rules arbitrarily and enable/disable rule groups, although I think that's probably a step farther than it needs to be taken.
Comment #4
stella commentedI'm not convinced that this should be implemented, though I remain undecided. Rules which produce false positives can be changed to be 'minor' if necessary until they can be fixed. However, I disagree about allowing you to prevent the 'elseif' vs 'else if' check. You may not like the rule, but it is part of the Drupal coding standards now. If you wish to get the coding standards changed, then start a discussion in the http://groups.drupal.org/coding-standards-and-best-practices group on g.d.o.
Comment #5
icecreamyou commentedPerhaps a more valid use case will make the reason for such a feature more clear: let's say I'm taking over the maintainership of a module and I want to create a new branch that conforms to coding standards. If the previous maintainer used "else if" 73 times throughout 11 files, It would be useful to be able to temporarily disable the "else if" rule so I could immediately see that db_query() was used instead of update_sql() on line 249 of mymodule.install without scrolling through all the "else if" complaining. The advantage is that I'm able to find the really important errors faster, without having to read 73 other complaints that look very similar to the errors I'm really looking for.
One alternative would be to take the same approach that the W3C XHTML validator does and allow grouping by error type.
Comment #6
stella commentedNo, because in your use case, you wouldn't run the 'coding style' coder review - just the security or sql reviews.
Comment #7
icecreamyou commentedFair enough. What do you think about grouping by error type? It doesn't have a lot to do with my original request but it would make the distribution of errors clearer.
Comment #8
stella commentedWell there's already different reviews (coding style, sql, security, etc) which does the majority of grouping, while there's minor, major and critical levels. So that provides some categorization already - what additional sort of grouping are you suggesting?
Comment #9
icecreamyou commentedI don't mean in terms of categorization...
Currently, the list of errors shows up like this:
I'm proposing this structure:
Comment #10
stella commentedI've added this capability to 6.x-2.x and 7.x branches. You just need to create file called 'modulename.coder_ignores.txt' (or modulename.coder_review_ignores.txt for D7) in your module's directory and implement the new 'hook_coder_ignores()' hook - see coder.module for an example implementation for now until documentation is written.