Last updated July 20, 2016.
This page describes how to contribute to the community by reviewing patches. Patches are pieces of code that solve an issue either in Drupal core or a contributed module. This article will address the following:
- Why reviewing is important
- Choosing a patch to review
- Be constructive
- The patch review process
- Helpful tips
Why reviewing is important
The review process is an essential part of the development of Drupal core, since there are many more patches to be reviewed than there are people to commit those patches.
Changes in smaller contributed modules tend to be made by the module maintainers themselves, but larger projects (especially Drupal core) often have patches in a "Needs review" status.
To learn more about patches, see the Drupal patch page.
Choosing a patch to review
The contributor and development blocks have links to both patch queues and resources for development. You can enable these blocks in your user profile, if you haven't already.
Patches are submitted for everything from small changes (such as adding a missing period in a code comment) to large changes (such as a rewrite of major parts of core). For those new to the patch review process, bug fixes (this is broken; here's the fix) tend to be easier to review than tasks (let's change things) or feature requests (I want a pony).
Before you begin, remember that the patch is the result of someone else's hard work. A good review post thanks the contributor for their work, identifies what was done well, identifies what should be improved, and suggests a next step. Read more about how to give constructive feedback.
The patch review process
Before looking at a patch, it's useful to familiarize yourself with the issue it changes. Read and try to understand the issue summary and comments, so you can evaluate whether the proposed patch adequately addresses everyone's concerns.
Patches need to be reviewed by someone other than the original author before being declared Reviewed and tested by the community. During normal development, patches will go through several revisions, sometimes by multiple authors, before being committed to the official code repository. Although this process isn't foolproof, it helps ensure that code has been peer reviewed for performance, readability, usability, and so forth, as well as regressions and bugs. It is OK if you do not review all aspects of a patch. Post what you did review (including a note about what you did and didn't review) and others will review the other aspects of the patch.
In general the steps for patch review are:
Reproduce the problem
Before you begin testing a patch you should perform a clean Drupal install and follow the instructions provided in the patch notes to reproduce the problem or issue addressed by the patch. If there are no steps to reproduce the issue and you cannot duplicate please ask the original poster to provide them.
Review the code
Read the patch. It is important to open the patch and read the code that has been edited. This will allow you to determine whether the patch is adequately addressing the issue it is intended to correct. There are some common criteria all patches need to comply with before they'll be considered for core and contributed code:
- Does the code address the issue the patch is intended to correct?
- Does the patch stay within scope to address only that issue?
- If unrelated issues are found, are separate followup issues filed to address them instead?
- Does the patch make any major changes or add new functionality? If so, does it have appropriate code comments?
- If the patch includes automated tests, are they both necessary and sufficient?
For contrib patches each project may have different requirements. For core, minimum requirements are outlined in the core gates.
Test the Patch
There are several steps you should take every time you test a patch. Ideally you should run all these steps first on a clean Drupal install, then again with the patched version. This ensures that any unexpected bugs you might find are actually a result of the patch rather than the original code.
- Does the patch actually solve the problem?
- Does the patch introduce any new bugs?
- If there are new UI elements added do they follow established patterns?
Update the issue status
RTBC If the patch meets all the above criteria, add your comments, explain what you did your review, and change the status from "Needs review" to "Reviewed and tested by the community." In your comment, describe what you reviewed and how, as well as any questions or doubts you had while reviewing and what the answers were.
Needs work If the patch does not meet all the above criteria, you should still thank the contributor for the work and identify what is done correctly. Explain what is problematic in the patch and add any helpful information you can provide from your testing. Change the issue status from "Needs review" to "Needs work." See How to give constructive feedback.
Needs review If you have only reviewed one portion of the patch, post what you have reviewed and what still needs to be reviewed. If you find nitpicky issues with changed lines, do not mark the issue as "Needs work." Instead, post a patch that fixes the nits and an interdiff and leave the issue status as "Needs review."
To see how others approach the review process please see:
- YesCT's Video - Get good reviews, give good reviews. Faster.
- xjm's Guide to Patch Reviews
- webchick's 6 Pass Patch Reviews
The following are some helpful tips and things to keep in mind while reviewing patches:
- Keep a text editor open for typing and write down your thoughts immediately as you are reviewing, or use Dreditor. When you are done, you can edit your notes into a more structured review comment.
- Take a look at both the big picture and the little details.
- If you think the patch takes the wrong approach, make an alternative suggestion.
- Don't stop reviewing at the first sign of trouble. If there are bugs, imagine how things should have worked. If there are usability problems, try to think of a better interface.
- Is it a hack? Does the patch deal with the underlying issues it's trying to solve, or is it a quick fix or papering over more serious issues? In some cases quick or partial fixes are needed if they're blocking progress elsewhere, but if something looks like a hack, then it's unlikely to be accepted.
- Pay attention to what the submitter has said about the patch. If things are not clear, write down your questions and post them in the comments. Questions can trigger productive discussions.