Upgrade PHP CS Fixer to v2 #8822

keradus · 2017-03-08T19:45:28Z

Hi.
I have noticed that you updated PHP CS Fixer to be under FriendsOfPHP vendor, which is great !

Let us put it forward - v1 has reached end of life.
This is my proposal how to update it - with all the upgrade needed to use new version.

Commit logs are meaningful, I did changes one by one.

Keep in mind that v2 brings a lot changes in rules and detects more and more violations - even when using same ruleset (like more rules in PSR2 "level"/"set" or same rule now is more powerful to cover more cases). For that, it's also worth to re-apply the rules on the codebase. To try to keep this PR atomic and easy see the changes, I apply the rules in separated PR - #8823 .
Feel free to close any of the PRs.

magento-cicd2 · 2017-03-08T19:45:30Z

All committers have signed the CLA.

orlangur · 2017-03-09T09:41:59Z

.gitignore

@@ -25,6 +25,8 @@ atlassian*
 /.grunt
 /Gruntfile.js
 /package.json
+/.php_cs
+/.php_cs.cache


I see the same line in composer's gitignore... No way to specify other path, like var/.php_cs.cache or something, right?

deafult location of .php_cs.cache is where .php_cs.dist is.

orlangur · 2017-03-09T21:26:34Z

.php_cs.dist

@@ -0,0 +1,50 @@
+<?php


Do this file has to be in the root? No other options like .github or .travis folders?

Then please do changes similar to https://github.com/magento/magento2/blame/develop/.htaccess#L225 in both .htaccess and .htaccess.sample.

I didn't change the way where you keep the file. If you want to move it to non-default location and point the location every time you run the PHP CS Fixer from CLI - it's possible, it's up to you. Also, I doubt that this file should be used only on Travis env.
Anyway, change of where you store the file is outside of scope of this PR.

