Bug #38477

Codesniffer breaks up due to a file delete

Added by Andy Grunwald 11 months ago. Updated 11 months ago.

Status:Closed Start date:2012-06-29
Priority:Should have Due date:
Assignee:Andy Grunwald % Done:

100%
Category:-
Target version:- Estimated time:1.00 hour
Votes: 0

Description

In Jenkins-Job 4619 the PHP_Codesniffer aborted due to an ERROR of a deleted file.

...
php -l typo3/sysext/statictemplates/media/scripts/wapversionLib.inc
No syntax errors detected in typo3/sysext/statictemplates/media/scripts/wapversionLib.inc
php -l typo3/sysext/statictemplates/media/scripts/xmlversionLib.inc
No syntax errors detected in typo3/sysext/statictemplates/media/scripts/xmlversionLib.inc
[typo3-v4-core-gerrit] $ ruby -v /tmp/hudson4016905562595591210.rb
ruby 1.8.7 (2010-01-10 patchlevel 249) [x86_64-linux]
Running PHP Codesniffer...
PHP Notice:  Undefined offset: 1 in /usr/share/php/PHP/CodeSniffer/CLI.php on line 322
PHP Notice:  Undefined offset: 2 in /usr/share/php/PHP/CodeSniffer/CLI.php on line 322
ERROR: The file "typo3/sysext/cms/tslib/media/fileicons/inc.gif" does not exist.

Usage: phpcs [-nwlsapvi] [-d key[=value]]
    [--report=<report>] [--report-file=<reportfile>] [--report-<report>=<reportfile>] ...
    [--report-width=<reportWidth>] [--generator=<generator>] [--tab-width=<tabWidth>]
    [--severity=<severity>] [--error-severity=<severity>] [--warning-severity=<severity>]
    [--config-set key value] [--config-delete key] [--config-show]
    [--standard=<standard>] [--sniffs=<sniffs>] [--encoding=<encoding>]
...

The question is why?
Why checks the codesniffer .gif-Files?
Why checks the codesniffer files, which are deleted?

Expected results:
The codesniffer runs on the changes / added php files.

History

#1
Updated by Christian Trabold 11 months ago

  • Status changed from New to Under Review
  • Assignee set to Christian Trabold
  • % Done changed from 0 to 50

While investigating I found an issue with the Regular Expression in the file checker:

The checks considers all changed files in the changeset ending with "php" and "inc":

test_files = files.split.grep(/php|inc/)

In the given scenario we've got an image file called "inc.gif" and the grep matches.

I try to modify the pattern so it checks only the file extension. It also should only check files that have been added or modified and ignore deleted files.

#2
Updated by Andy Grunwald 11 months ago

Uh. Nasty bug.
I think the following regex will do the job:

test_files = files.split.grep(/\.(php|inc)$/)

The regex only checks files, which are end by ".php" or ".inc".
This is Perl regex.

#3
Updated by Christian Trabold 11 months ago

  • Status changed from Under Review to Resolved
  • % Done changed from 50 to 100
  • Estimated time set to 1.00

I patched the scripts for php -l and phpcs:

 1 diff --git a/phpcs.sh b/phpcs.sh
 2 index a15e6f0..114575a 100644
 3 --- a/phpcs.sh
 4 +++ b/phpcs.sh
 5 @@ -5,8 +5,11 @@ require 'rake'
 6 
 7  puts "Running PHP Codesniffer..." 
 8 
 9 -files = `git diff --name-only HEAD HEAD^`
10 -test_files = files.split.grep(/php|inc/)
11 +files = `git diff --name-only --diff-filter=ACMRTUXB HEAD HEAD^`
12 +test_files = files.split.grep(/\.(php|inc)$/)
13 +
14 +# Exit if no files need to be checked
15 +exit 0 unless test_files.size > 0
16 
17  # Check files
18  begin
19 diff --git a/phpl.sh b/phpl.sh
20 index e3df8f4..ec53b51 100644
21 --- a/phpl.sh
22 +++ b/phpl.sh
23 @@ -5,8 +5,8 @@ require 'rake'
24 
25  puts "Check for PHP-Syntax errors..." 
26 
27 -files = `git diff --name-only HEAD HEAD^`
28 -test_files = files.split.grep(/php|inc/)
29 +files = `git diff --name-only --diff-filter=ACMRTUXB HEAD HEAD^`
30 +test_files = files.split.grep(/\.(php|inc)$/)
31 
32  #test_files = FileList['**/*.php'].exclude("adodb/","contrib/","openid/lib/","class.nusoap.php")
33  test_files.each do |f|

This fixes two issues:

  1. Deleted files are ignored through --diff-filter option
  2. Only files with these extensions are checked with the improved regex
    • .php
    • .inc

#4
Updated by Christian Trabold 11 months ago

  • Assignee changed from Christian Trabold to Andy Grunwald

Andy: could you double check the change and then close the ticket. Thanks!

#5
Updated by Andy Grunwald 11 months ago

  • Status changed from Resolved to Closed

Looks great.
Thanks Christian!

Also available in: Atom PDF