Reviewing patches

Every change made to Drupal core is the result of an associated issue and patch. Since there are only one or two people at any one time who can commit code to the development version of Drupal, and several thousand patches are submitted for each release, the review process is an essential part of core development.

Changes in contributed modules tend to be made by the module maintainers themselves, but there are often patches to review, especially to some of the larger modules.

An important thing to note is that anyone who has some basic familiarity with Drupal can review patches, you don't need to know any PHP or MySQL, or even javascript or HTML to make a useful contribution.

Choosing a patch to review
The contributor and development blocks (enable these in your user profile if you haven't already) have links to both patch queues and resources for development.

The easiest place to start is patches needing review against the development version of Drupal core (currently Drupal 7), since this tends to have the most recent and active issues.

Patches are submitted for everything from adding a missing period in a code comment, to fixing a typo in a variable name, to thousand of lines rewriting major parts of core. Bug fixes tend to be easier to review for those new to the patch review process since there is (usually) less consideration needed compared to architectural changes.

If you're not sure where to start, there are always people willing to help you review patches, often ones they've written themselves, in #drupal on IRC.

The patch review process
Before looking at a patch, it's useful to familiarize yourself with the issue it changes. Issues posted and the discussions following them should ideally identify the behavior which needs to be changed, outline proposed changes and reasons for them, and provide those changes in the form of a patch. Whereas posting a patch is often the last stage of an issue for some open source projects (with a higher number of committers), in Drupal it's often only an early milestone in the process between posting the original issue and committing to core.

Patches usually need a positive review by at least two people other than the original author before being set to RTBC. In the normal course of development, patches will go through several iterations, often with changes by multiple authors before making their way into core. Although this process isn't foolproof, it guarantees that nearly all code committed to core has been peer reviewed by a number of people, has been refined for performance, readability, usability, and will not introduce regressions or new bugs into the system.

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 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, 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. In most cases:

• Mark the newer issue duplicate, unless the more recent issue has more discussion and an up to date patch on it
• 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

"Won't fix", and "by design"

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

Reading patches
There are some common criteria all patches need to comply with before they'll be considered for core (and ideally for contrib too).

• Is the patch in unified diff format? +/- is good >< is bad
• 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?
• Is the patch likely to cause performance issues? Does it need benchmarking
• Does the patch add theme functions or .tpl.php files where appropriate?
• If new pages are added, are they are 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 this apply only to the specific area it's affecting in core, or can it be applied to other areas and in contrib? Drupal aims for loosely coupled, extensible code, so if possible new additions to core should be optimized for re-use, and to avoid duplication.

If the patch has lots and lots of code style errors (tabs all over the place, poor commenting), 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 minor code style errors, then make a note of these, but continue to testing the actual patch.

Applying patches
You can get technical information on applying patches and setting up a development environment in other pages in this section. If the patch doesn't apply to your Drupal installation set the issue status to "patch - code needs work". A patch message of "offset" is usually acceptable, but "fuzz" or "failed hunks" mean that the patch needs to be updated due to changes in the code since it was created. If you apply patches to a CVS checkout, it's much easier to roll fixes in yourself and create a new patch, which is often quicker than detailing the problems when typing up the review.

Testing patches
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 existing issues in core itself.

• Confirm the patch does what it's supposed to (fix a bug, change a string, modify a form)
• Confirm that it doesn't break any of Drupal's automated tests - this is now done automatically when the patch is submitted to the issue queue, but can take some time.

Other things to consider

• Write down your thoughts as you are reviewing, not afterwards.
  Keep a text editor open for typing in and write down your thoughts immediately. When you are done, clean it up and structure it.
• Take a look at both the big picture and the details.
  Simply saying "I [don't] like this feature" or "-1" is of no use and is strongly discouraged. Similarly, diving into a patch and saying nothing but "there is a typo in function so and so" could be a waste of time as you could be continuing a patch that 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
  When posting your issue follow-up, it is important to think critically and speak positively. 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 currently envisions. 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.