Patch Review Session for Drupal 7 Core
Patch Review Session for Drupal 7 Core
Update: This was posted prior to Drupal.org's move from CVS to git. For information on how to use git for Drupal patches, start here.
Last week Ariane was in on #drupal on irc.freenode.net and heard some of the core developers talking about how badly backlogged the patch review queue is. Apparently there are many good patches that haven't been reviewed and therefore haven't made it into the current release. Each of these little patches helps to make Drupal more robust and with the Drupal 7 code freeze creeping up, this is an area that needs extra help. At Affinity Bridge we have been talking about how to contribute more to Drupal core and this seemed like a good opportunity to get involved. We decided to put some of our time into helping out and realized we could use a little mentoring to get started. Ariane put the word out on Twitter and soon we had a volunteer to help us. Yesterday we were lucky enough to have Karoly Negyesi (aka chx on Drupal.org) come by and teach us about core patch reviews and the Drupal issue queue.
We started by taking a tour of the issue queue. Some good places to start are at the novice queue or the main patch queue and some of the most common ways you can help out are:
- re-rolling patches - often the test bot fails and the patch needs to be re-submitted.
- giving critical reviews of patches - install the patch, does it fix the problem? Remember, try and think of reasons it isn't ready to be committed and report back on what you found.
- weeding out old/invalid patches - going to the last page of the patch queue is a great place to find issues like this. If you find something that is already fixed or doesn't apply anymore (maybe that feature was removed?) then mark it as "won't fix".
By taking a look at some examples, Karoly guided us through what the appropriate action might be. One of the things we found out? Just be assertive! Adding tags to issues (such as the "usability" tag), marking as "won't fix", or even re-rolling failed patches are all simple things that you can do to help out.
Crash course
Angie Byron (aka webchick on Drupal.org) also joined online to help us through. She gave us a very helpful five-step process for reviewing a patch on a current checkout from CVS using command line:
Step 1: Change into the Drupal root directory which is probably something like 'cd /Sites/htdocs/drupal7'
Step 2: Download the patch from the issue thread on drupal.org:curl -lO http://drupal.org/path/to/file/my-patch.patch
Step 3: Apply it to your local Drupal install: patch -p0 < my-patch.patch Here you should test out the patch and make sure it does what is intended. Providing feedback on whether or not it works, on code style, or on other problems are all extremely valuable contributions.
Step 4: Roll the patch! (ie. re-create the patch) cvs diff -up > my-patch.patch
Step 5: Post it to the issue queue! When you do this, make sure to change the issue status from "needs work" to "needs review", otherwise the testing bot won't retest it. To attach your patch to your reply on the issue, use the "Attach a file" field on the form.
Step 6: Go back to Square One - this resets your code base to the current revision of Drupal: cvs up -dPC
Common issues
A problem that we saw a couple of times was when the test bot failed on a good patch. One hint that this was a problem was when the bot reported that it failed to install for a patch that didn't have any installation-related code. One way to help clear up a problem like this is to re-roll the patch and post it with the "needs review" status. This tells the bot to retest it and the issue moves forward. Another hint that we found for getting your own issues reviewed is to report it against Drupal 7. Since Drupal 7 is under current major development, it is more likely that someone will take a look. If you find an issue in Drupal 6, try it in Drupal 7 and then provide some feedback if the problem still exists.
Resources
If you have some time and are looking for help or people that need help reviewing patches, check out #drupal on IRC and it's likely that someone can find you some work. For more detailed information on this workflow, check out the Applying Patches handbook page. Something else we found useful was the page on reverting patches. Some articles that Angie pointed out were:
Patch Queue Quick Glossary
roll/re-roll - create a patch that can apply to HEAD. Usually this involves using the command "cvs diff -up > patch.patch"
patch - a .patch file that contains information about how to change code
diff - a comparison of the changes on your copy of Drupal and the main copy of Drupal
code freeze - a point in time when the community will not be adding new features to the current version of core. The Drupal 7 code freeze is scheduled for September 1, 2009.
Finally, issue queue statuses, more information on this handbook page:
- needs work - this patch was reviewed but has problems
- needs review - this patch has been looked at and is not ready to be used
- RTBC/reviewed and tested by community - also thought of as "ready to be committed" by some, this patch looks good and will be applied to Drupal core
Thanks to Robin for taking photos!