I updated `.htaccess, thanks

No, I'm asking whether php-cs-fixer checks any other location besides current folder by default. If not - no need to move configuration file.

@keradus please clarify this item and we are done.

How config path is determined:

path from CLI argument

path of what you are fixing (php-cs-fixer fix foo/ -> foo/.php_cs(.dist))

current working dir (php-cs-fixer fix foo/ -> .php_cs(.dist))

to work the best when one may want to run tool providing only subfolders, like
php-cs-fixer fix foo/
and then
php-cs-fixer fix bar/
the best DX is when the config file is on cwd level, so root of the project

Ok :) Too many details, what I needed is if php-cs-fixer can look into some .php_cs/ folder also.

no (except when manually passing the config file as parameter)

orlangur · 2017-03-09T21:31:51Z

.php_cs.dist

+    ->exclude('dev/tests/functional/vendor')
+    ->exclude('dev/tests/integration/tmp')
+    ->exclude('dev/tests/integration/var')
+    ->exclude('lib/internal/Cm')


Please exclude generated folder also (var/generation was moved to generated).

Some of these paths are irrelevant already but it will not spoil anything and better to be addressed in separate PR or within this PR processing by Magento folks.

I got confused is it supposed to be var/generation or generated.
Anyway, indeed. Out of the scope of this PR. it's up to you which paths should be excluded. I only updated the namespaces and so on. Not changing any logic.

This one: https://github.com/magento/magento2/tree/develop/generated

Ok, no objections from my side, could be updated with any other paths later.

orlangur · 2017-03-09T21:41:30Z

.php_cs.dist

+        'new_with_braces' => true,
+        'no_empty_statement' => true,
+        'no_extra_consecutive_blank_lines' => ['use'],
+        'no_extra_consecutive_blank_lines' => true,


Quick snippet:

php -r 'var_dump(["no" => ["use"], "no" => true]);' array(1) { ["no"]=> bool(true) }

Is there some generator for https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/master/UPGRADE.md#renamed-rules ? :) Probably should be just ['use'].

I first translated rules - originally rules extra_empty_lines and remove_lines_between_uses were both translated into no_extra_consecutive_blank_lines, but with different config.
Then sort lines in array. Indeed, should be only once.

orlangur · 2017-03-09T21:45:08Z

.php_cs.dist

+        'concat_space' => ['spacing' => 'one'],
+        'include' => true,
+        'new_with_braces' => true,
+        'no_empty_statement' => true,


join_function -> no_alias_functions is missing (or omitted due to changed behavior?)

join_function - works only with join function.
new fixer works with dozens of them.

Aha, I see, and it is not configurable, ok.

orlangur · 2017-03-09T21:47:29Z

.php_cs.dist

+    ->setRules([
+        '@PSR2' => true,
+        'array_syntax' => ['syntax' => 'short'],
+        'concat_space' => ['spacing' => 'one'],


operators_spaces -> binary_operator_spaces is missing

It has wider scope

Anything undesired/unexpected? It's not an issue if now it fixes more cases than before :)

some thinks it change too much operators. eg in this repo i noticed you like to have space around !. Don't know if this is intended or not.
I do not want to define the ruleset you want to use or change paths. Just pure v1->v2 upgrade. If you want to update ruleset, go ahead ;) but it's your decision.

Ok :) Here we like consistency, not "zero spaces for concat, one space otherwise".

Good for you ;)
Again, my purpose was not to figure out which ruleset you want to follow, that's part of the community ;) I just took what was in file

orlangur · 2017-03-09T21:51:31Z

.php_cs.dist

+        'ordered_imports' => true,
+        'standardize_not_equals' => true,
+        'ternary_operator_spaces' => true,
+    ]);


Except mentioned ruleset is identical 👍

keradus · 2017-03-10T13:35:19Z

Funny there are random Travis failures in this repo...

orlangur · 2017-03-10T13:41:34Z

0 => 'Your password reset link has expired.'

Aha, didn't see this error before, probably this test relies on system time.

orlangur · 2017-03-10T14:14:25Z

LGTM 🥇

I don't think anybody could do php-cs-fixer upgrade better.

Now let's wait for Fabien come here and say we are using outdated Symfony components and then I could say I saw everything in my life :)

keradus · 2017-03-10T16:25:56Z

Thank you.
Having approval, I signed CLA.

keradus · 2017-03-15T07:22:25Z

So... could this be merged then ?

okorshenko · 2017-03-17T22:53:31Z

@keradus thank you for your contribution!
@orlangur thank you for review!

…op-sync-03072024 Cia 2.4.7 develop 2.4 develop sync 03072024

keradus added 5 commits March 8, 2017 18:30

Upgrade PHP CS Fixer to v2

e79e488

PhpCsFixerReview - remove forced config file, tool is autofinding it

00d531c

rename config file

384af79

ignore local PHP CS Fixer files

bbc5f4e

update config to follow v2 way

212f375

keradus mentioned this pull request Mar 8, 2017

Upgrade PHP CS Fixer to v2 + re-apply #8823

Closed

orlangur requested changes Mar 9, 2017

View reviewed changes

keradus added 2 commits March 10, 2017 13:41

update filename in .htaccess

6f7c4a0

remove duplicated rule

bd1a8cd

orlangur approved these changes Mar 10, 2017

View reviewed changes

okorshenko self-assigned this Mar 16, 2017

okorshenko added this to the March 2017 milestone Mar 16, 2017

magento-team merged commit bd1a8cd into magento:develop Mar 17, 2017

keradus deleted the cs_fixer branch March 18, 2017 10:37

magento-devops-reposync-svc pushed a commit that referenced this pull request Apr 9, 2024

Merge pull request #8822 from magento-cia/cia-2.4.7-develop-2.4-devel…

6225e64

…op-sync-03072024 Cia 2.4.7 develop 2.4 develop sync 03072024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade PHP CS Fixer to v2 #8822

Upgrade PHP CS Fixer to v2 #8822

keradus commented Mar 8, 2017 •

edited by okorshenko

Loading

magento-cicd2 commented Mar 8, 2017 •

edited

Loading

orlangur Mar 9, 2017

keradus Mar 10, 2017

orlangur Mar 9, 2017

keradus Mar 10, 2017

orlangur Mar 10, 2017

orlangur Mar 10, 2017

keradus Mar 10, 2017

orlangur Mar 10, 2017

keradus Mar 10, 2017

orlangur Mar 9, 2017

keradus Mar 10, 2017

orlangur Mar 10, 2017

orlangur Mar 9, 2017

keradus Mar 10, 2017

orlangur Mar 9, 2017

keradus Mar 10, 2017

orlangur Mar 10, 2017

orlangur Mar 9, 2017

keradus Mar 10, 2017

orlangur Mar 10, 2017

keradus Mar 10, 2017

orlangur Mar 10, 2017

keradus Mar 10, 2017

orlangur Mar 9, 2017

keradus commented Mar 10, 2017

orlangur commented Mar 10, 2017

orlangur commented Mar 10, 2017

keradus commented Mar 10, 2017

keradus commented Mar 15, 2017

okorshenko commented Mar 17, 2017

Upgrade PHP CS Fixer to v2 #8822

Upgrade PHP CS Fixer to v2 #8822

Conversation

keradus commented Mar 8, 2017 • edited by okorshenko Loading

magento-cicd2 commented Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus commented Mar 10, 2017

orlangur commented Mar 10, 2017

orlangur commented Mar 10, 2017

keradus commented Mar 10, 2017

keradus commented Mar 15, 2017

okorshenko commented Mar 17, 2017

keradus commented Mar 8, 2017 •

edited by okorshenko

Loading

magento-cicd2 commented Mar 8, 2017 •

edited

Loading