PhpCsFixer ignores stdout with finder defined

[MeCapron]

Q	A
Version	1.13.0
Bug?	no
New feature?	yes
Question?	yes
Documentation?	no
Related tickets	-

Hello,

we are using grumphp in a Gitlab CI and we are facing an issue with the configuration of PhpCsFixer.

As we have a very very large code base which contains a lot of legacy code, even in the CI we are running GrumPHP ONLY in the files that are commited to avoid exploding the memory of the CI.

To do so, we are running Grumphp this way :

git diff --diff-filter=dr --name-only -r origin/$CI_MERGE_REQUEST_TARGET_BRANCH_NAME... | php -dmemory_limit=128M ./vendor/bin/grumphp run --config=grumphp.yml

Howerver the PhpCsFixer is running over ALL files because of this line when you have a custom finder :

https://github.com/phpro/grumphp/blob/v1.13.0/src/Task/PhpCsFixer.php#L96

        if ($context instanceof GitPreCommitContext || !$config['config_contains_finder']) {
            $arguments->addFiles($files);
        }

This is running fine when we are in a GitPreCommitContext or when we don't run any finder.

Could we add a configuration to list only files from the stdout even if we have a finder on or we are not in the GitPreCommitContext?

Does anyone has maybe another idea to solve this?

My configuration

grumphp:
    tasks:
        phpcsfixer:
            allow_risky: true
            config: './grumphp/.php-cs-fixer.php'
            triggered_by: [ 'php', 'phtml' ]
            config_contains_finder: true
            verbose: true

$finder = PhpCsFixer\Finder::create()
    ->in('./app');

$config = new PhpCsFixer\Config();
$config->setFinder($finder)
    ->setRules([
        '@Symfony'                               => true,
        '@PHP80Migration'                        => true
    ]);

return $config;

Steps to reproduce:

• Create two files
• Run grumphp in a stdout way (like a git diff)
• Phpcsfixer will run on the two files instead of the only one you have passed in the stdout with the configuration given

Result: Out of memory because running on all files

[yguedidi]

@MeCapron potential other idea: if command arguments are accessible in the PHP-CS-Fixer config file, with $argv, you could build a custom Finder with only the files you need

[veewee]

Adding an env var to toggle config_contains_finder to false on CI could also do the trick.

[MeCapron]

Adding an env var to toggle config_contains_finder to false on CI could also do the trick.

Sorry, I did not set the entire snippet but I use exclusion on the finder so I need to have the finder defined to true. Do you have another idea maybe?

@MeCapron potential other idea: if command arguments are accessible in the PHP-CS-Fixer config file, with $argv, you could build a custom Finder with only the files you need

Thats sounds great, I will check this if I don't have any other solution. However, I think that it can be convenient to have only the needed files to be checked even with a config finder set in : we could want to perform php-cs-fixer on a stdout but still exclude some folders.

Isn't a misinterpretation of myself on how php-cs-fixer designed the usage of this finder?

[veewee]

In that case, we might need a smarter way to set php-cs-fixer's intersection mode, which deals with the intersection of passing files and a finder

https://github.com/phpro/grumphp/blob/3ec61c1678c4c370f02b05fef606fd561d923c8e/src/Task/PhpCsFixer.php#L91

[MeCapron]

In that case, we might need a smarter way to set php-cs-fixer's intersection mode, which deals with the intersection of passing files and a finder

https://github.com/phpro/grumphp/blob/3ec61c1678c4c370f02b05fef606fd561d923c8e/src/Task/PhpCsFixer.php#L91

Do you have any idea or thoughts on it?

I can help you, but if you already have ideas...

[anthonyhacart]

Hello, we encounter the kind of issues, any chance we can help ?

[MeCapron]

After digging a bit, it seems that they are no chance to have the args because of the task launched in a subprocess.

It seems that the best way would probably to add a tag to know whether or not the command was ran with a STDIN to add the flag and check if the files should be added or not. You are the expert here so this is just my coin. Maybe a Metadata to know if files are provided from STDIN?

It could be done somewhere here :

https://github.com/phpro/grumphp/blob/e9f7c4d1888dab46d33642d3925f00e55adaf37a/src/Console/Command/RunCommand.php#L110-L117

And then in the PhpCsFixer :

        if ($context instanceof GitPreCommitContext || !$config['config_contains_finder'] || $files->getMetadata()->isFromStdin()) {
            $arguments->addFiles($files);
        }

From now on until we have a proper solution I will just always pass the files to the arguments

[veewee]

The problem with that solution is that files that are ignored in the finder might be checked as well. Since it falls back to the originally provided path-mode.

I don't think adding an isFromStdin() is a good solution either. Since regular git hooks also pass the git diff through stdin to make sure partial commits work as well.

Maybe better to fix it with configuration on the task?

• force_add_files: '%env(bool:ENV_VAR_THAT_DETERMINES_IT_RUNS_IN_CI)%'
• force_path_mode: '@=env.ENV_VAR_THAT_DETERMINES_IT_RUNS_IN_CI ? "intersect" : null'

If you don't configure them - the task will run as expected. If you do configure them, it means you know best and it just does whatever you ask it to?