Feature/update php cs fixer 2.8.x #336

toofff · 2017-11-10T16:28:50Z

The project php-cs-fixer-styleci-bridge has not been released for several months.
When you run php-cs-fix in version 2, nothing works.

I propose to you to pass this project directly and the others with a configuration for php-cs-fix version 2.

On the other hand for the configuration I am maybe going a little strong?

Closes #158
Closes #157

… 2.x

… 2.x for project

greg0ire · 2017-11-10T16:34:08Z

.php_cs.dist

+        'ternary_to_null_coalescing' => true,
+    ])
+    ->setUsingCache(true)
+;


Please configure your IDE to always add a newline at end of file, this will make the red pictogram disappear.

Ooops I forgot to do it for this project.

core23 · 2017-11-10T16:40:59Z

So we must remove StyleCI integration after merging this?

core23 · 2017-11-10T16:42:11Z

project/.php_cs.dist

+ *
+ * It's auto-generated by sonata-project/dev-kit package.
+ *
+ * Package `sllh/php-cs-fixer-styleci-bridge` is required to get it working.


This is still true?

It's just a bad copy paste from the old file.

greg0ire · 2017-11-10T17:31:58Z

.php_cs.dist

+
+return PhpCsFixer\Config::create()
+    ->setFinder($finder)
+    ->setRiskyAllowed(true)


I'm not sure this should be true... will it create BC-breaks by e.g. changing signatures to add void, things like that?

I set the setRiskyAllowed option to true to use the options below :

declare_strict_types

modernize_types_casting

psr4

strict_comparison

strict_param

But is this really necessary for the different Sonata project?

The setRiskyAllowed can stay here, but we have to filter the risky fixers.

For example, strict_param and strict_comparison might introduce BC break on current stables.

greg0ire · 2017-11-10T17:32:16Z

.php_cs.dist

