Fix coding standards #27852

stof · 2018-07-05T11:43:06Z

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 Add NativeConstantInvocationFixer 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 NativeConstantInvocationFixer - should not prepend a backslash when the file is not namespaced 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 NativeConstantInvocationFixer - add the scope option 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 Add the native_function_invocation fixer in the Symfony:risky ruleset PHP-CS-Fixer/PHP-CS-Fixer#3873 to include it in the ruleset automatically.

TODOs:

agree on the updated rules
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.

keradus · 2018-07-05T12:39:51Z

@stof , would be great if you can create a benchmark without and with those changes !

stof · 2018-07-05T12:50:29Z

I don't think a benchmark will show much differences here, as the hot path of the DI component (and of a few other places) were already fully-qualified manually by @nicolas-grekas (well, maybe not in 2.8, but at least in newer versions).

nicolas-grekas · 2018-07-05T12:51:54Z

make separate PRs for newer branches with their own updates (exclude rules, and CS fixes), once this PR gets merged.

Actually, the process for this is:

merge everything up to master
merge this PR on 2.8
merge everything up to master. Resolving conflicts just means running the command again on each branch (thus no need for per-branches PRs.)

stof · 2018-07-05T12:52:56Z

@nicolas-grekas the update of the config file (exclude rules) will be needed for newer branches too (due to renaming some code, or adding more places needing to be excluded).

stof · 2018-07-05T12:53:19Z

but yes, fixing conflicts would be done by running the command again.

linaori · 2018-07-05T13:33:33Z

Wouldn't it make more sense to have use statements for global imports? Especially as readability and merge conflicts are a commonly provided rejection for doing this in existing code.

stof · 2018-07-05T13:45:27Z

@iltar this would require using use const and use function, which cannot be used in 2.8 or in 3.x due to our PHP requirements.

And for classes, our existing rule is already to avoid use statements for global classes, so this is consistent.

and regarding merge conflicts, this is precisely the reason why I'm doing this in 2.8, not in master (so that all branches are using the same CS), and why I'm doing it now and not 6 months ago (the change is now automated)

linaori · 2018-07-05T13:53:57Z

I mention this because a lot of people are complaining about the readability of backslashes in the code, and are asking for imports instead. I do agree with them on this point, because it does hurt readability quite some. I understand we can't merge this in 2.8, but why not do it in 3.4+ only? 2.8 support won't last long anymore either way and to be fair, people should've already upgraded a while ago.

stof · 2018-07-05T13:58:39Z

@iltar 3.4 supports PHP 5.5.9+, so this would not be 3.4+, but 4.0+. And having CS violations in 4.x for any code contributed in 3.4 and merged up would mean that we suffer from branch merging for the whole 3.4 lifetime (i.e. for the next 2 years: https://symfony.com/roadmap/3.4).

linaori · 2018-07-05T14:00:04Z

Ah my bad, I forgot that 3.4 was not on 7.x yet. Yay for LTS.

stof · 2018-07-10T18:36:05Z

regarding the constant invocation, I will revert the change, and disable the rule temporarily, until PHP-CS-Fixer/PHP-CS-Fixer#3876 is merged and released. This would avoid some of the existing changes which add visual clutter for no performance benefit.

fabpot · 2018-07-12T07:15:35Z

@stof Can you revert what needs to be reverted so that I can merge this one and then I will take care of propagating changes up to master?

fabpot · 2018-07-12T07:16:36Z

fabbot is now using v2.12.2

We don't want to run it on non-namespaced code to reduce visual clutter.

stof · 2018-07-24T09:52:05Z

@fabpot done.

The list of excluded files in the php-cs--fixer configuration file (updated in the first commit of this PR) will need some adjustments for newer branches (I have a branch locally with a similar commit for master (I started there before switching target branch), and there were a few differences.
I could perform the propagation of changes to newer branches, taking care of this if you want.

I intentionally kept multiple commits in the history, to better see the different changes performed in this PR (as some of them are automated).

…ed functions

nicolas-grekas · 2018-07-24T10:08:07Z

CliDumperTest.php needs a tweak: the last frame (%d. %slosure%s() ==> Twig%cTemplate->render(): {) is now removed on PHP7 & HHVM.
Let's replace it with a "%A" in the fixture.

keradus · 2018-07-24T11:11:29Z

src/Symfony/Component/Process/Tests/PipeStdinInStdoutStdErrStreamSelect.php

@@ -35,22 +35,22 @@
    }

    if (in_array(STDOUT, $w) && strlen($out) > 0) {
-        $written = fwrite(STDOUT, (binary) $out, 32768);
+        $written = fwrite(STDOUT, (string) $out, 32768);


wow, I did not expect to see binary casting so much in real code :D

nicolas-grekas · 2018-07-26T08:30:23Z

Thank you @stof.

@nicolas-grekas

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

nicolas-grekas · 2018-07-26T09:13:18Z

Now merged up to master.
I also enabled the "ordered_imports" fixer.
Thank you all for this!

SpacePossum · 2018-07-27T20:52:54Z

nice to see the new fixers being used :D thanks all!

stof requested a review from dunglas as a code owner July 5, 2018 11:43

carsonbot added the Status: Needs Review label Jul 5, 2018

nicolas-grekas added this to the 2.8 milestone Jul 5, 2018

stof mentioned this pull request Jul 10, 2018

[Finder] Allow arrays as parameters of some methods for better fluent experience and code readability #27891

Merged

2 tasks

stof added 3 commits July 24, 2018 09:39

Update the list of excluded files for the CS fixer

8892b98

Disable the native_constant_invocation fixer until it can be scoped

720ed4d

We don't want to run it on non-namespaced code to reduce visual clutter.

Apply fixers

f00b327

Enable the fixer enforcing fully-qualified calls for compiler-optimiz…

04654cf

…ed functions

stof force-pushed the fix_cs_2_8 branch from 80b93b8 to 04654cf Compare July 24, 2018 10:05

stof requested a review from xabbuh as a code owner July 24, 2018 10:05

keradus reviewed Jul 24, 2018

View reviewed changes

Fix Clidumper tests

538c69d

nicolas-grekas merged commit 538c69d into symfony:2.8 Jul 26, 2018

stof deleted the fix_cs_2_8 branch July 26, 2018 09:33

Tobion mentioned this pull request Jun 11, 2019

Native function and constant invocation CS composer/composer#8178

Closed

Uh oh!

Fix coding standards #27852

Fix coding standards #27852

Uh oh!

Conversation

stof commented Jul 5, 2018 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keradus commented Jul 5, 2018

Uh oh!

stof commented Jul 5, 2018

Uh oh!

nicolas-grekas commented Jul 5, 2018

Uh oh!

stof commented Jul 5, 2018

Uh oh!

stof commented Jul 5, 2018

Uh oh!

linaori commented Jul 5, 2018

Uh oh!

stof commented Jul 5, 2018

Uh oh!

linaori commented Jul 5, 2018

Uh oh!

stof commented Jul 5, 2018

Uh oh!

linaori commented Jul 5, 2018

Uh oh!

stof commented Jul 10, 2018

Uh oh!

fabpot commented Jul 12, 2018

Uh oh!

fabpot commented Jul 12, 2018

Uh oh!

stof commented Jul 24, 2018

Uh oh!

nicolas-grekas commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keradus Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jul 26, 2018

Uh oh!

nicolas-grekas commented Jul 26, 2018

Uh oh!

SpacePossum commented Jul 27, 2018

Uh oh!

Uh oh!

stof commented Jul 5, 2018 •

edited by nicolas-grekas

Loading

nicolas-grekas commented Jul 24, 2018 •

edited

Loading