One of the main responsibilities of a Drupal core committer is doing a final review of patches before committing them. Since July 1, 2014, I’ve committed over 250 patches to Drupal 8. Thanks to Chapter Three for making that possible.
Reviewing code is time consuming. Here are some things that make my job easier:
- Well written issue summaries
- Screenshots of before and after, if the user interface has changed
- Evidence that the person who sets the issue status to "Reviewed & tested by the community" has considered Drupal’s core gates
- A draft change record or a list of change records that need updating, if necessary
This process can be overkill for a one line patch, but once the patch is more than a couple of lines is helps the reviewer a great deal.
There is a part of reviewing that feels a bit like playing Papers, Please, but some of the most mundane parts are automatable.
My current IDE, PHPStorm, allows me to inspect code for obvious mistakes. After applying the patch from the issue to review, I select Code > Inspect Code… from the menu.
By setting the scope to be "Uncommitted files," PHPStorm will only inspect the files changed by the patch. PHPStorm will then detect unused variables, spelling mistakes, PHPDoc issues, and a plethora of other common problems. My favorite inspection is a duplicate array key, as this can be quite difficult to spot.
PHPStorm has helped me find bugs in Drupal. For example, while reviewing a patch that changed core/includes/theme.inc, the inspection informed me that it was unable to find the JSON class. As a result, I created https://www.drupal.org/node/2217755 which is now a critical bug because it prevents the language switcher block from working.
If you set up PHPStorm to use PHP Codesniffer and get the sniffs for Drupal installed, the code inspection tool will also check the code against our coding standards. Note that it is entirely possible to use this without PHPStorm.
Drupal has not yet reached the level of automation of other projects like Symfony, which does an automated code style review on all contributions, but hopefully the recent work on testbot infrastructure by jthorson and others will get us there.
I look forward to reviewing your patches for Drupal 8!
