Page MenuHomePhabricator

Create pre-commit PHP lint test
Closed, ResolvedPublic

Description

E_PARSE errors are never acceptable. But people don't like to test their code.

So pre-commit should run 'php -l' on php source files so people can't commit if they mess up.

I'll get to this sometime soon-ish.


Version: unspecified
Severity: enhancement

Details

Reference
bz26172

Event Timeline

bzimport raised the priority of this task from to High.Nov 21 2014, 11:24 PM
bzimport set Reference to bz26172.

Done, now checks all .php or .inc or .php5 files changed in a commit for syntax validity.

Should output the following on failure:

$ svn ci -m 'Try #2'
Sending test.php
Transmitting file data .svn: Commit failed (details follow):
svn: Commit blocked by pre-commit hook (exit code 1) with output:
PHP Parse error: syntax error, unexpected T_STRING in - on line 1


PHP error in: test.php:
Errors parsing test.php


Not that I'm adversed to this, but I thought that we didn't want to do a pre-commit hook as it blocks? And would be especially annoying for large (i18n) etc commits...?

Also, what if the file already had it in...? (Granted, if it was tested, they would've found it..)

Can we unleash checkSyntax.php in its fury on files committed?

PHP 5.3.2-1ubuntu4.5 with Suhosin-Patch (cli) (built: Sep 17 2010 13:49:46)
Copyright (c) 1997-2009 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2010 Zend Technologies

(In reply to comment #4)

Which php version is used?

5.3.2.

(In reply to comment #2)

Not that I'm adversed to this, but I thought that we didn't want to do a
pre-commit hook as it blocks?

I don't like the idea of doing extensive pre-commit hooks due to that issue. But php -l is typically very fast.

And would be especially annoying for large (i18n)
etc commits...?

It may be, we're not sure yet. Gonna test it later with Raymond.

Also, what if the file already had it in...? (Granted, if it was tested, they
would've found it..)

Then you fix it too, while you're at it ;-)

(In reply to comment #5)

Can we unleash checkSyntax.php in its fury on files committed?

checkSyntax.php is slower and checks lots of non-syntax stuff too. That would be better as a post-commit hook.

Commit your script to SVN?

In lint.php..

  • Recursive directory crawling PHP syntax checker
  • Uses parsekit, which is much faster than php -l for lots of files due to the

(In reply to comment #9)

Commit your script to SVN?

I need to clean up the pre/post commit hooks and put them somewhere. Puppet makes more sense than SVN, though.

In lint.php..

  • Recursive directory crawling PHP syntax checker
  • Uses parsekit, which is much faster than php -l for lots of files due to the

Yes, it's faster, but it's horrendously broken on 5.3.x last I played with it.

(In reply to comment #9)

Commit your script to SVN?

maintenance/checkSyntax.php ?

(In reply to comment #8)

checkSyntax.php is slower and checks lots of non-syntax stuff too. That would
be better as a post-commit hook.

It isn't much slower on a known list of files. A few regexes is faster than php
-l and even parsekit. And some stuff it checks for worth declining on
sight, such as common causes of "headers already sent" errors.

Also, it avoids using parsekit on PHP 5.3 where it's broken.

(In reply to comment #12)

(In reply to comment #8)

checkSyntax.php is slower and checks lots of non-syntax stuff too. That would
be better as a post-commit hook.

It isn't much slower on a known list of files. A few regexes is faster than php
-l and even parsekit. And some stuff it checks for worth declining on
sight, such as common causes of "headers already sent" errors.

Also, it avoids using parsekit on PHP 5.3 where it's broken.

We should rewrite it to not depend on MW at all, then it'll be fast :D

This and the empty summary prevention REALLY need fixing :P

Fixed and re-deployed. See /trunk/tools/subversion/hooks.