PHP codesniffer (phpcs) - how to allow override when using as part of a svn pre-commit hook?

Question

We have a PHP 5 web application and we're currently evaluating PHP CodeSniffer in order to decide whether forcing code standards improves code quality.

We use subversion for our code repository and deployment base and I have added a SVN pre-commit hook to ensure all files committed are free from coding standard smells. The hook technically works but causes too many headaches to be actually useful:

1. If we have to fix an emergency bug that is causing a site outage, the last thing we need is the commit to be denied due to some minor whitespace indentation issue.
2. We have ALOT of legacy code that has sometimes hundreds of phpcs errors - it is not pragmatic to fix all the phpcs errors in these files right now. One example is a file full of functions that don't have doc comments. Another example is if a class name starts with a lowercase letter an error is thrown but fixing this might involve changing 10, 20+ files which would need need committing which would then be sniffed, recurse...
3. We have some files that are a bit large (e.g. 4000 lines of code?) and phpcs takes several minutes to check them. Delaying the commit by this long is unacceptable.
4. I haven't tested this yet but I imagine if you do a svn branch and commit it, phpcs will check everything and take a very long time to check all 1000 files?

Given that we can't refactor our whole codebase today - Does anyone know how I can use a svn commit parameter that will tell the svn pre-commit hook to not run phpcs?

Or perhaps there is another way to remove the headaches described?

Answer 1

Why run it on a pre-commit? I've used both PHPUnderControl and Hudson to automate php "builds"... Basically, they run an ant/phing build script which runs the automated tests (PHPUnit) and code quality scanners (including PHPCS) after every commit (automatically detected). So it won't reject the commit, but it will send a nice email to whomever you want that the build failed and list why (the specific lines of offending code)...

Answer 2

I think ircmaxell has a excellent point - this kind of standards checking should be somewhere other than a pre-commit hook, e.g. in a continuous integration envioronment or at a pinch, a post-commit hook and should be based around information providing rather than blocking commits!

Bearing this in mind, I decided for the moment, to use an opt-in approach. I configured the svn pre-commit hook to look in the commit message for a keyword and run phpcs if it was found.

In pre-commit hook script, e.g. /var/www/svn/repos/<reponame>/hooks/;

#!/bin/sh

REPOS="$1"
TXN="$2"
SVNLOOK=/usr/bin/svnlook
PHPCS=/usr/bin/scripts/phpcs-svn-pre-commit

if [[ `$SVNLOOK log -t $TXN $REPOS | tr "[:upper:]" "[:lower:]"` =~ "\[?standardcode\]?" ]]; then

  # Run the PHP code sniffer                                                                                                                     
  PHPCS_STRICT=`$PHPCS "$REPOS" -t "$TXN"`
  if [[ $? -ne 0 ]]; then
      echo "$PHPCS_STRICT" >>/dev/stderr
      echo "*** Commit blocked - Please fix coding standard errors." >>/dev/stderr
      exit 1
  fi
fi

exit 0

Notes:

• The keyword I chose was [standardcode] and the log message was converted to lowercase to make the keyword match case insensitive.
• The phpcs commit hook (/usr/bin/scripts/phpcs-svn-pre-commit) comes packaged with phpcs (at least in CentOS 5.5).

The idea is that a developer can choose to put the keyword in their commit message as a sort of badge of honour but they are not forced to have their code checked if it is not appropriate for their commit.

Answer 3

We have found the following works well, balancing the need to get code in without fuss, but preventing releases that diverge from our standards.

First, we have an "open arms" policy on commit:

• All code, working or not, conforming or not, accepted: so long as its commit log message has a bug tracking ID (pre-commit hook check)
• Encourages frequent commits & its benefits: collaboration, undo, backup

Then, we have a "clenched fist" policy on staging/release builds:

• Build fails if any, even trivial, deviation from rules, which means: mandatory syntax correctness, standards compliance, code documentation, and all tests pass with flying colors
• Prevents releasing anything buggy. If something buggy does get through, it's a problem with the code AND a problem with the build (process hole, not enough tests, etc.)

All of that is automated through phing using phpcs (and of course phpunit, phpdocumentor, etc).