Php-cs-fixer: Path-aware fixer sets

[mlocati]

(ref. #3077)

What about implementing a way to specify fixer sets that depend on the path of the file currently being fixed?

For instance, we could have the following .php_cs configuration file:

<?php
use PhpCsFixer\Config;
use PhpCsFixer\Finder;
use PhpCsFixer\PathAwareRules;

return Config::create()
    ->setRiskyAllowed(true)
    ->setFinder(
        PhpCsFixer\Finder::create()
            ->in(__DIR__)
    )
    ->setRules([
        '@PSR2' => true,
        'psr4' => true,
    ])
    ->addPathAwareRules(
        PathAwareRules::create()
            ->setRelativeTo(__DIR__)
            ->setInheritRules(true)
            ->setRegularExpressions([
                '/^tests\//',
            ])
            ->setRules([
                'psr4' => false,
            ])
    )
    ->addPathAwareRules(
        PathAwareRules::create()
            ->setRelativeTo(__DIR__)
            ->setInheritRules(false)
            ->setRegularExpressions([
                '/^bin\//',
                '/^dev-bin\//',
            ])
            ->setRules([
                '@PSR1' => true,
            ])
   )
;

This file:

1. defines a default set of rules (Config::setRules())
2. for paths relative to __DIR__ (PathAwareRules::setRelativeTo(__DIR__)) that match /^tests\// (PathAwareRules::setRegularExpressions()) we use the default set of rules (PathAwareRules::setInheritRules(true)) but disabling the psr4 fixer.
3. for paths relative to __DIR__ (PathAwareRules::setRelativeTo(__DIR__)) that match /^bin\//or /^dev-bin\// (PathAwareRules::setRegularExpressions()) we use only (PathAwareRules::setInheritRules(false)) the @PSR1 preset.

[SpacePossum]

Way back I build a prototype for something like:

<?php

$header = <<<'EOF'
This file is part of PHP CS Fixer.

(c) Fabien Potencier <[email protected]>
    Dariusz Rumiński <[email protected]>

This source file is subject to the MIT license that is bundled
with this source code in the file LICENSE.
EOF;

$rules = array(
    '@Symfony' => true,
    '@Symfony:risky' => true,
    'array_syntax' => array('syntax' => 'long'),
    'combine_consecutive_unsets' => true,
    // one should use PHPUnit methods to set up expected exception instead of annotations
    'general_phpdoc_annotation_remove' => array('annotations' => array('expectedException', 'expectedExceptionMessage', 'expectedExceptionMessageRegExp')),
    'header_comment' => array('header' => $header),
    'heredoc_to_nowdoc' => true,
    'no_extra_consecutive_blank_lines' => array('tokens' => array('break', 'continue', 'extra', 'return', 'throw', 'use', 'parenthesis_brace_block', 'square_brace_block', 'curly_brace_block')),
    'no_short_echo_tag' => true,
    'no_unreachable_default_argument_value' => true,
    'no_useless_else' => true,
    'no_useless_return' => true,
    'ordered_class_elements' => true,
    'ordered_imports' => true,
    'phpdoc_add_missing_param_annotation' => true,
    'phpdoc_order' => true,
    'semicolon_after_instruction' => true,
    'strict_comparison' => true,
    'strict_param' => true,
);


$config = [
    'src' =>    
    PhpCsFixer\Config::create()
        ->setRiskyAllowed(true)
        ->setRules($rules)
        ->setFinder(
        PhpCsFixer\Finder::create()
            ->in(__DIR__.'/src')
        ),
    'test' =>   
    PhpCsFixer\Config::create()
        ->setRiskyAllowed(true)
        ->setRules(array(
        array_merge($rules, ['php_unit_strict' => true, # other PHPUnit fixers#])
        ))
        ->setFinder(
        PhpCsFixer\Finder::create()
            ->exclude('tests/Fixtures')
            ->in(__DIR__.'/tests')
        )
];

return $config;

Never finished it, biggest issue was caching. I wonder, would a setup like that also works for your use-cases?

[mlocati]

would a setup like that also works for your use-cases?

Nope: we have a rather complex system (see here for the list of checks that we should use to determine the fixer list).

[keradus]

what if one provided same file with multiple different PathAwareRule ?
like once file.php with concat=space, and 2nd time with concat=none ?

[mlocati]

I'd pick the first PathAwareRules that satisfies the criteria.
So, for instance, way we may have a default PathAwareRules for a folder, and a particular PathAwareRules for a specific file in that folder.

[mlocati]

For instance:

$config
    ->addPathAwareRules(
        PathAwareRules::create()
            ->setRegularExpressions([
                '/^tests\/file.php$/',
            ])
            ->setRules($fileSpecificRules)
    )
    ->addPathAwareRules(
        PathAwareRules::create()
            ->setRegularExpressions([
                '/^tests\//',
            ])
            ->setRules($rulesForTheFolder)
    )
;

If somebody wants to include the $rulesForTheFolder in $fileSpecificRules, it's enough to write something like

$fileSpecificRules = ['fixerHandle' => false] + $rulesForTheFolder;

[keradus]

then it depends on order of injection to main config. for that I'm :-1:
one will set /src -> fixer_a, / -> fixera,fixer_b and wonder why fixer_b was not applied for /src

[mlocati]

one will set /src -> fixer_a, / -> fixera,fixer_b and wonder why fixer_b was not applied for /src

Because he explicitly said /src -> fixer_a...

BTW, to solve this, we could add a flag to the PathAwareRules class, to say if the process should use only the rules defined by the PathAwareRules instance, or if all rules of all the applicable PathAwareRules should be applied.

Something like this (pseudocode):

$finalRules = null;
foreach ($pathAwareRulesList as $par {
    /* @var PathAwareRules $par */
    if (false === $par->isForCurrentPath()) {
        continue;
    }
    $parRules = $par->getRules(); // This should take care of setInheritRules
     if (true === $par->getNameOfTheNewFlag()) {
        $finalRules = $parRules;
        break;
    }
    $finalRules = ($finalRules ?: []) + $parRules;
}
if (null === $finalRules) {
    $finalRules = $theDefaultRulesOfConfig;
}

[keradus]

then you will end up having conflicting rules for different paths which merge output is unpredictable

[mlocati]

I'm sorry I don't understand what you mean...

[KorvinSzanto]

@keradus That could just be solved with a descriptive exception thrown if a file path exists in two rule sets. No need to try to merge things.

[mlocati]

Well, I wouldn't do that: how could you say "use these fixers for all the files in this folder, but use these other fixers for this specific file in that folder"?

[keradus]

then it's different from what you said in

I'd pick the first PathAwareRules that satisfies the criteria.

[mlocati]

then it's different from what you said in

I'd pick the first PathAwareRules that satisfies the criteria.

No. Consider this example:

$config
    ->addPathAwareRules(
        PathAwareRules::create()
            ->setRegularExpressions([
                '/^tests\/file.php$/',
            ])
            ->setRules($fileSpecificRules)
    )
    ->addPathAwareRules(
        PathAwareRules::create()
            ->setRegularExpressions([
                '/^tests\//',
            ])
            ->setRules($rulesForTheFolder)
    )
;

As you can see, if the execution picks the first matching criteria, we'd use $fileSpecificRules for tests/file.php, and $rulesForTheFolder for all the other files in the tests folder.

[ktomk]

@SpacePossum: Your setup looks great to me, for the cache: just take one cache per set, I'm fine with that, too.

[KacerCZ]

@SpacePossum I like your prototype. Have you tried to complete its implementation<
Recently I found the need for exactly the same use case as in your example.
I check code and tests and can't enable "php_unit_test_class_requires_covers" rule because some of regular classes are named "Test".

[SpacePossum]

Hi @KacerCZ !

I haven't finished the prototype. We are still discussing how the new configuration could look like for v3.0 and forward, but it probably won't look (close) like my prototype. I still think it would be value to allow configuration of rules based on (exceptions) of files and directories. Therefore I hope to convince other for this need as well and figure out how to do implement it nicely (might be baselines or something, who knows!).

[mlocati]

I'm wondering if the new configuration approach @SpacePossum is referring to would support a rather complex situation.

For example, a typical concrete5 project has this structure (where <HANDLE> is not known in advance, and we may have 0...N of them):

| Path | Rule set |
|---|---|
| /index.php | Rule set A |
| /concrete/dispatcher.php | Rule set A |
| /concrete/bin/concrete5 | Rule set A |
| /concrete/attributes/<HANDLE>/controller.php | Rule set B |
| /application/attributes/<HANDLE>/controller.php | Rule set B |
| /packages/<HANDLE>/attributes/<HANDLE>/controller.php | Rule set B |
| /packages/<HANDLE>/controller.php | Rule set B |
| /concrete/authentication/<HANDLE>/controller.php | Rule set B |
| /application/authentication/<HANDLE>/controller.php | Rule set B |
| /packages/<HANDLE>/authentication/<HANDLE>/controller.php | Rule set B |
| /concrete/blocks/<HANDLE>/controller.php | Rule set B |
| /application/blocks/<HANDLE>/controller.php | Rule set B |
| /packages/<HANDLE>/blocks/<HANDLE>/controller.php | Rule set B |
| /concrete/geolocation/<HANDLE>/controller.php | Rule set B |
| /application/geolocation/<HANDLE>/controller.php | Rule set B |
| /packages/<HANDLE>/geolocation/<HANDLE>/controller.php | Rule set B |
| /concrete/themes/<HANDLE>/page_theme.php | Rule set B |
| /application/themes/<HANDLE>/page_theme.php | Rule set B |
| /packages/<HANDLE>/themes/<HANDLE>/page_theme.php | Rule set B |
| /concrete/controllers/**/* | Rule set B |
| /application/controllers/**/* | Rule set B |
| /packages/<HANDLE>/controllers/**/* | Rule set B |
| /concrete/jobs/**/* | Rule set B |
| /application/jobs/**/* | Rule set B |
| /packages/<HANDLE>/jobs/**/* | Rule set B |
| /concrete/bootstrap/**/* | Rule set C |
| /application/bootstrap/**/* | Rule set C |
| /concrete/config/**/* | Rule set C |
| /application/config/**/* | Rule set C |
| /packages/<HANDLE>/config/**/* | Rule set C |
| /concrete/routes/**/* | Rule set C |
| /application/routes/**/* | Rule set C |
| /packages/<HANDLE>/routes/**/* | Rule set C |
| /concrete/src/**/* | Rule set C |
| /application/src/**/* | Rule set C |
| /packages/<HANDLE>/src/**/* | Rule set C |
| /tests/**/* | Rule set C |
| Anything else | Rule set D |

At the moment, we use some of the internal functions of php-cs-fixer to implement this approach:

• users provide one or more paths (to files or directories)
• we resolve all the paths to the full file list (if required)
• we group the files/directories having the same rule set
• we run php-cs-fixer for every group of files/directories having the same ruleset

[mlocati]

About the above comment of mine:

• the paths are relative to a predefined values (the web root)
• the rule sets A, B and C start from rule set D, but alter (add/edit/remove) some of its rules