Mention and recommend to use PHP-CS-Fixer when contributing code #8246

javiereguiluz · 2017-08-01T08:23:28Z

This article is intimidating because it looks like you have to manually review a hundred things. However, you just need to execute 1 command ... and if you forget to execute it, that's OK because we have "fabbot" to help you.

GuilhemN

Great improvement 👍

julienfalque · 2017-08-02T20:49:33Z

contributing/code/standards.rst

-example containing most features described below:
+Instead of reviewing your code manually, Symfony makes it simple to ensure that
+your contributed code matches the expected code syntax. First, install the
+`PHP-CS-Fixer tool`_ and then, run this command to fix any problem:


The project name doesn't use dashes: it should be PHP CS Fixer.

julienfalque · 2017-08-02T20:52:44Z

contributing/code/standards.rst

+
+.. code-block:: terminal
+
+    $ php php-cs-fixer.phar fix /path/to/project --rules=@Symfony


The Symfony project has a .php_cs.dist file that holds config for the tool (files to fix and rules to apply). Isn't it better to use this file instead of --rules=@Symfony?

it must use it's own config instead of manually provided ruleset, as @Symfony ruleset is the base, and then in config file there are adjustments, which could even be different for each branch like 2.8/3.1/3.3

keradus · 2017-08-03T07:26:53Z

contributing/code/standards.rst

-Remember that the main advantage of standards is that every piece of code
-looks and feels familiar, it's not about this or that being more readable.
+These Symfony coding standards are based on the `PSR-0`_, `PSR-1`_, `PSR-2`_ and
+`PSR-4`_ standards, so you may already know most of them.


are you sure about it? both psr0 and psr4 ?

keradus · 2017-08-03T07:29:34Z

contributing/code/standards.rst

+
+.. code-block:: terminal
+
+    $ php php-cs-fixer.phar fix /path/to/project --rules=@Symfony


providing a path is also bad, as it will override path configured in .php_cs.dist file, leading to fixing even ignored files., command shall be just php php-cs-fixer.phar fix -v executed from project root dir

keradus · 2017-08-03T07:32:23Z

contributing/code/standards.rst

+
+.. code-block:: terminal
+
+    $ php php-cs-fixer.phar fix /path/to/project --rules=@Symfony


you are aware that this gonna provide bad UX ?
when you just clone the project and execute the command, it will find violations. why? because Symfony doesn't eats its own food and allowed to have violations originally never fixing all of them.

fabbot is configured to run PHP CS Fixer only on files modified in PR, if user will run it locally before sending a PR, whole project will be fixed.

While idea is nice and I'm 👍 for asking user to check the styles himself before sending a PR, I would do it only after Symfony repo would not raise CS violations itself.

What if we change it like this:

$ php php-cs-fixer.phar fix /path/to/modified/file.php

then you would override finder from config and fix the file even if it is excluded via config.
$ php php-cs-fixer.phar fix --path-mode=intersection /path/to/modified/file.php would handle it properly, but this approach basically means you ask end-user to manually provide all modified files. still, bad UX
I would rather propose to keep command simple (to fix all, cache mechanism is on by default, so only first execution would be slower), but just fix project before providing this info in docs

javiereguiluz · 2017-08-03T07:47:09Z

@keradus and @julienfalque thanks for your reviews! I've made all the recommended changes.

keradus · 2017-08-03T10:34:56Z

contributing/code/standards.rst


 * Symfony is released under the MIT license, and the license block has to be
  present at the top of every PHP file, before the namespace.

+.. _`PHP CS Fixer tool`: https://github.com/FriendsOfPHP/PHP-CS-Fixer


everywhere there is website, here you put repo
please use cs.sensiolabs.org for consistency

I prefer not to change that. The http://cs.sensiolabs.org/ website looks outdated to me: the copyright says 2014, it uses a deprecated SensioLabs logo, it hasn't been moved to SensioCloud like the rest of sites, etc. Maybe @fabpot can tell us if he's planning to update or remove http://cs.sensiolabs.org/ Thanks!

website is up to date.

it provides information about newest v2.4 release

copyrights you have mentions are for logo only, not website, probably we could bump it, why not...

logo was in use for PHP CS Fixer and I did agreed with Fabien to have it be dedicated for PHP CS Fixer in future as well

it was not moved to SensioCloud but github.io page to let community fully control it (eg trigger web updates, as original solution to autoupdate website was failing all the time - that was the main reason to move the website in first place), check the CNAME of domain. I do know that old s3 bucket still contains old data, but it's not in use at all. Similar, get.sensiolabs.org could stop providing PHP CS Fixer (maybe it already stopped, not sure, asked Fabien months ago)

