phpro / grumphp

A PHP code-quality tool
MIT License
4.15k stars 431 forks source link

grumphp does not detect issues after changes #41

Closed orasik closed 9 years ago

orasik commented 9 years ago

Hello, I have installed grumphp as a new project and started playing with one file. The structure is as follows:

src/MyBundle/MyTestClass.php
vendor/
.gitignore
grumphp.yml
composer.json
composer.lock

my grumphp.yml

parameters:
  git_dir: .
  bin_dir: ./vendor/bin
  tasks:
    blacklist:
      keywords:
        - "die("
        - "var_dump("
        - "exit;"
    phpcs:
      standard: "vendor/leaphub/phpcs-symfony2-standard/leaphub/phpcs/Symfony2/"
    phpspec: ~
    phpcsfixer: ~

I have added phpcs to composer as per your README page.

When I run grumphp command line for first time, it works very well and detect any issues related to symfony2 coding standards or blacklist keywords:

php ./vendor/bin/grumphp git:pre-commit --config=grumphp.yml 

However if I make any change to MyTestClass.php file, it will not detect it after running the command again. Interestingly when I left the code for some time (around 1 hour) and ran it again it worked fine again! Also when I run phpcs directly from command line I am able to see if there is any issues with code so I suspect that there is caching or something that affects running tasks.

do you use any caching with grumphp?

my php code:

<?php
namespace File;

class MyTest_Class
{

    protected $nameValue;

    /**
     * @return mixed
     */
    public function getNameValue()
    {
        die();
        exit;

        return $this->nameValue;
    }

    /**
     * @param string $nameValue
     *
     * @return mixed
     */
    public function setNameValue($nameValue)
    {
        var_dump(null);

        return $nameValue;
    }
}
veewee commented 9 years ago

Hello @Orasik,

GrumPHP does not cache anything. It runs the task commands directly from the list of changed files in GIT. It might be possible you need to git add the file first, but that doesn't seem te be te problem.

Are you running the command directly on your filesystem or in a virtualbox with a shared folder or something? Can you tell me which operating system you are using?

orasik commented 9 years ago

Thanks @veewee for your fast response!

veewee commented 9 years ago

Hi @Orasik,

I am also working on Mac. Here are the commands I ran to test:

mkdir tmp
cd tmp
git init
composer require phpro/grumphp
# Copy your grumphp file to the clipboard
pbpaste > grumphp.yml
composer require squizlabs/php_codesniffer --dev
composer require --dev leaphub/phpcs-symfony2-standard
# Copy the PHP file to your clipboard
pbpaste > MyTestClass.php
echo "vendor" > .gitignore
git add -A
git commit -m"Test"

At this point I get following errors:

FILE: tmp/MyTestClass.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | ERROR | Missing class doc comment
----------------------------------------------------------------------

Time: 22ms; Memory: 3Mb

The executable for "php-cs-fixer" could not be found.
You have blacklisted keywords in your commit:
MyTestClass.php:17:        die();
MyTestClass.php:18:        exit;
MyTestClass.php:30:        var_dump(null);

Now I run

vi MyTestClass.php
# Add docblock the line above class: /** docblock */
# Remove the die(); line
# Save the file (esc :wq)
git commit -am"Test"

Now I see these errors:

The executable for "php-cs-fixer" could not be found.
You have blacklisted keywords in your commit:
MyTestClass.php:15:        exit;
MyTestClass.php:27:        var_dump(null);

This seems normal to me. Can you try to reproduce with above commands?

Thanks!

orasik commented 9 years ago

I have ran that, now from different machine but again mac. I noticed the following: 1) If I do only git commit -m "test message" it will cache the previous result. So even if I deleted exit and die() lines, it will still show the error:

You have blacklisted keywords in your commit:
src/MyTestClass.php:17:        die();
src/MyTestClass.php:18:        exit;
src/MyTestClass.php:30:        var_dump(null);

but if I do git commit -am "test message" this will work fine! 2) Running the command:

php vendor/bin/grumphp git:pre-commit

will show the cached message as I mentioned above, so I guess this command is not considering option -a in this command?

3) Because I have phpcs configured in this machine to run in /usr/bin/phpcs it will always show the error:

Warning: include_once(PHP/CodeSniffer/CLI.php): failed to open stream: No such file or directory in /usr/bin/phpcs on line 31

Warning: include_once(): Failed opening 'PHP/CodeSniffer/CLI.php' for inclusion (include_path='.:') in /usr/bin/phpcs on line 31

Fatal error: Class 'PHP_CodeSniffer_CLI' not found in /usr/bin/phpcs on line 34

Thanks! Oras

veewee commented 9 years ago

Hello @Orasik,

1) I found the problem: The blacklist command uses the cached git changes: git grep --cached -n -e "blacklisted"

So the blacklist command will only run on the changes that are actually being committed. Since you are not using the -a option, you are not adding the changes to the file to the commit. This is why this task runs with the cached version.

It doesn't seem like a problem te me, since the pre-commit command is designed to run at git commit. In a next version however, it will be possible to run the tasks from the command line. More info at #33, #40 .

2) The -a option is used to stage all modified / deleted files. This will mark the files as cached and the blacklist will run on the correct version of the file.

3) This also works here. I did the following:

composer remove squizlabs/php_codesniffer
composer global require squizlabs/php_codesniffer

In my include path I have:

export PATH="$HOME/.composer/vendor/bin:$PATH"

But this would be exactly the same if you symlink the file to for example /usr/local/bin. How did you installed the package on your machine? Maybe it's still better to add phpcs as dev dependency. This way GrumPHP will work for people on your team who don't have phpcs globally installed.

veewee commented 9 years ago

Since these tasks work as expected, I will close this issue. Feel free to open a new one when you encounter another issue.