search

phpro / grumphp

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

PHP-CS-Fixer fails when a file is checked but excluded through .php_cs file #95

Closed tifabien closed 8 years ago

tifabien commented 8 years ago

First, thanks for your great work !

I faced a problem regarding PHP-CS-fixer and both pre-commit hook and run command.

When I exclude a file to be checked by cs-fixer through the .php_cs file, then execute the grumphp run command ( the excluded file is tracked ), it fails because actually when we provide a file path in the php-cs-fixer bin, it overwrites the .php_cs config

I created an issue in the php-cs-fixer project in order to maybe change this behavior.

I'm stuck with the run command as my CI server rejects all my builds. My question is, do you think we could execute php-cs-fixer task without file name passed as argument when we execute the run command?

veewee commented 8 years ago

I read the discussion at: https://github.com/FriendsOfPHP/PHP-CS-Fixer/issues/1719 One thing we could do is read the .php_cs configuration file and use the configured finder to filter the files in our resultset. Not sure if this is possible and how it should be done, but I think this is the best option which will also speed up the phpcsfixer task in GrumPHP. (Currently the phpcsfixer is pretty slow because the tool runs for every committed file. It was on my checklist to take a look on how to improve this speed.)

Would this possible solution solve your issue?

tifabien commented 8 years ago

Your solution seems interesting and I think it should solve this issue :)

keradus commented 8 years ago

Please play with https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/1742

veewee commented 8 years ago

@keradus,

I just took a look at the commit and it will probably solve @tifabien's problem. Yesterday I started on a similar approach, but in GrumPHP: https://github.com/veewee/grumphp/commit/77bfdf4d9f0b54bc5febebfbff879e37c649f334 As you can see, the phpcsfixer configuration is being loaded and an intersection between the finder in phpcsfixer and the commited files in GrumPHP is being made. This was mostly done for starting less phpcsfixer commands, but would also fix this problem.

Just out of curiousity: might it also be an option to add multiple directories / files to the CLI arguments? Currently we have to start phpcsfixer for every file that is being changed during a commit. This is a pretty slow task.

keradus commented 8 years ago

I started mine yesterday as well ;)

Your solution would be problematic when one want to run GrumPHP with PHP CS Fixer v2@dev, as you start using the PHP CS Fixer classes.

That is exactly my goal, but first I will need to deal with https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/1744 and probably https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/1666

veewee commented 8 years ago

Okay, that would be great! So I guess there is nothing we can do in our project. I don't think this cross version support is soemthing we want to add to GrumPHP. We will have to wait until this is fixed in php-cs-fixer and for the multiple files until this is implemented.

keradus commented 8 years ago

So I guess there is nothing we can do in our project.

You can do it on your end but not sure is it needed to be implemented twice.

I don't think this cross version support is soemthing we want to add to GrumPHP.

That could be tricky indeed. One question about it - when there will be PHP CS Fixer 2.0, you will only support 1.x or 2.x ?

veewee commented 8 years ago

Not sure yet. I haven't looked at phpcsfixer v2 yet. If the CLI is more or less the same, it might be possible to support both. If that is not possible, we will probably force the usage of v2 at some point. It's a development tool, so I guess that won't be a big problem to upgrade.

keradus commented 8 years ago

Guide for CLI usage upgrade: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/UPGRADE.md May be useful.

veewee commented 8 years ago

Looks great! Thanks!

veewee commented 8 years ago

@keradus: It seems like the 2 issues in php-cs-fixer you mentioned got merged. Is this issue resolved?

keradus commented 8 years ago

Hi @veewee , https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/1742 is merged indeed, but it's merged into 2.x@dev line (not 1.x@stable).

Anyway, I think this is more an issue of PHP CS Fixer, and as such there is no need for having the issue opened here, as grumphp may be considered as tool-runner.

veewee commented 8 years ago

Ok, Thanks!