Moodle

PHP CodeSniffer: tool for coding standard consistency

Details

  • Type: New Feature New Feature
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: 2.0
  • Fix Version/s: 2.0.8
  • Component/s: Administration
  • Labels:
    None
  • Affected Branches:
    MOODLE_20_STABLE
  • Fixed Branches:
    MOODLE_20_STABLE

Description

To help improve the consistency and readability of Moodle code, I've added a standalone version of PEAR PHP_CodeSniffer in lib/pear. I've also added a small CLI script called phpcs

The tool produces a report (per-file, summary or XML) of requested files, or of recursed directories.

I've modified the PEAR code slightly so that it will default to the Moodle standard. Other standards have been removed.

Another modification is to detect the presence of a "thirdpartylibs.xml" file in folders. This file is used to ignore any non-moodle code, because we won't be changing them so there is no need to report their errors. I've created one for lib/. This file could be used to help us manage the 3rd party library updates and track their licenses and versions.

Coding guidelines used to create the CodeSniffer Moodle Standard: http://docs.moodle.org/en/Development:Coding_style#Files

Issue Links

This issue will help resolve:
MDL-19235 Clean up and rationalise phpdocs and copyright statements across Moodle Critical Closed
This issue will be resolved by:
MDL-22192 Codesniffer wants a blank line between @param and @return in function comments - not mentioned in coding guidelines Minor Open
MDL-26067 CodeSniffer doesn't recognize $OUTPUT as a valid variable name. Minor Open
MDL-25733 CodeSniffer will die "randomly" if run on multiple files. Minor Open
MDL-22191 Codesniffer does not respect coding style guidelines re function comments Minor Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Petr Škoda (skodak) added a comment -

Are we allowed to import this PEAR lib into core?

Show
Petr Škoda (skodak) added a comment - Are we allowed to import this PEAR lib into core?
Hide
Permalink
Petr Škoda (skodak) added a comment -

Also if possible we should not be modifying the PEAR code at all - I know we did that in quickforms and excel lit, bud do we really need that here?

Show
Petr Škoda (skodak) added a comment - Also if possible we should not be modifying the PEAR code at all - I know we did that in quickforms and excel lit, bud do we really need that here?
Hide
Permalink
Martin Dougiamas added a comment -

The license is OK in this case (the three-clause modified BSD is compatible with GPLv2 and GPLv3).

I'm not 100% sure we need this in the core distribution, but do recognise that it'll probably mean more developers might use it and check it if it's there. I do wish you'd discussed this here before you went for the big check-in though. More and more PEAR libraries causes more license issues later on.

Assuming we keep it, that phpcs script must definitely not stay in the home directory. Put it with the library, I think. I'd take out the /usr/bin/php too and make people specify it manually, for a bit of extra safety.

Show
Martin Dougiamas added a comment - The license is OK in this case (the three-clause modified BSD is compatible with GPLv2 and GPLv3). I'm not 100% sure we need this in the core distribution, but do recognise that it'll probably mean more developers might use it and check it if it's there. I do wish you'd discussed this here before you went for the big check-in though. More and more PEAR libraries causes more license issues later on. Assuming we keep it, that phpcs script must definitely not stay in the home directory. Put it with the library, I think. I'd take out the /usr/bin/php too and make people specify it manually, for a bit of extra safety.
Hide
Permalink
Martin Dougiamas added a comment -

Also, none of your new code actually fits our coding guidelines!

Show
Martin Dougiamas added a comment - Also, none of your new code actually fits our coding guidelines!
Hide
Permalink
Martin Dougiamas added a comment -

I updated the README, moved and renamed phpcs => runsniffer script, and modifed some rules for line length and require statements

Show
Martin Dougiamas added a comment - I updated the README, moved and renamed phpcs => runsniffer script, and modifed some rules for line length and require statements
Hide
Permalink
Matt Gibson added a comment -

Just tried this in eclipse as follows (on Windows)

Run > external tools > external tools configuration...
Set up a new program with the following parameters:
location: C:\xampp\php\php.exe
arguments: ${workspace_loc:/assmoodle2/docroot/lib/pear/PHP/runsniffer} ${resource_loc}

but it didn't work till I added this bit to codesniffer/MoodleCLI.php around line 104:

if (isset($_SERVER['PWD'])) { $file = $_SERVER['PWD'] . '/' . $arg; } else { //windows $file = $arg; }

which is assuming an absolute file path is supplied.

I still got an error:
Notice: Undefined offset: 1 in C:\xampp\htdocs\assmoodle2\docroot\lib\pear\PHP\CodeSniffer\Standards\Moodle\Sniffs\Commenting\FileCommentSniff.php on line 579

so I altered around line 579 of that file to:

if (!preg_match('/\\\/', $filename)) { $path_parts = explode('/', $filename); } else { // windows $path_parts = explode('\\', $filename); }

It now works fine from within eclipse on Windows

Show
Matt Gibson added a comment - Just tried this in eclipse as follows (on Windows) Run > external tools > external tools configuration... Set up a new program with the following parameters: location: C:\xampp\php\php.exe arguments: ${workspace_loc:/assmoodle2/docroot/lib/pear/PHP/runsniffer} ${resource_loc} but it didn't work till I added this bit to codesniffer/MoodleCLI.php around line 104: if (isset($_SERVER['PWD'])) { $file = $_SERVER['PWD'] . '/' . $arg; } else { //windows $file = $arg; } which is assuming an absolute file path is supplied. I still got an error: Notice: Undefined offset: 1 in C:\xampp\htdocs\assmoodle2\docroot\lib\pear\PHP\CodeSniffer\Standards\Moodle\Sniffs\Commenting\FileCommentSniff.php on line 579 so I altered around line 579 of that file to: if (!preg_match('/\\\/', $filename)) { $path_parts = explode('/', $filename); } else { // windows $path_parts = explode('\\', $filename); } It now works fine from within eclipse on Windows
Hide
Permalink
Anthony Borrow added a comment -

Just a couple of thoughts for consideration. In CONTRIB, we currently do not have a way of distributing plugin libraries. I think that when licensing permits it and when we need to make modifications to the original code for Moodle functionality (as seems to be the case here) that storing optional libraries would be helpful. Having such libraries handled as other plugins could be helpful for packages as well when we have something like the JMOL resource type and JMOL filter that both use common files which could be stored as a library. Peace - Anthony

Show
Anthony Borrow added a comment - Just a couple of thoughts for consideration. In CONTRIB, we currently do not have a way of distributing plugin libraries. I think that when licensing permits it and when we need to make modifications to the original code for Moodle functionality (as seems to be the case here) that storing optional libraries would be helpful. Having such libraries handled as other plugins could be helpful for packages as well when we have something like the JMOL resource type and JMOL filter that both use common files which could be stored as a library. Peace - Anthony
Hide
Permalink
Anthony Borrow added a comment -

I would also like to see the ability to use something like codesniffer on CONTRIB code. Peace - Anthony

Show
Anthony Borrow added a comment - I would also like to see the ability to use something like codesniffer on CONTRIB code. Peace - Anthony

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.