+    ->setRiskyAllowed(true)
+    ->setRules([
+        '@Symfony' => true,
+        '@Symfony:risky' => true,


What does this do exactly?

toofff · 2017-11-11T09:44:12Z

Yes it will remove StyleCI integration after merging.
I forgot to do it yesterday, but can I add PHPCSFIxer in travis?
What do you think @core23 @greg0ire ?

soullivaneuh · 2017-11-11T10:08:23Z

The project php-cs-fixer-styleci-bridge has not been released for several months.

Not released from month does not mean this one is not maintained at all. Otherwise, I would put a message about it. 😉

When you run php-cs-fix in version 2, nothing works.

Well, simply because the StyleCI configuration has diverged form php-cs-fixer v2. 😕

soullivaneuh/php-cs-fixer-styleci-bridge#79

And AFAIK, nothing changed on this side.

I personnaly 👎 to remove it right know and would like to change the CI instead (I'm working on a new one) to still have one working waiting that.

But if the @sonata-project/contributors vote to remove it right know, I'm ok with it.

soullivaneuh

Please add PHP56Migration:risky and PHP56Migration (if exists?`

soullivaneuh · 2017-11-11T10:10:14Z

.php_cs.dist

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


Please make a link to project/.php_cs.dist or require it we should have the same configuration.

Please use require to the project configuration.

soullivaneuh · 2017-11-11T10:12:49Z

.php_cs.dist

+            'syntax' => 'short',
+        ],
+        'braces' => [
+            'allow_single_line_closure' => true,


Is it compliant to SF standards?

soullivaneuh · 2017-11-11T10:13:14Z

.php_cs.dist

+        ],
+        'modernize_types_casting' => true,
+        'no_extra_consecutive_blank_lines' => [
+            'break',


Why this list? Can't we just enable the fixer globally?

soullivaneuh · 2017-11-11T10:14:02Z

.php_cs.dist

+            'throw',
+            'use',
+        ],
+        'no_useless_else' => true,


AFAIK, this one is buggy: PHP-CS-Fixer/PHP-CS-Fixer#2694

soullivaneuh · 2017-11-11T10:14:25Z

.php_cs.dist

+        'phpdoc_order' => true,
+        'psr4' => true,
+        'semicolon_after_instruction' => true,
+        'strict_comparison' => true,


Please remove it. This one may introduce BC breaks.

soullivaneuh · 2017-11-11T10:14:29Z

.php_cs.dist

+        'psr4' => true,
+        'semicolon_after_instruction' => true,
+        'strict_comparison' => true,
+        'strict_param' => true,


Please remove it. This one may introduce BC breaks.

soullivaneuh

Also, if we do this, we must have a Travis job to run php-cs-fixer.

soullivaneuh · 2017-11-11T10:16:29Z

.php_cs.dist

+        'ordered_imports' => true,
+        'phpdoc_order' => true,
+        'psr4' => true,
+        'semicolon_after_instruction' => true,


This one is already included on Symfony rules set, please remove.

soullivaneuh · 2017-11-11T10:16:52Z

.php_cs.dist

+        'no_useless_return' => true,
+        'ordered_imports' => true,
+        'phpdoc_order' => true,
+        'psr4' => true,


To be remove if we keep Symfony:risky.

soullivaneuh · 2017-11-11T10:17:50Z

web/index.php

@@ -9,6 +9,8 @@
 * file that was distributed with this source code.
 */

+declare(strict_types=1);


Remove this declare from dev-kit files. It should not be introduced with common cs files.

soullivaneuh · 2017-11-11T10:47:14Z

As PHP 7.1 will be required on next major, we may take benefit to introduce really risky rules when PHP version is 7.1+?

soullivaneuh · 2017-11-11T22:16:32Z

src/Config/DevKitConfiguration.php

@@ -42,8 +42,7 @@ public function getConfigTreeBuilder()
                ->arrayNode('packages')
                    ->prototype('scalar')->end()
                ->end()
-            ->end()
-        ;


Which rule is doing that? It should not be changed.

soullivaneuh · 2017-11-11T22:17:00Z

src/Console/Command/DispatchCommand.php

-            'has_projects' => true,
-            'has_wiki' => false,
-            'default_branch' => end($branches),
+            'homepage'           => 'https://sonata-project.org/',


Array must not be aligned.

soullivaneuh

Some of my reviews was outdated since some changes.

Could you please make .php_cs.dist a symbolic link to project/.php_cs.dist ? Then we ca discuss about the configuration itself. 👍

soullivaneuh

The project/.travis.yml.twig file needs to be updated in order to run php-cs-fixer on a single job.

You may add maintainer edit option on this PR if you want me to finish it. 👍

soullivaneuh · 2017-11-13T16:46:56Z

composer.json

        "symfony/config": "^3.0",
        "symfony/console": "^3.0",
        "symfony/filesystem": "^3.0",
+        "symfony/yaml": "^3.0",


Well, why not get the latest?

soullivaneuh · 2017-11-13T16:53:14Z

project/.php_cs.dist

+            'header' => $header,
+            'location' => 'after_open',
+        ],
+        'no_extra_consecutive_blank_lines' => [


Please leave this at true to keep the Symfony default config.

soullivaneuh · 2017-11-13T16:53:34Z

project/.php_cs.dist

+return PhpCsFixer\Config::create()
+    ->setFinder($finder)
+    ->setRules([
+        '@Symfony' => true,


Could you please add the following rules sets?

Symfony:risky

PHP56Migration

PHP56Migration:risky

PHPUnit57Migration:risky

toofff · 2017-11-14T10:25:15Z

@soullivaneuh I will take time at the lunch break to finalize the last comments.

Thank you for your help

toofff · 2017-11-14T13:28:53Z

@soullivaneuh I think I have corrected all your requests.

By cons I do not feel that it works the symbolic link for the file .php_cs.dist at the root of the project. Because it should have returned an error with the current configuration.
The only solution I find is to duplicate the .php_cs.dist file at the root of the project.

Do I need to do other PRs in other Sonata projects to remove dependency sllh/php-cs-fixer-styleci-bridge, for example the projet SonataAdminBundle ?

soullivaneuh

If the link does not work, then just require it. The configuration is just PHP code after all, it should work.

What about my comment? #336 (comment)

I may do it if you prefer.

Thank you for the effort.

soullivaneuh · 2017-11-22T12:13:02Z

project/.travis.yml.twig

@@ -75,6 +75,7 @@ matrix:

 before_install:
  - git remote add upstream ${UPSTREAM_URL} && git fetch --all
+  - if [[ $lint = 1 ]]; then wget https://github.com/FriendsOfPHP/PHP-CS-Fixer/releases/download/v2.8.1/php-cs-fixer.phar; fi


Use http://cs.sensiolabs.org/download/php-cs-fixer-v2.phar instead.

soullivaneuh · 2017-11-22T13:00:47Z

.travis.yml

@@ -14,10 +14,12 @@ cache:

 before_install:
  - phpenv config-rm xdebug.ini
+  - wget http://cs.sensiolabs.org/download/php-cs-fixer-v2.phar


--output-document php-cs-fixer.phar

soullivaneuh · 2017-11-22T13:01:25Z

.php_cs.dist

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


Please use require to the project configuration.

toofff · 2017-11-22T13:02:05Z

@soullivaneuh If you do not mind, I'd really like to finish it myself.

For your comment #336, if I understand correctly, you want PHP 7.1 to be required?

So I just have to delete the rules for PHP 5.x and add the rules for PHP 7.1 in the .php_cs.dist file?
I also have to correct the errors that will be reported by PHP-Cs-Fixer after launching it with the new rules for PHP 7.1?
If it's good for you, I'm doing it now.

soullivaneuh · 2017-11-22T13:07:44Z

@soullivaneuh If you do not mind, I'd really like to finish it myself.

No problem, just help proposing. 👍

For your comment #336, if I understand correctly, you want PHP 7.1 to be required?

No. I mean, update the configuration to add the risky rules en PHP migration rules if php-cs-fixer is run under PHP 7.1.

I also have to correct the errors that will be reported by PHP-Cs-Fixer after launching it with the new rules for PHP 7.1?

I don't know, it could be fixed on dev-kit PR. @sonata-project/contributors WDYT?

In any case, please wait this PR being merged before doing anything.

Do I need to do other PRs in other Sonata projects to remove dependency sllh/php-cs-fixer-styleci-bridge, for example the projet SonataAdminBundle ?

If you want to do it, you can. But after getting this PR merged and deployed across repositories.

toofff · 2017-11-22T14:06:43Z

@soullivaneuh

I found the problem with the symbolic link, it was a simple problem of path.

dunglas · 2017-11-23T20:41:36Z

.travis.yml

@@ -14,10 +14,12 @@ cache:

 before_install:
  - phpenv config-rm xdebug.ini
+  - wget http://cs.sensiolabs.org/download/php-cs-fixer-v2.phar --output-document php-cs-fixer.phar


I recommend to download the PHAR from GitHub (https://github.com/FriendsOfPHP/PHP-CS-Fixer/releases) because:

It is available trough HTTPS

You can stick to a specific version, it makes the tests more predictable

dunglas · 2017-11-23T20:46:00Z

project/.travis.yml.twig

@@ -75,6 +75,7 @@ matrix:

 before_install:
  - git remote add upstream ${UPSTREAM_URL} && git fetch --all
+  - if [[ $lint = 1 ]]; then wget http://cs.sensiolabs.org/download/php-cs-fixer-v2.phar --output-document php-cs-fixer.phar; fi


Maybe am I missing something, but it looks like the $lint var is always undefined.

dunglas · 2017-11-23T20:46:40Z

src/Console/Command/DependsCommand.php

@@ -37,7 +37,7 @@ protected function configure()
     */
    protected function execute(InputInterface $input, OutputInterface $output)
    {
-        $branchDepth = intval($input->getOption('branch-depth'));
+        $branchDepth = (int) ($input->getOption('branch-depth'));


Parenthesis around $input->getOption('branch-depth') are useless.

dunglas · 2017-11-23T20:48:47Z

For what it's worth, API Platform switched from StyleCI to PHP CS Fixer some times ago too, because PHP CS Fixer is more flexible (especially when using PHP 7+).

soullivaneuh · 2017-11-30T14:16:20Z

I agree @dunglas, it's quite bad than StyleCI diverged from php-cs-fixer v2. It's also why the bridge I written is hard to maintain.

I'm currently trying a work to make a similar tool but running php-cs-fixer (and others) natively.

Thanks for the review BTW! 👍

soullivaneuh · 2017-11-30T14:17:07Z

.travis.yml

@@ -14,10 +14,12 @@ cache:

 before_install:
  - phpenv config-rm xdebug.ini
+  - wget https://github.com/FriendsOfPHP/PHP-CS-Fixer/releases/download/v2.8.2/php-cs-fixer.phar


I would like to just download v2, minor version should be BC.

soullivaneuh · 2017-11-30T14:18:11Z

composer.json

-        "vlucas/phpdotenv": "^2.2"
+        "sebastian/diff": "^2.0",
+        "silex/silex": "^2.2",
+        "symfony/config": "^3.3",


Why those changes? They are not part of the PR, please revert and just keep php-cs-fixer-styleci-bridge removal.

soullivaneuh · 2017-11-30T14:19:36Z

project/.php_cs.dist

+    'ternary_to_null_coalescing' => true,
+];
+
+if (\PHP_VERSION_ID >= 50600 && \PHP_VERSION_ID < 70000) {


AFAIK, we can use the migration below even with PHP7. Please remove \PHP_VERSION_ID < 70000

soullivaneuh · 2017-11-30T14:19:56Z

project/.php_cs.dist

+        '@PHP56Migration:risky' => true,
+        '@PHPUnit57Migration:risky' => true,
+    ]);
+} else if (\PHP_VERSION_ID >= 70000 && \PHP_VERSION_ID < 70100) {


Remove \PHP_VERSION_ID < 70100

And simple if can be used.

soullivaneuh · 2017-11-30T14:20:11Z

project/.php_cs.dist

+    $rules = array_merge($rules, [
+        'PHP70Migration:risky' => true,
+    ]);
+} else if (\PHP_VERSION_ID >= 70100 && \PHP_VERSION_ID < 70200) {


Remove PHP_VERSION_ID < 70200 and use simple if.

soullivaneuh · 2017-11-30T14:25:42Z

project/.travis.yml.twig

@@ -49,6 +49,7 @@ matrix:
 {% if docs_target %}
    - php: '{{ php|last }}'
      env: TARGET=lint
+      env: lint=1


We don't need this variable.

You should instead download php-cs-fixer binary on this file: https://github.com/sonata-project/dev-kit/blob/master/project/.travis/install_lint.sh (you may use the already present composer global require).

And add the php-cs-fixer command here:

dev-kit/project/Makefile

Line 16 in 5d31b22

git diff --exit-code

(BTW, you don't need the --dry-run and the --diff as we already check the diff at the end.

Sorry for that, I forgot to tell you.

soullivaneuh · 2017-12-04T12:02:42Z

I'll continue and finish your PR in #358, we need it for user-bundle.

Thanks a lot for your work! 👍

toofff · 2017-12-04T12:54:55Z

@soullivaneuh

I did not have time to look at it this weekend.
I wanted to propose to you to finish it, so that it is easier for you.

Thanks and see you soon on an upcoming pull request ;)

toofff added 2 commits November 10, 2017 17:21

Feat: replace sllh/php-cs-fixer-styleci-bridge by php-cs-fixer branch…

b8049aa

… 2.x

Feat: replace sllh/php-cs-fixer-styleci-bridge by php-cs-fixer branch…

6e4d61b

… 2.x for project

greg0ire reviewed Nov 10, 2017

View reviewed changes

Fix review for @greg0ire

c53c15e

core23 reviewed Nov 10, 2017

View reviewed changes

greg0ire reviewed Nov 10, 2017

View reviewed changes

Fix review for @core23

428c7bb

soullivaneuh requested changes Nov 11, 2017

View reviewed changes

soullivaneuh reviewed Nov 11, 2017

View reviewed changes

soullivaneuh requested changes Nov 11, 2017

View reviewed changes

toofff added 4 commits November 11, 2017 12:49

Fix review for @soullivaneuh

c67f118

Fix review for @soullivaneuh

459cff1

Fix review for @soullivaneuh

0f69bcf

Fix review for @toofff

0115cfa

soullivaneuh requested changes Nov 11, 2017

View reviewed changes

toofff added 5 commits November 13, 2017 17:13

Fix review for @soullivaneuh

30b2bf5

Require symfony/yaml for project

d66657b

Feat: launch php-cs-fixer in travis

665ba78

Fix review for @toofff

e2e39d0

Fix review for @toofff

11ceb53

soullivaneuh requested changes Nov 13, 2017

View reviewed changes

soullivaneuh mentioned this pull request Nov 13, 2017

CS: Enable risky fixers and PHP7 migrations for master branch #338

Closed

toofff added 6 commits November 14, 2017 13:56

Update dependencies in their latest version

e11d915

Update rules php-cs-fixer for @soullivaneuh

687eb24

Update last rules php-cs-fixer for @soullivaneuh

525a722

Feat: add php-cs-fixer in travis file for projects Sonata

4b0f5ad

Fix configuration for php-cs-fixer

54a8aa2

Fix configuration for php-cs-fixer

ffa9451

toofff added 2 commits November 14, 2017 14:53

Delete symbolic link for the configuration file php-cs-fixer

85515e5

Fix review by php-cs-fixer

ecefbd5

greg0ire requested a review from soullivaneuh November 18, 2017 20:03

soullivaneuh requested changes Nov 22, 2017

View reviewed changes

Update sources of PhpCsFixer

3e1efb1

soullivaneuh requested changes Nov 22, 2017

View reviewed changes

toofff added 2 commits November 22, 2017 14:39

Fix symbolic link and travis

1d7967d

Fix php cs fixer configuration

e904120

dunglas suggested changes Nov 23, 2017

View reviewed changes

Fix review for @dunglas

3a0276b

soullivaneuh requested changes Nov 30, 2017

View reviewed changes

soullivaneuh closed this Dec 4, 2017

Uh oh!

Feature/update php cs fixer 2.8.x #336

Feature/update php cs fixer 2.8.x #336

Uh oh!

Conversation

toofff commented Nov 10, 2017 • edited by soullivaneuh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

core23 commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toofff commented Nov 11, 2017

Uh oh!

soullivaneuh commented Nov 11, 2017

Uh oh!

soullivaneuh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soullivaneuh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soullivaneuh commented Nov 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soullivaneuh left a comment

Choose a reason for hiding this comment

Uh oh!

soullivaneuh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soullivaneuh Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toofff commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

toofff commented Nov 10, 2017 •

edited by soullivaneuh

Loading

soullivaneuh Nov 13, 2017 •

edited

Loading

toofff commented Nov 14, 2017 •

edited

Loading

dunglas Nov 23, 2017 •

edited

Loading