NativeConstantInvocationFixer - add the scope option #3876
Conversation
keradus
changed the title
This PR was merged into the 2.12 branch. Discussion ---------- NamespacesAnalyzer - Optimize performance Instead of looping on all tokens, even inside the namespace it identified, the analyzer now continues the analysis after the end of the identified namespace, thanks to the fact that namespaces cannot be nested. When implementing #3876, I compared the NamespacesAnalyzer implementation with `\PhpCsFixer\Fixer\FunctionNotation\NativeFunctionInvocationFixer::getUserDefinedNamespaces`. The NamespacesAnalyzer also identifies the namespace name instead of skipping over it (and returns the global namespace too). But I found out that NativeFunctionInvocationFixer was more optimized, as it was not re-analyzing all tokens inside a namespace. This makes NamespacesAnalyzer implement the same optimization. Commits ------- 8321c26 Optimize the namespaces analyzer
|
@keradus anything missing here ? |
|
Hi @stof , |
stof
force-pushed
the
scope_constant_invocation
branch
from
8d15a82
to
885a13f
Compare
| $this->doTest($expected, $input); | ||
| } | ||
|
|
||
| public function testFixScopedOnly() |
There was a problem hiding this comment.
I don't know if it matters anything, but I'm missing a (scoped only) test with no namespace keyword at all.
<?php
echo PHP_VERSION . PHP_EOL;Should be untouched
| @@ -864,6 +864,8 @@ Choose from the list of available rules: | |||
| ``get_defined_constants``. User constants are not accounted in this list | |||
| and must be specified in the include one; defaults to ``true`` | |||
| - ``include`` (``array``): list of additional constants to fix; defaults to ``[]`` | |||
| - ``scope`` (``'all'``, ``'namespaced'``): only fix constant invocations that are made | |||
There was a problem hiding this comment.
The order of options is all, namespaced, however the description order is namespaced, all. Could be confusing.
There was a problem hiding this comment.
that's exactly the same wording than for the scope option of the native_function_invocation fixer.
There was a problem hiding this comment.
Fix that one too? 😇 I dunno, I'll leave that call to (one of) the maintainers.
There was a problem hiding this comment.
the order is arbitrary and I doubt someone compares the README against the source/describe command, I see no need for an update here (or in the fixer)
There was a problem hiding this comment.
I wasn't comparing anything to anything, I just ment in this specific line in itself. It reads a, b: description of b, or a.
There was a problem hiding this comment.
Im not saying that you were, still no change needed IMHO
stof
force-pushed
the
scope_constant_invocation
branch
from
885a13f
to
451a81d
Compare
This PR was squashed before being merged into the 2.8 branch (closes #27852). Discussion ---------- Fix coding standards | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This PR is mostly about running the PHP-CS-Fixer (v2.12.1) in the whole codebase. - I updated the exclude rule to avoid some false positives for the `error_suppression` fixer (we have more files triggering unsilenced deprecations on purpose than when building the initial whitelist, mostly). - I ran the fixer with this updated config. Most changes were related to fully-qualifying some constants, with the new fixer implemented in PHP-CS-Fixer/PHP-CS-Fixer#3127, for which @nicolas-grekas and I suggested a config to include in the Symfony ruleset. Based on the output, I suggested a feature request in PHP-CS-Fixer/PHP-CS-Fixer#3872 as we might want to avoid the `\` in non-namespaced files to improve readability. We might want to remove the second commit of this PR if we decide to wait for the feature to be implemented (update: implementation is contributed in PHP-CS-Fixer/PHP-CS-Fixer#3876) - I added the `native_function_invocation` fixer explicitly, to automatically fully-qualify calls to compiler-optimized functions. This feature was implemented in PHP-CS-Fixer based on our feature request (as currently, we do such thing only manually in some hot path, because it could not be automated). I opened PHP-CS-Fixer/PHP-CS-Fixer#3873 to include it in the ruleset automatically. TODOs: - [x] agree on the updated rules - [x] update fabbot to use the new version of PHP-CS-Fixer - [ ] make separate PRs for newer branches with their own updates (exclude rules, and CS fixes), once this PR gets merged. Commits ------- 538c69d Fix Clidumper tests 04654cf Enable the fixer enforcing fully-qualified calls for compiler-optimized functions f00b327 Apply fixers 720ed4d Disable the native_constant_invocation fixer until it can be scoped 8892b98 Update the list of excluded files for the CS fixer
|
ping @FriendsOfPHP/php-cs-fixer @localheinz @kubawerlos @ntzm |
| @@ -225,6 +212,59 @@ protected function createConfigurationDefinition() | |||
| ->setAllowedValues([$constantChecker]) | |||
| ->setDefault(['null', 'false', 'true']) | |||
| ->getOption(), | |||
| (new FixerOptionBuilder('scope', 'Only fix constant invocations that are made within a namespace or fix all.')) | |||
There was a problem hiding this comment.
As we've added this option, can we also add some a test or two to check that 'scope' => 'all' works correctly
There was a problem hiding this comment.
well, all the non-modified tests are running with the all scope
| ]); | ||
| } | ||
|
|
||
| private function fixConstantInvocations(Tokens $tokens, $start, $end) |
There was a problem hiding this comment.
missing PHPDoc to typehint $start and $end
There was a problem hiding this comment.
the composer.json says that PHP-Cs-Fixer supports PHP 5.6+
| ]); | ||
| } | ||
|
|
||
| private function fixConstantInvocations(Tokens $tokens, $start, $end) |
There was a problem hiding this comment.
If a file with multiple namespaces is fixed this method might be called multiple times (based on the new config option).
The $useDeclarations found and the token analyzer (created later on) are based on the complete tokens collection, these can be reused as long as no tokens are inserted.
There was a problem hiding this comment.
but this method inserts tokens.
Actually, a better way (for both this fixer and the function invocation one) would be to analyze use statements only for the current namespace anyway. but the use statement analyzer does not support that currently.
stof
force-pushed
the
scope_constant_invocation
branch
2 times, most recently
from
5c9fee3
to
54a6af8
Compare
keradus
force-pushed
the
scope_constant_invocation
branch
from
0f2ecb2
to
3dbf8f6
Compare
|
Thank you @stof. |
…tof, keradus) This PR was squashed before being merged into the 2.13-dev branch (closes #3876). Discussion ---------- NativeConstantInvocationFixer - add the scope option Closes #3872 The way to implement the option is heavily inspired by the existing feature in NativeFunctionInvocationFixer, except that I reused the NamespacesAnalyzer implementation instead of copy-pasting `\PhpCsFixer\Fixer\FunctionNotation\NativeFunctionInvocationFixer::getUserDefinedNamespaces` I also updated the Symfony ruleset to restrict the fixer to the namespaced scope as adding the `\` when the code is already in the global scope has no benefit for performance (as both cases are fully qualified) while increasing visual burden (which was the reason to limit it to only a few constants in the Symfony ruleset). Commits ------- 3dbf8f6 NativeConstantInvocationFixer - add the scope option
This PR was squashed before being merged into the 2.8 branch (closes #27852). Discussion ---------- Fix coding standards | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This PR is mostly about running the PHP-CS-Fixer (v2.12.1) in the whole codebase. - I updated the exclude rule to avoid some false positives for the `error_suppression` fixer (we have more files triggering unsilenced deprecations on purpose than when building the initial whitelist, mostly). - I ran the fixer with this updated config. Most changes were related to fully-qualifying some constants, with the new fixer implemented in PHP-CS-Fixer/PHP-CS-Fixer#3127, for which @nicolas-grekas and I suggested a config to include in the Symfony ruleset. Based on the output, I suggested a feature request in PHP-CS-Fixer/PHP-CS-Fixer#3872 as we might want to avoid the `\` in non-namespaced files to improve readability. We might want to remove the second commit of this PR if we decide to wait for the feature to be implemented (update: implementation is contributed in PHP-CS-Fixer/PHP-CS-Fixer#3876) - I added the `native_function_invocation` fixer explicitly, to automatically fully-qualify calls to compiler-optimized functions. This feature was implemented in PHP-CS-Fixer based on our feature request (as currently, we do such thing only manually in some hot path, because it could not be automated). I opened PHP-CS-Fixer/PHP-CS-Fixer#3873 to include it in the ruleset automatically. TODOs: - [x] agree on the updated rules - [x] update fabbot to use the new version of PHP-CS-Fixer - [ ] make separate PRs for newer branches with their own updates (exclude rules, and CS fixes), once this PR gets merged. Commits ------- 538c69dc26 Fix Clidumper tests 39d2079 Enable the fixer enforcing fully-qualified calls for compiler-optimized functions f00b3279ea Apply fixers 720ed4d379 Disable the native_constant_invocation fixer until it can be scoped 8892b98627 Update the list of excluded files for the CS fixer
Closes #3872
The way to implement the option is heavily inspired by the existing feature in NativeFunctionInvocationFixer, except that I reused the NamespacesAnalyzer implementation instead of copy-pasting
\PhpCsFixer\Fixer\FunctionNotation\NativeFunctionInvocationFixer::getUserDefinedNamespacesI also updated the Symfony ruleset to restrict the fixer to the namespaced scope as adding the
\when the code is already in the global scope has no benefit for performance (as both cases are fully qualified) while increasing visual burden (which was the reason to limit it to only a few constants in the Symfony ruleset).