I think you have quite outdated informations.

cs.sensiolabs.org was not moved as it's handled by Github now. So, all is good.

keradus · 2017-08-03T10:35:30Z

Thanks for picking it up ;)
sadly, #8246 (comment) is still valid. what do you think about it?

keradus · 2017-08-03T12:19:32Z

contributing/code/standards.rst

+.. code-block:: terminal
+
+    $ cd your-project/
+    $ php php-cs-fixer.phar fix /path/to/modified/file.php


as line was modified my comment is not displayed (as it's "outdated").
yet, this command will work for most the cases, it's wrong
#8246 (comment)

I'm sorry but I don't understand your last comments. Please tell me the exact command that the user must execute and I'll copy+paste it. Thank you.

2 scenarios:
A. current state of PR:

$ php php-cs-fixer.phar fix /path/to/modified/file.php

cons:

file will be fixed even if it's excluded in config fifle

if multiple files were fixed, dev need to provide all of them. this UX sucks and discourage user to run the command at all

B. proper command

$ php php-cs-fixer.phar fix -v

(-v just to see changed files, progress, and so)

cons:

user will fix whole projects, which means that he will also be fixing violations from branch he want contribute to. so even on newly cloned project, he will see violations.

solution
ask user to execute later command, but before asking him that - fix the branches of Symfony to not report violations

Doesn't Fabbot already run the PHP CS fixer and fails if it reports violations? If that's the case, there cannot be any violations in the branches, right?

@wouterj the problem is that we started using CS fixer when Symfony already had a ton of code. And we didn't execute the first time to fix the entire codebase to avoid merge conflicts and breaking all the pending PRs. So, Symfony doesn't contain CS errors for its new code, but it contains some CS issues for its oldest code ... and that's why you can't run the command.

fabbot.io run PHP CS Fixer (and few other tools) only for modified files.
@javiereguiluz the background is correct, yet a lot of effort was already made to apply CS to whole codebase (and got merged dozens of times). While not all is green indeed, there is not that much of it currently for LTS and higher versions

…ng code (javiereguiluz) This PR was squashed before being merged into the 2.7 branch (closes #8246). Discussion ---------- Mention and recommend to use PHP-CS-Fixer when contributing code This article is intimidating because it looks like you have to manually review a hundred things. However, you just need to execute 1 command ... and if you forget to execute it, that's OK because we have "fabbot" to help you. Commits ------- 8bee654 Mention and recommend to use PHP-CS-Fixer when contributing code

Mention and recommend to use PHP-CS-Fixer when contributing code

5c06fe0

carsonbot added the Status: Needs Review label Aug 1, 2017

GuilhemN approved these changes Aug 2, 2017

View reviewed changes

julienfalque reviewed Aug 2, 2017

View reviewed changes

keradus reviewed Aug 3, 2017

View reviewed changes

Fixed all reported issues

a6fdc96

Fixed the link

7b31773

keradus reviewed Aug 3, 2017

View reviewed changes

Fixed the command again

30e9e3e

keradus reviewed Aug 3, 2017

View reviewed changes

javiereguiluz mentioned this pull request Aug 6, 2017

[RFC] Should we apply PHP CS Fixer to all the code of all maintained branches? symfony/symfony#23810

Closed

Minor fixes

5def3f1

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels Jan 4, 2018

javiereguiluz closed this Jan 4, 2018


		.. code-block:: terminal

		$ php php-cs-fixer.phar fix /path/to/project --rules=@Symfony

Uh oh!

Mention and recommend to use PHP-CS-Fixer when contributing code #8246

Mention and recommend to use PHP-CS-Fixer when contributing code #8246

Uh oh!

Conversation

javiereguiluz commented Aug 1, 2017

Uh oh!

GuilhemN left a comment • 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!

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!

keradus Aug 3, 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!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz Aug 3, 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!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commented Aug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keradus Aug 3, 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!

Choose a reason for hiding this comment

Uh oh!

keradus commented Aug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz Aug 3, 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!

keradus Aug 3, 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!

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!

Uh oh!

GuilhemN left a comment •

edited

Loading

keradus Aug 3, 2017 •

edited

Loading

javiereguiluz Aug 3, 2017 •

edited

Loading

keradus Aug 3, 2017 •

edited

Loading

javiereguiluz Aug 3, 2017 •

edited

Loading

keradus Aug 3, 2017 •

edited

Loading