Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.I use the Coder Review module to check standards whilst coding and it is a very helpful tool. However one warning still remains, caused by the class name in scheduler_handler_field_scheduler_countdown.inc
Line 17: Classes and interfaces should use UpperCamel naming. [style_class_names]
class scheduler_handler_field_scheduler_countdown extends views_handler_field {
We discussed this on #1566024: Coding Standards and Coder Review in this comment and decided to leave it as is. However, I was mistaken in that Coder Review would still complain at the base Views class name. If we fix the Scheduler class name to use UpperCamelCase this clears the warning.
With this correction and with other Coder fixes due to be committed, namely #2163269: Allow {@inheritdoc} in release standards review and #1834598: Coder_Review does not recognize @file docblock in .js files we get a completely clean Coder Review output - which makes it much easier and quicker to spot new errors introduced by code changes.
I propose that we fix the class name to match Drupal naming standards, even if the class it extends is named incorrectly. It is only used in one place, in a file unaffected by the imminent changes for #2170353: Reduce memory footprint
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 2370931-9.countdown_class_name.patch | 1.84 KB | jonathan1055 |
| |||
Comments
Comment #1
jonathan1055 CreditAttribution: jonathan1055 commentedJust coded this change, and it breaks existing views which use the handler :-(
There might be a way to write an update function to correct the views, though.
Comment #2
pfrenssenUnfortunately Views does not follow the Drupal coding standards regarding class names. It's best to keep the class names intact.
There is a solution to keep PHP CodeSniffer clean: we can provide our own ruleset that provides an "exclude pattern" that will instruct CodeSniffer to ignore the Views class names. A ruleset is an XML file that contains the rules that CodeSniffer follows. It is very handy since you can scan in full confidence (essential when using continuous integration) and it is easy to add new exceptions to the file.
When scanning the codebase, instead of doing this:
You would do this:
See here for an example phpcs-ruleset.xml. This is used by a project I have worked for. It contains an exception for this exact problem (see "Views handlers not strictly follow Drupal class name conventions").
You can discover which "Sniffs" to use by running phpcs with the "-s" option, this will show all sniffs that were failing. In this case the sniff is "Drupal.NamingConventions.ValidClassName". For example:
Comment #3
pfrenssenWith the latest versions of phpcs and the Coder Sniffer standards I'm getting many warnings :-/ Will do some cleanup.
Comment #4
jonathan1055 CreditAttribution: jonathan1055 commentedThanks for the info regarding rulesets. I have not used codesniffer or any of the command-line reviews in D7 (I did used the old style.pl in D6). I am currently using the Coder Review module from within my development site. I do not get any warnings (apart from the ones listed above for which there are patches for Coder review).
I tried to find where the view class names were stored in the db, but I think they are in binary blobs, which kind-of puts me off trying to write an update function for them.
Setting this issue back to active (so it does not appear in our current review sprint list), but will probably close it as 'wont fix' in due course.
Comment #5
pfrenssenAddressed coding standards violations that were found by the latest version of PHP CodeSniffer. I have not tested yet.
There are a few left which I decided to leave in:
$ phpcs --standard=Drupal --extensions='php,module,inc,install,test,profile,theme,js,css,info,txt' -s . FILE: /home/pieter/v/contrib/scheduler/scheduler.test -------------------------------------------------------------------------------- FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S) -------------------------------------------------------------------------------- 570 | WARNING | Only string literals should be passed to t() where possible | | (Drupal.Semantics.FunctionCall.NotLiteralString) --------------------------------------------------------------------------------These text strings are already correctly declared in scheduler.module so they can be picked up by the localization service without problems. This is just testing that the strings are being output correctly.
FILE: ...ter/v/contrib/scheduler/scheduler_handler_field_scheduler_countdown.inc -------------------------------------------------------------------------------- FOUND 5 ERROR(S) AFFECTING 4 LINE(S) -------------------------------------------------------------------------------- 17 | ERROR | Class name must begin with a capital letter | | (Drupal.NamingConventions.ValidClassName.StartWithCaptial) 17 | ERROR | Class name must use UpperCamel naming without underscores | | (Drupal.NamingConventions.ValidClassName.NoUnderscores) 44 | ERROR | Public method name | | "scheduler_handler_field_scheduler_countdown::option_definition" | | is not in lowerCamel format, it must not contain underscores | | (Drupal.NamingConventions.ValidFunctionName.ScopeNotLowerCamel) 54 | ERROR | Public method name | | "scheduler_handler_field_scheduler_countdown::options_form" is | | not in lowerCamel format, it must not contain underscores | | (Drupal.NamingConventions.ValidFunctionName.ScopeNotLowerCamel) 87 | ERROR | Public method name | | "scheduler_handler_field_scheduler_countdown::scale_filter_callback" | | is not in lowerCamel format, it must not contain underscores | | (Drupal.NamingConventions.ValidFunctionName.ScopeNotLowerCamel) --------------------------------------------------------------------------------This is all due to Views not adhering to coding standards, we will have to stick with these unfortunately.
FILE: /home/pieter/v/contrib/scheduler/scheduler.module -------------------------------------------------------------------------------- FOUND 1 ERROR(S) AFFECTING 1 LINE(S) -------------------------------------------------------------------------------- 750 | ERROR | Arguments with default values must be at the end of the argument | | list (PEAR.Functions.ValidDefaultValue.NotAtEnd) --------------------------------------------------------------------------------We're following the declaration of
date_popup_element_value_callback()which is not declared by us but by the date_popup module.Comment #6
pfrenssenLet's postpone this for after the release. This will also conflict heavily with #2170353: Reduce memory footprint. Leaving status to needs review for the moment to get test result.
Comment #7
jonathan1055 CreditAttribution: jonathan1055 commentedThanks for this. I've quickly skimmed over the patch, and there are lots of things there which Coder Review does not pick up. We'll need to decide exactly what our policy is on coding standards, and if we use CodeSniffer then I need to work out how to use it ;-)
Got your full set of passes with automated testing, so setting to postponed for now, but excellent work - thank you.
Comment #8
pfrenssenAll these were found using PHP CodeSniffer with the ruleset from Coder Sniffer. It's also part of the Coder module, but run from the command line. See Installing Coder Sniffer.
Comment #9
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedMinor change to commenting, but this is already no longer a problem. Clean on local, but worth checking on drupal.org, now that we have #3171011: Add phpcs.xml.dist file
Comment #11
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedAll OK. Fixed.