Skip to content

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

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

Closed
wants to merge 5 commits into from
Closed

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

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c06fe0
Mention and recommend to use PHP-CS-Fixer when contributing code
javiereguiluz Aug 1, 2017
a6fdc96
Fixed all reported issues
javiereguiluz Aug 3, 2017
7b31773
Fixed the link
javiereguiluz Aug 3, 2017
30e9e3e
Fixed the command again
javiereguiluz Aug 3, 2017
5def3f1
Minor fixes
javiereguiluz Jan 4, 2018
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
47 changes: 32 additions & 15 deletions contributing/code/standards.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,33 @@
Coding Standards
================

When contributing code to Symfony, you must follow its coding standards. To
make a long story short, here is the golden rule: **Imitate the existing
Symfony code**. Most open-source Bundles and libraries used by Symfony also
follow the same guidelines, and you should too.
Symfony code is contributed by thousands of developers around the world. To make
every piece of code look and feel familiar, Symfony defines some coding standards
that all contributions must follow.

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-1`_, `PSR-2`_ and `PSR-4`_
standards, so you may already know most of them.

Symfony follows the standards defined in the `PSR-0`_, `PSR-1`_, `PSR-2`_ and `PSR-4`_
documents.
Making your Code Follow the Coding Standards
--------------------------------------------

Since a picture - or some code - is worth a thousand words, here's a short
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:

.. code-block:: terminal

$ cd your-project/
$ php php-cs-fixer.phar fix /path/to/modified/file.php
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@javiereguiluz javiereguiluz Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@keradus keradus Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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


If you forget to run this command and make a pull request with any syntax issue,
our automated tools will warn you about that and will provide the solution.

Symfony Coding Standards in Detail
----------------------------------

If you want to learn about the Symfony coding standards in detail, here's a
short example containing most features described below:

.. code-block:: html+php

Expand Down Expand Up @@ -122,7 +136,7 @@ example containing most features described below:
}

Structure
---------
~~~~~~~~~

* Add a single space after each comma delimiter;

Expand Down Expand Up @@ -181,7 +195,7 @@ Structure
* Do not use spaces around ``[`` offset accessor and before ``]`` offset accessor.

Naming Conventions
------------------
~~~~~~~~~~~~~~~~~~

* Use camelCase, not underscores, for variable, function and method
names, arguments;
Expand Down Expand Up @@ -223,7 +237,7 @@ Service Naming Conventions
* A group name uses the underscore notation.

Documentation
-------------
~~~~~~~~~~~~~

* Add PHPDoc blocks for all classes, methods, and functions (though you may
be asked to remove PHPDoc that do not add value);
Expand All @@ -234,14 +248,17 @@ Documentation

* Omit the ``@return`` tag if the method does not return anything;

* The ``@package`` and ``@subpackage`` annotations are not used.
* The ``@package`` and ``@subpackage`` annotations are not used;

* Inline the ``@inheritdoc`` tag.

License
-------
~~~~~~~

* 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

Copy link
Member

@keradus keradus Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

.. _`PSR-0`: http://www.php-fig.org/psr/psr-0/
.. _`PSR-1`: http://www.php-fig.org/psr/psr-1/
.. _`PSR-2`: http://www.php-fig.org/psr/psr-2/
Expand Down