Merge 1.x into master

Author: SonataCI

CONTRIBUTING.md

@@ -2,11 +2,13 @@
 
 Thanks for your interest in Sonata projects!
 
+This document is about issues and pull requests.
+
 ## Summary
 
 * [Issues](#issues)
 * [Pull Requests](#pull-requests)
-* [Label rules]()
+* [Code Reviews](#code-reviews)
 
 ## Issues
 
@@ -338,6 +340,88 @@ We agreed that blank color is boring and so deja vu. Pink is the new way to do.
 ```
 (Obviously, this commit is fake. :wink:)
 
+## Code Reviews
+
+Grooming a PR until it is ready to get merged is a contribution by itself.
+Indeed, why contribute a PR if there are hundreds of PRs already waiting to get reviewed and hopefully, merged?
+By taking up this task, you will try to speed up this process by making sure the merge can be made with peace of mind.
+
+### Commenting on a PR
+
+Before doing anything refrain to dive head-first in the details of the PR and try to see the big picture,
+to understand the subject of the PR. If the PR fixes an issue, read the issue first.
+This is to avoid the pain of making a reviewer rework their whole PR and then not merging it.
+
+Things to hunt for :
+
+- missing docs . This is what you should look for first. If you think the PR lacks docs,
+ask for them, because you will be better at reviewing it if you understand it better,
+and docs help a lot with that.
+- missing tests : Encourage people to do TDD by making clear a PR will not get merged
+if it lacks tests. Not everything is easy to test though, keep that in mind.
+- BC breaks : If there is a BC-break, ask for a deprecation system to be created instead,
+and make sure the `master` branch is used.
+- Unclear pieces of code : does the code use clear, appropriate variable or class names,
+or does it use names like `data`, `result`, `WhateverManager`, `SomethingService`?
+Are exception names still meaningful if you remove the `Exception` suffix? Do all
+exceptions have a custom message?
+Is the contributor trying to be clever or to be clear?
+- Violations of the [SOLID][solid] principles :
+    - S : If a class is 3000 lines long, maybe it does too many things?
+    - O : Is there a big switch statement that might grow in the future?
+    - L : Does the program behave reasonably when replacing a class with a child class?
+    - I : Are interfaces small and easy to implement? If not, can they be split into smaller interfaces?
+    - D : Is the name of a class hardcoded in another class, with the `new` keyword or a static call?
+- Spelling / grammar mistakes, including in commit messages or UPGRADE / CHANGELOG notes.
+- Dependency modifications : is anything new introduced, if yes is it worth it?
+
+[solid]: https://en.wikipedia.org/wiki/SOLID_(object-oriented_design)
+
+Leave no stone unturned. When in doubt, ask for a clarification. If the
+clarification seems useful, and does not appear in a code comment or in a commit
+message, say so and / or make use a squash-merge to customize the commit message.
+Ideally, the project history should be understandable without an internet connection,
+and the PR should be understandable without having a look at the changes.
+
+Also, make sure your feedback is actionable, it is important to keep the ball rolling,
+so if you raise a question, try to also provide solutions.
+
+### Labelling the PR
+
+Applying labels requires write access to PRs, but you can still advise if you do not have them.
+There are several labels that will help determine what the next version number will be.
+Apply the first label that matches one of this conditions, in that order:
+
+- `major`: there is a BC-break. The PR should target the `master` branch.
+- `minor`: there is a backwards-compatible change in the API. The PR should target the stable branch.
+- `patch`: this fixes an issue (not necessarily reported). The PR should target the stable branch.
+- `docs`: this PR is solely about the docs. `pedantic` is implied.
+- `pedantic`: this change does not warrant a release.
+
+Also if you see that the PR lacks documentation, tests, a changelog note,
+or an upgrade note, use the appropriate label.
+
+### Reviewing PRs with several commits
+
+If there are several commits in a PR, make sure you review it commit by commit,
+so that you can check the commit messages, and make sure the commit are independent
+and atomic.
+
+### Merging
+
+Do not merge something you wrote yourself. Do not merge a PR you reviewed alone,
+instead, merge PRs that have already be reviewed and approved by another reviewer.
+If there is only one commit in the PR, prefer the squash feature, otherwise, always
+use a regular merge.
+And finally, use your common sense : if you see a PR about a typo,
+or if there is a situation (faulty commit, revert needed) maybe you can merge it directly.
+
+### Be nice to the contributor
+
+Thank them for contributing. Encourage them if you feel this is going to be long.
+In short, try to make them want to contribute again. If they are stuck, try to provide them with
+code yourself, or ping someone who can help.
+
 [sphinx_install]: http://www.sphinx-doc.org/en/stable/
 [pip_install]: https://pip.pypa.io/en/stable/installing/
 [sf_docs_standards]: https://symfony.com/doc/current/contributing/documentation/standards.html