Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 3.7.2

Bug #19648 Ignore patterns no longer work as expected in 1.4
Submitted: 2012-10-11 04:12 UTC
From: droch_trulia Assigned: squiz
Status: Closed Package: PHP_CodeSniffer (version 1.4.0)
PHP Version: 5.3.0 OS: Linux
Roadmaps: (Not assigned)    
Subscription  
Comments Add Comment Add patch


Anyone can comment on a bug. Have a simpler test case? Does it work for you on a different platform? Let us know! Just going to say 'Me too!'? Don't clutter the database with that please !
Your email address:
MUST BE VALID
Solve the problem : 6 + 9 = ?

 
 [2012-10-11 04:12 UTC] droch_trulia (Dave Rochwerger)
Description: ------------ I was previously using PHP code sniffer 1.3.0 and ignore patterns worked as expected. For example: "phpcs --ignore='*/js/yui*.js' src" would exclude all JS files named starting with "yui" in the "js" directory. However, since upgrading to 1.4.0 the yui*.js files are not being ignored. It does work if I modify the pattern to simply "yui*.js", but this is not sufficient (there are many other complex cases where I need the asterisk to work properly, I'm providing this as a simple example). I have since downgraded back to 1.3.0 and everything works again. This is not due to bash expanding asterisks. Something has changed in phpcs to break the exclude pattern. This issue occurs both with the "ignore" command line flag and with the exclude pattern in the phpcs.xml config file. Test script: --------------- phpcs --ignore='*/js/yui*.js' src Expected result: ---------------- All files in any subdirectory named "js" that start with yui should be excluded from code sniffing. Actual result: -------------- All files are scanned. Note 1: Removing the directory part seems to work, but is not a workable solution. Note 2: It *appears* that the exclude pattern is only executing on the base name of the file as opposed to the whole path.

Comments

 [2012-10-11 04:19 UTC] droch_trulia (Dave Rochwerger)
-PHP Version: 5.3.0 +PHP Version: 5.3.8
 [2012-10-11 04:19 UTC] squiz (Greg Sherwood)
-Status: Open +Status: Feedback -PHP Version: 5.3.8 +PHP Version: 5.3.0 -Assigned To: +Assigned To: squiz
I think the problem you're having is that the path being checked by the ignore pattern processor is now relative (from 1.3.6) instead of absolute. The pull request for that change is here: https://github.com/squizlabs/PHP_CodeSniffer/pull/38 The commit is here: https://github.com/squizlabs/PHP_CodeSniffer/commit/70a1749813527df94e67d6848 be3977cef149c92 The */js/yui*.js pattern still works, but not if the "js" directory is directly under the path you give PHP_CodeSniffer on the command line. So if you dir structure is: MyProject MyProject/js MyProject/js/yui-minified.js And you run: phpcs MyProject --ignore=*/js/yui*.js Then when PHPCS gets to the yui-minified.js file, is is checking the relative path: js/yui-minified.js If used to test the path: /path/to/project/dir/MyProject/js/yui-minified.php The change is a good one because it makes rulesets more portable, but it has caused an issue for you in this case. The best this to do is change the ruleset so you ignore "^js/yui*.js" or, if you have other js dirs you also want to exclude, add two rules ("^js/yui*.js" and "*/js/yui*.js). Hope that explains the change, why it was made, and how you can change your standard, if this is actually the case.
 [2012-10-11 04:38 UTC] droch_trulia (Dave Rochwerger)
@squiz: The solution you gave does not work. I just tried again with about 7 variations of the pattern - none of them work. The directory structure is: build/project/include/js/yui_history.js phpcs is being executed from the "build" directory. I tried the following patterns, all of which failed to stop the example "yui_history.js" file from being included. 1. phpcs --standard=my_std.xml project/include/js --ignore='*/include/js/yui*.js' 2. phpcs --standard=my_std.xml project/include/js --ignore='*include/js/yui*.js' 3. phpcs --standard=my_std.xml project/include/js --ignore='*js/yui*.js' 4. phpcs --standard=my_std.xml project/include/js --ignore='^project/include/js/yui*.js' 5. phpcs --standard=my_std.xml project/include/js --ignore='^include/js/yui*.js' 6. phpcs --standard=my_std.xml project/include/js --ignore='include/js/yui*.js' 7. phpcs --standard=my_std.xml project/include/js --ignore='js/yui*.js' The only thing I can think of is that the relative checks are relative the directory I provided on the command line ("build/project/include/js/") as opposed to where I run it from ("build"). If that's the case, this makes the ignore patterns not very useful.
 [2012-10-11 06:25 UTC] squiz (Greg Sherwood)
Yes, the relative checks *are* relative to the directory on the command line. Where you run PHPCS from is not used. If you look at the pull request, you can see why the change was made and the specific case that required this change. I assure you, they are still useful, but now also much more portable for large projects. Thanks a lot for getting back to me so quickly, and for providing the directory structure. I will take a look at what pattern is needed to exclude that file and get back to you. Don't worry; this isn't something I'm trying to close off without resolving. I'll figure out what the patterns are doing in your code and fix any issues I find.
 [2012-10-11 08:21 UTC] squiz (Greg Sherwood)
(Sorry for this long winded reply. This is also for my own notes if I ever have to come back here.) When you run your command like this: phpcs --standard=my_std.xml project/include/js PHPCS is going to see the yui_history.js file without any proceeding path details. So your ignore regex is going to have to be ^yui*.js or *yui*.js or just yui*.js, as you discovered originally. Is this how you always run PHPCS or is this just a one-off check of this directory? I am assuming this is just a command you've run and you find the ignore pattern otherwise works fine. If so, I understand the issue you are having because your my_std.xml file obviously contains ignore patterns that work when you check your whole project but they wont necessarily work when manually checking sub-directories due to the relative path change. But in this particular case, it is only if you are directly checking a directory that has an ignore rule. For example, checking a 'js' directory when the ignore rule is '*/js/...' or checking a 'build' directory when you have an ignore rule '*/build/...'. In these case, it is best to include 2 ignore paths to cover all the cases: 1. */js/yui*\.js for any yui files inside js dirs anywhere in your project 2. ^(js/)?yui*\.js for when checking the include dir or the js dir directly You could merge these 2 ignore paths into ^((js|*/js)/)?yui[^/]+\.js That covers most things, but still has a problem in that it will ignore any yui[something].js file if it exists directly under the directory you are checking. It can't enforce the fact that it has to be in a directory called js without having access to the full path. And that is where the real problem is here I think. So what I'd like to do is change the ignore path stuff so that it is back to how it used to be. That means changing it to be based on the absolute path instead of the relative one. I'll then need to add an option for ruleset.xml files so they can specify a ignore rule as being relative.
 [2012-10-11 09:03 UTC] squiz (Greg Sherwood)
I've reverted PHPCS back to using absolute paths. If you can, please grab the latest code from the git repo and take a look: git clone git://github.com/squizlabs/PHP_CodeSniffer.git cd PHP_CodeSniffer php scripts/phpcs --ignore=*/js/yu*.js /path/to/build/project/include/js Or revert back your my_std.xml file and use that standard instead.
 [2012-10-12 00:21 UTC] droch_trulia (Dave Rochwerger)
Is the latest branch available through pear? Thanks for fixing that. To give you more background info, the reason we specify individual paths like that is because I had to break up the phpcs ant task into 4 parts because phpcs was eating through more than 4GB of memory running over our whole repo (I have a post phpcs process that merges the 4 output files into one). I noticed that one of the versions reduced the memory footprint as well, so we may not need to do it that way anymore (i've also since reduced the ruleset). As an alternative to your recent change to fix the issue we have, could you have an option that specifies whether the ignore pattern is checked on relative or absolute paths? That might be flexible to all scenarios. Perhaps something like: <exclude-pattern path="absolute">*/js/yui*.js</exclude-pattern> and <exclude-pattern path="relative">^yui*.js</exclude-pattern> And then just default to the most common case.
 [2012-10-12 02:32 UTC] squiz (Greg Sherwood)
No, the latest code is not yet available in PEAR. Only from the git repo. If you dont have git installed and can't run those commands I posted, email me (gsherwood at squiz dot net) and I'll package up a custom PEAR release for you. And I guess we think alike, because the XML tag attribute is exactly how I ended up implementing the change. All ignore paths are absolute by default, but you can set them to relative if you want: <exclude-pattern type="relative">^/tests/*</exclude-pattern> <exclude-pattern type="relative">^/data/*</exclude-pattern> And you can obviously mix absolute and relative paths together. As for the memory change I implemented, it is only for the summary report. The biggest issue with memory over a large code base is storing the messages themselves in memory. The files, for example, never remain in memory unless you are using a multi-file sniff (like the included sample one Generic DuplicateClassNameSniff, or a custom one you've built). The summary report doesn't show the messages themselves, so it doesn't need to store the messages. It now just stores a counter for each file, reducing memory significantly. So if you're using 4GB of memory, you've either got more errors than I've ever seen before (I can store about 30k in less than 200MB) or you are are using multi-file sniffs over a large code base, which I would recommend removing. Hope that helps.
 [2012-11-02 03:30 UTC] squiz (Greg Sherwood)
-Status: Feedback +Status: Closed
This change has now been released.