Added page "Community Reviews" #5480
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,4 +5,5 @@ Community | |
| :maxdepth: 2 | ||
|
|
||
| releases | ||
| reviews | ||
| other | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,205 @@ | ||
| Community Reviews | ||
| ================= | ||
|
|
||
| Symfony is an open-source project driven by a large community. If you don't feel | ||
| ready to contribute code or patches, reviewing issues and pull requests (PRs) | ||
| can be a great start to get involved and give back. | ||
|
|
||
| Why Reviewing Is Important | ||
| -------------------------- | ||
|
|
||
| Community reviews are essential for the development of the Symfony framework, | ||
| since there are many more pull requests and bug reports than there are members | ||
| in the Symfony core team to review, fix and merge them. | ||
|
|
||
| On the `Symfony issue tracker`_, you can find many items in a "Needs Review" | ||
| status: | ||
|
|
||
| * **Bug Reports**: Bug reports primarily need to be checked for completeness. | ||
| Is any important information missing? Can the bug be easily reproduced? | ||
|
|
||
| * **Pull Requests**: Pull requests contain code that fixes a bug or implements | ||
| new functionality. Reviews of pull requests ensure that they are implemented | ||
| properly, are covered by test cases, don't introduce new bugs and maintain | ||
| backwards compatibility. | ||
|
|
||
| Note that **anyone who has some basic familiarity with Symfony and PHP can | ||
| review bug reports and pull requests**. You don't need to be an expert to help. | ||
|
|
||
| Be Constructive | ||
| --------------- | ||
|
|
||
| Before you begin, remember that you are looking at the result of someone else's | ||
| hard work. A good review comment thanks the contributor for their work, | ||
| identifies what was done well, identifies what should be improved and suggests a | ||
| next step. | ||
|
|
||
| Create a GitHub Account | ||
| ----------------------- | ||
|
|
||
| Symfony uses GitHub_ to manage bug reports and pull requests. If you want to | ||
| do reviews, you need to `create a GitHub account`_ and log in. | ||
|
|
||
| The Bug Report Review Process | ||
| ----------------------------- | ||
|
|
||
| A good way to get started with reviewing is to pick a bug report from the | ||
| `bug reports in need of review`_. | ||
|
|
||
| The steps for the review are: | ||
|
|
||
| #. **Is the Report Complete?** | ||
|
|
||
| Good bug reports contain a link to a fork of the `Symfony Standard Edition`_ | ||
| (the "reproduction project") that reproduces the bug. If it doesn't, the | ||
| report should at least contain enough information and code samples to | ||
| reproduce the bug. | ||
|
|
||
| #. **Reproduce the Bug** | ||
|
|
||
| Download the reproduction project and test whether the bug can be reproduced | ||
| on your system. If the reporter did not provide a reproduction project, | ||
| create one by forking_ the `Symfony Standard Edition`_. Reproduce the bug | ||
|
There was a problem hiding this comment. see above about the SE fork |
||
| with the instructions given by the reporter. | ||
|
|
||
| #. **Update the Issue Status** | ||
|
|
||
| At last, add a comment to the bug report. **Thank the reporter for reporting | ||
| the bug**. Include the line ``Status: <status>`` in your comment to update | ||
| the status of the ticket. This line will trigger our `Carson Bot`_ which | ||
| updates the labels of the issue accordingly. You can set the status to one of | ||
| the following: | ||
|
|
||
| **Needs Work** If the bug does not contain enough information to be | ||
| reproduced, explain what information is missing and move the report to this | ||
| status. | ||
|
|
||
| **Works for me** If the bug *does* contain enough information to be | ||
|
There was a problem hiding this comment. should be have 2 states for this? one for not reproducible and one for new feature? There was a problem hiding this comment. How can a bug report be a new feature? Shouldn't we rather update "Bug" to "Feature" then and remove the issue from he review process? |
||
| reproduced but works on your system, or if the reported bug is a feature and | ||
| not a bug, provide a short explanation and move the report to this status. | ||
|
|
||
| **Reviewed** If you can reproduce the bug, move the report to this status. | ||
| If you created a reproduction project, include the link to the project in | ||
| your comment. | ||
|
|
||
| .. topic:: Example | ||
|
|
||
| Here is a sample comment for a bug report that could be reproduced: | ||
|
|
||
| .. code-block:: text | ||
|
|
||
| Thank you @weaverryan for creating this bug report! This indeed looks | ||
| like a bug. I reproduced the bug in the "kernel-bug" branch of | ||
| https://github.com/webmozart/symfony-standard. | ||
|
There was a problem hiding this comment. should'nt the link just be There was a problem hiding this comment. I think the current example provides less friction as you can copy paste the repo URL from the browser, whereas you need to some more clicks to find the URL of the correct branch. |
||
|
|
||
| Status: Reviewed | ||
|
There was a problem hiding this comment. This sounds a bit redundant as anyone can set a status? |
||
|
|
||
| The Pull Request Review Process | ||
| ------------------------------- | ||
|
|
||
| The process for reviewing pull requests (PRs) is similar to doing a review of a | ||
| bug report. Reviews of pull requests usually take a little longer since you need | ||
| to understand the functionality that has been fixed or implemented and then find | ||
| out whether the implementation is complete. | ||
|
|
||
| It is okay to do partial reviews. If you do a partial review, comment how far | ||
| you got and leave the PR in "Needs Review" state. | ||
|
|
||
| Pick a pull request from the `PRs in need of review`_ and follow these steps: | ||
|
|
||
| #. **Is the PR Complete**? | ||
|
|
||
| Every pull request must contain a header that gives some basic information | ||
| about the PR. You can find the template for that header in the | ||
| :ref:`Contribution Guidelines <contributing-code-pull-request>`. | ||
|
|
||
| #. **Is the Base Branch Correct?** | ||
|
|
||
| GitHub displays the branch that a PR is based on below the title of the | ||
| pull request. Is that branch correct? | ||
|
|
||
| * Bugs should be fixed in the oldest, maintained version that contains the | ||
| bug. Check :doc:`Symfony's Release Schedule <releases>` to find the oldest | ||
| currently supported version. | ||
|
|
||
| * New features should always be added to the current development version. | ||
| Check the `Symfony Roadmap`_ to find the current development version. | ||
|
There was a problem hiding this comment. We should explain here that you need to differ between BC breaking changes and BC compatible changes given that we have development branches for 2.x and 3.x, shouldn't we? |
||
|
|
||
| #. **Reproduce the Problem** | ||
|
|
||
| Read the issue that the pull request is supposed to fix. Reproduce the | ||
| problem on a clean `Symfony Standard Edition`_ project and try to understand | ||
| why it exists. | ||
|
|
||
| #. **Review the Code** | ||
|
|
||
| Read the code of the pull request and check it against some common criteria: | ||
|
There was a problem hiding this comment. just note that we will be able to (partially) automate some of these aspects eventually .. |
||
|
|
||
| * Does the code address the issue the PR is intended to fix/implement? | ||
| * Does the PR stay within scope to address *only* that issue? | ||
| * Does the PR contain automated tests? Do those tests cover all relevant | ||
| edge cases? | ||
| * Does the PR contain sufficient comments to easily understand its code? | ||
| * Does the code break backwards compatibility? If yes, does the PR header say | ||
| so? | ||
| * Does the PR contain deprecations? If yes, does the PR header say so? Does | ||
| the code contain ``trigger_error()`` statements for all deprecated | ||
| features? | ||
| * Are all deprecations and backwards compatibility breaks documented in the | ||
| latest UPGRADE-X.X.md file? Do those explanations contain "Before"/"After" | ||
|
There was a problem hiding this comment. [...] "Before"/"After" examples [...] |
||
| examples with clear upgrade instructions? | ||
|
|
||
| Eventually, some of these aspects will be checked automatically. | ||
|
|
||
| #. **Test the Code** | ||
|
|
||
| Take your project from step 3 and test whether the PR works properly. | ||
| Replace the Symfony vendor by the code in the PR by running the following Git | ||
| commands. Insert the PR ID for the ``<ID>`` placeholders: | ||
|
|
||
| .. code-block:: text | ||
|
|
||
| $ cd vendor/symfony/symfony | ||
| $ git fetch origin pull/<ID>/head:pr<ID> | ||
|
There was a problem hiding this comment. is it really called origin? Afaik, Composer calls its repository There was a problem hiding this comment. Hmm, just checked and origin is available (besides composer). |
||
| $ git checkout pr<ID> | ||
|
|
||
| Now you can test the project against the code in the PR. | ||
|
|
||
| #. **Update the PR Status** | ||
|
|
||
| At last, add a comment to the PR. **Thank the contributor for working on the | ||
| PR**. Include the line ``Status: <status>`` in your comment to update the | ||
| status of the ticket. This line will trigger our `Carson Bot`_ which updates | ||
| the labels of the issue accordingly. You can set the status to one of the | ||
| following: | ||
|
|
||
| **Needs Work** If the PR is not yet ready to be merged, explain the issues | ||
| that you found and move it to this status. | ||
|
|
||
| **Reviewed** If the PR satisfies all the checks above, move it to this | ||
| status. A core contributor will soon look at the PR and decide whether it can | ||
| be merged or needs further work. | ||
|
|
||
| .. topic:: Example | ||
|
|
||
| Here is a sample comment for a PR that is not yet ready for merge: | ||
|
|
||
| .. code-block:: text | ||
|
|
||
| Thank you @weaverryan for working on this! It seems that your test | ||
| cases don't cover the cases when the counter is zero or smaller. | ||
| Could you please add some tests for that? | ||
|
|
||
| Status: Needs Work | ||
|
|
||
| .. _GitHub: https://github.com | ||
| .. _Symfony issue tracker: https://github.com/symfony/symfony/issues | ||
| .. _Symfony Standard Edition: https://github.com/symfony/symfony-standard | ||
| .. _create a GitHub account: https://help.github.com/articles/signing-up-for-a-new-github-account/ | ||
| .. _forking: https://help.github.com/articles/fork-a-repo/ | ||
| .. _bug reports in need of review: https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aopen+is%3Aissue+label%3A%22Bug%22+label%3A%22Status%3A+Needs+Review%22+ | ||
| .. _PRs in need of review: https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=is%3Aopen+is%3Apr+label%3A%22Status%3A+Needs+Review%22+ | ||
| .. _Contribution Guidelines: https://github.com/symfony/symfony/blob/master/CONTRIBUTING.md | ||
| .. _Symfony's Release Schedule: http://symfony.com/doc/current/contributing/community/releases.html#schedule | ||
| .. _Symfony Roadmap: https://symfony.com/roadmap | ||
| .. _Carson Bot: https://github.com/carsonbot/carsonbot | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put focus on the Symfony SE fork first? Imo many bugs are easily reproducable by describing some simple steps or by giving some small example code snippet. The thing is, we should make it as easy as possible to contribute and forking the Standard Edition is obviously more work than writing text that provides the same details.
So I think we should keep this paragraph, but just resort a bit which approach we put focus on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah ... i think we should classify the bugs a bit more to suggest when an SE fork is the sensible way to provide a reproducible test case .. in the end for a simple bug a description will be more efficient than having to download and run an SE fork (though what would be awesome is if platform.sh would provide some infrastructure to quickly spin up such forks for core devs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my experience there is almost no bug that is easier to reproduce without a SE fork than with. Even if a bug sounds simple, you need to set up the environment, test whether the bug can be reproduced, if not (often the case) understand why etc. I personally have switched to asking for SE forks even for simple bugs. This way the risk of wasting time is much lower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I shared the same thought as @lsmith77 and @xabbuh, but I think that forks are really done too little, and we should experiment with trying to encourage them. Yes, it's more work for contributors, but it may also be more satisfying for them to ship some code with the bug in it, and ultimately, this is an effort to relieve burden on the core team (and a fork is the best way). Let's try it this way for now.