Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grumphp does not detect issues after changes #41

Closed
orasik opened this issue Sep 16, 2015 · 6 comments
Closed

grumphp does not detect issues after changes #41

orasik opened this issue Sep 16, 2015 · 6 comments

Comments

@orasik
Copy link

orasik commented Sep 16, 2015

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;
    }
}
@orasik orasik changed the title grumphp does not detect changes grumphp does not detect issues after changes Sep 16, 2015
@veewee
Copy link
Contributor

veewee commented Sep 16, 2015

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
Copy link
Author

orasik commented Sep 16, 2015

Thanks @veewee for your fast response!

  • I am running files on local machine.
  • OS is Mac OS
  • Running from PHPStorm
    I've tried the same with a colleague who's using Windows and we had the same issue.

@veewee
Copy link
Contributor

veewee commented Sep 16, 2015

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
Copy link
Author

orasik commented Sep 16, 2015

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?

  1. 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
Copy link
Contributor

veewee commented Sep 17, 2015

Hello @orasik,

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 .

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.

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
Copy link
Contributor

veewee commented Sep 22, 2015

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

@veewee veewee closed this as completed Sep 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants