Last updated November 3, 2013.
This page describes how to contribute by reviewing patches. Patches are pieces of code that solve an issue either in Drupal core or a contributed module.
In general, the criteria for patch review are:
Does it fully comply with Drupal coding standards?
If the patch includes tests, are they both necessary and sufficient?
Does the patch actually solve the problem?
Hint: there should be a reproducible test case where the current code fails and the patched code succeeds.
- Yes to all of the above.
If the patch meets all of the above criteria, say so and change the issue status from Needs review to Reviewed and tested by the community.
- No to one or more of the above.
If you cannot in good conscience affirm all of the above, change the issue status from Needs review to Needs work and explain what is lacking.
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).
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 usually need a positive review by at least two people other than the original author before being declared Reviewed and tested by the community. In the normal course of development, patches will go through several revisions, often with changes 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, and usability (good), as well as regressions and bugs (bad).
An example of an approach to reviewing patches is here.
Patch review criteria
In general patches are reviewed along the following metrics. Note that to provide a useful review you only need to cover one or two of these aspects at a time - since ideally other developers will fill in any gaps in previous reviews as the issue progresses.
Assessing the issue as a whole
Before even looking at a patch, it's useful to consider the following questions:
- Is the issue a duplicate of an older one?
- Can you reproduce the problem described in the bug report?
- If it's a feature request, is the feature worthwhile? Is it best suited to core or a contributed module? Is it already implemented in a contributed module?
Marking issues as duplicate
Due to the large number of issues on Drupal.org, and the tendency for people to 'post first, search later', there are many duplicate issues - sometimes each has some discussion and even patches. Finding duplicate issues and marking them as such is a task anyone can do, and helps to focus everyone's efforts in one place, and ensure they are aware of the work going on elsewhere.
If you find two issues which seems to be about the same thing, mark one of them as duplicate:
- Mark the newer issue duplicate, unless the more recent issue has more discussion and a reviewable patch that the older issue lacks.
- If you mark an issue as duplicate, always add links between the issues so that it's easy to reference the duplicate and primary issue from each other
Marking issues as "won't fix", and "by design"
The issue statuses "won't fix" and "by design" should be used very sparingly - so in most cases it's safer to post your opinion first without changing status, and let another reviewer mark it as such if they agree.
There are some common criteria all patches need to comply with before they'll be considered for core and contributed code:
- Does the patch meet Drupal's coding standards?
- Is the code secure?
- If the patch makes major changes to a function or adds a new one, does it have appropriate code comments?
- Does the patch degrade performance and does it need benchmarking?
- Does the patch add theme functions or
.tpl.phpfiles where appropriate?
- If new pages are added, are they in a logical place? If forms or other UI elements are added, are they intuitive and logical to use? Does it come with appropriate contextual help? Is the interface consistent?
- If the patch is adding new functionality, does it apply only to the specific area in core, or can it be applied to other areas in core and even in contrib? Drupal aims for loosely coupled, extensible code, so new additions to core should be optimized for re-use and avoid duplication.
If the patch has lots of style errors (tabs instead of spaces, spaces at the end of a line, incorrect spelling, grammar, or punctuation in comments), then it's often quickest to note these in your comment and set it to 'needs work', or to re-roll without these errors - ideally with meaningful comments about the general approach of the patch. However if there are only a few minor style errors, then make note of them, but continue testing.
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.
- Confirm the patch does what it's supposed to (fix a bug, change a string, modify a form, etc.)
- For contributed modules that are not yet automatically tested, confirm that the patch doesn't break any of Drupal's automated tests. On Drupal 6, this requires downloading installing the Simpletest module. Simpletest is included in Drupal 7 and newer, but in either case, it must be enabled from the modules page.
Other things to consider
- Keep a text editor open for typing and write down your thoughts immediately, as you are reviewing. When you are done, you can cut and paste your notes into a more structured review comment.
- Take a look at both the big picture and the details.Merely saying "I [don't] like this feature" or "-1" is strongly discouraged. Similarly, focusing on details like "there is a typo in function so and so" could be a waste of time if the overall patch has no hope of being committed.
- No patch can save the world, not even a Drupal core patch. If it works and does something useful on its own, then it is good to go. One of the worst things you can do is elaborate on other, equivalent approaches and suggest complex extensions to the patch. Additional features can be added later. A scalpel is often better than a Swiss Army Knife.
- 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 get in.
- Pay attention to what the submitter has said about the patch. If some things are not clear, write down your questions. You should have a good idea of what the patch does and who it is for before you start.
- Be constructive, collaborative and respectful When posting your issue follow-up, it is important to think critically and speak positively, acknowledging the work that was put in. Posting simply +1 or -1 doesn't help to move patches forward, and can just add clutter for others trying to read the issue and follow up.
- Your goal is to propose an even better solution than the patch does. Do not post negative feedback without a corresponding solution. If a patch doesn't meet code style guidelines - at least point out where they are and link to appropriate resources - some people find bugs and post patches before they've ever heard of Drupal's coding guidelines. A good review helps a patch proceed, instead of obstructing it. It is a rare case that a submitted patch has absolutely no merit and warrants a purely negative review.
- Post your findings in a clear and structured manner to the original issue. If you can, include an improved patch.