Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade PHP CS Fixer to v2 #8822

Merged
merged 7 commits into from
Mar 17, 2017
Merged

Upgrade PHP CS Fixer to v2 #8822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e79e488
Upgrade PHP CS Fixer to v2
keradus Mar 8, 2017
00d531c
PhpCsFixerReview - remove forced config file, tool is autofinding it
keradus Mar 8, 2017
384af79
rename config file
keradus Mar 8, 2017
bbc5f4e
ignore local PHP CS Fixer files
keradus Mar 8, 2017
212f375
update config to follow v2 way
keradus Mar 8, 2017
6f7c4a0
update filename in .htaccess
keradus Mar 10, 2017
bd1a8cd
remove duplicated rule
keradus Mar 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ atlassian*
/.grunt
/Gruntfile.js
/package.json
/.php_cs
/.php_cs.cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/pub/media/*.*
!/pub/media/.htaccess
Expand Down
2 changes: 1 addition & 1 deletion .htaccess
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@
order allow,deny
deny from all
</Files>
<Files .php_cs>
<Files .php_cs.dist>
order allow,deny
deny from all
</Files>
Expand Down
2 changes: 1 addition & 1 deletion .htaccess.sample
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@
order allow,deny
deny from all
</Files>
<Files .php_cs>
<Files .php_cs.dist>
order allow,deny
deny from all
</Files>
Expand Down
52 changes: 0 additions & 52 deletions .php_cs
View file
Open in desktop

This file was deleted.

49 changes: 49 additions & 0 deletions .php_cs.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keradus please clarify this item and we are done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Copyright © 2013-2017 Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

/**
* Pre-commit hook installation:
* vendor/bin/static-review.php hook:install dev/tools/Magento/Tools/StaticReview/pre-commit .git/hooks/pre-commit
*/
$finder = PhpCsFixer\Finder::create()
->name('*.phtml')
->exclude('dev/tests/functional/generated')
->exclude('dev/tests/functional/var')
->exclude('dev/tests/functional/vendor')
->exclude('dev/tests/integration/tmp')
->exclude('dev/tests/integration/var')
->exclude('lib/internal/Cm')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

->exclude('lib/internal/Credis')
->exclude('lib/internal/Less')
->exclude('lib/internal/LinLibertineFont')
->exclude('pub/media')
->exclude('pub/static')
->exclude('setup/vendor')
->exclude('var');

return PhpCsFixer\Config::create()
->setFinder($finder)
->setRules([
'@PSR2' => true,
'array_syntax' => ['syntax' => 'short'],
'concat_space' => ['spacing' => 'one'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operators_spaces -> binary_operator_spaces is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has wider scope

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'include' => true,
'new_with_braces' => true,
'no_empty_statement' => true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'no_extra_consecutive_blank_lines' => true,
'no_leading_import_slash' => true,
'no_leading_namespace_whitespace' => true,
'no_multiline_whitespace_around_double_arrow' => true,
'no_multiline_whitespace_before_semicolons' => true,
'no_singleline_whitespace_before_semicolons' => true,
'no_trailing_comma_in_singleline_array' => true,
'no_unused_imports' => true,
'no_whitespace_in_blank_line' => true,
'object_operator_without_whitespace' => true,
'ordered_imports' => true,
'standardize_not_equals' => true,
'ternary_operator_spaces' => true,
]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except mentioned ruleset is identical 👍

2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
"squizlabs/php_codesniffer": "1.5.3",
"phpmd/phpmd": "@stable",
"pdepend/pdepend": "2.4.0",
"friendsofphp/php-cs-fixer": "~1.2",
"friendsofphp/php-cs-fixer": "~2.1.1",
"lusitanian/oauth": "~0.3 <=0.7.0",
"sebastian/phpcpd": "2.0.0"
},
Expand Down
Loading