Allow php-cs-fixer to autofix

[tacone]

Hello, I would like to propose to make --dry-run optional. I understand auto-fixing may not suit everyone's needs, but I find it terribly handy.

Before finding about grumphp I even put together a simple package to accomplish that. I would be so happy to dump my homegrown solution for something like grumphp, but I know I'd miss the auto-fixing too much.

What do you think? It seems something very easy to accomplish, let me know if you can do it or you would like a pull request.

[veewee]

Hi @tacone,

Thanks for propising this feature.

Personally I am not a big fan of manipulating code automatically right before a commit.
This way you don't have control about what you commit. Imagine that the command goes wrong and your files are totally messed up.

I also have 2 other concerns:

• You will have to add the file again after the manipulations, but what about files with both staged and unstaged changes? This may result in unexpected behaviour.
• What about the priority of the task? If this task runs and changes files, this means that the tests that ran previously to the fix command could succeed, but after the automatic fix could fail.

What are your views on the topics above?

[tacone]

Hello @veewee, thank you for the thoughtful reply. You are right in what you say, and I agree with you that the dry run is a sensible choice and should be kept as default.

Regarding the concerns, it is possible even if unlikely that php-cs-fixer would mess up something. It is very unlikely that will change the actual behavior of the code or the result of the other tests.

My take on the whole matter is very simple: if GrumPHP will not let you commit without fixing, then you are going to fix anyways, automatically or not.

I agree with you that manually fixing gives you more control.

[veewee]

Hi @tacone,

I don't have a problem with the automatic fixing, but with the automatic git add $file of the code that is changed during validation.
Maybe the files could be marked as changed in a central service. When all tasks ran, the question could be asked to the user: The phpcsfixer changed 6 files. Do you want to add them to the commit?
Maybe with an option to view the diff of the unstaged files at that point.

At that point the phpcsfixer task should not throw any exceptions, but it is recommended that the ouput of the task is shown in a non-blocking way. (See #17)

Not sure that when you ask a question in GUI tools, they will also prompt the question the the user. So this is something we will need to take a look at.

[tacone]

Yes, I understood that.

The issues you raise are worth considering. I guess that the best short term solution to adjust for my workflow is just creating a custom task and live with it.

Thank you for now :)

[ceesvanegmond]

@veewee Why not make it optional to run the cs fixer? By default it doesn't fix the issues, but in the YAML file you can enable this.

[veewee]

As mentioned above: there are multiple problems with auto fixing the code.
In the latest release, additional problems were added: It is possible that the vagrant folder does not contain the git directory so automated git adds could not be possible on all systems.

For me, the big problem remains that you are committing code that you don't control. I want to avoid issues like "GrumPHP eats my code" when there is actually an issue in cs-fixer.

The way I see it, this feature is more a PITA then a feature somebody would want to use.
Anyway: I am open for discussions on the topic. If somebody knows how to solve all these problems while keeping the code easy and clean, it might be implemented one day :)

[ceesvanegmond]

For people who really want to use this feature; we created an seperate repository with this task. Check it out. https://github.com/wearejust/grumphp-extra-tasks

[spawnia]

@veewee Can you please consider adding this option? If you don't use it personally, that is fine - but it would be great to at least have the choice.

https://github.com/wearejust/grumphp-extra-tasks is working fine.

[fatemeh-ro]

I've installed GrumPHP Extra tasks via composer require --dev wearejust/grumphp-extra-tasks command and my grumphp.yml file is like this:

grumphp:
    extensions:
        - Wearejust\GrumPHPExtra\Extension\Loader
    tasks:
        php_cs_auto_fixerv2:
            path_mode: ~
            verbose: true
            diff: false
            triggered_by: [ 'php' ]

while I was committing my files this shows an error:

PHP Fatal error: Declaration of Wearejust\GrumPHPExtra\Extension\Loader::load(Symfony\Component\DependencyInjection\ContainerBuilder $container) must be compatible with GrumPHP\Extension\ExtensionInterface::load(Symfony\Component\DependencyInjection\ContainerBuilder $container): void in /home/fatima/code/programs/vendor/wearejust/grumphp-extra-tasks/src/Extension/Loader.php on line 17