fromAugust 2015

Catche That Typo!

Peer Code Review in Drupal

Photo The core Drupal community is notorious for obsessing over every little detail that is submitted as code. Single issues can have hundreds of comments and, at its very worst, can take years to be resolved.

As a community, though, we know that this obsession results in a much better product. Code quality comes at a cost: time. It is nearly impossible to both comprehensively review code and commit code quickly. But the upfront time costs for peer review will save you time down the line. Teams I've worked with have caught typos, security vulnerabilities, broken styles… you name it, we've caught it before it was deployed, thanks to the peer review process.

The remainder of this article outlines the step-by-step process needed to conduct a peer code review.

Working on New Code

Each time you start new work, make sure your local environment is as pristine as possible. Ideally, this would also include downloading a fresh copy of the database from your production server to ensure there are no half-baked Feature settings which could dirty your export.[1]

With the build environment as clean as possible, you're ready to start.

  1. Check out the branch which holds the development branch that you will later merge your work back into. This is typically called dev, and the command is git checkout dev.
  2. Ensure the branch is up-to-date. Ideally this is done with rebasing (git pull --rebase=preserve) or, in a simplified workflow, you would use merging strategy to update the branch (git pull).
  3. If there are Feature updates, you may need to import these changes into your local database. You can do this with drush fra --force --yes.
  4. Create a new b­ranch.
    git checkout -b XXX-terse-description.

    XXX represents the ticket number from your bug tracker, and terse-description should be replaced with a description of the problem.

You are now ready to begin working on your branch.

Go for it!

Exporting Your Code for Review

Once your work is complete, you're ready to share your work with your teammates.

  1. If your site is using Features, export your feature. (If it's a new feature, I typically do this through the web interface, otherwise I use the command drush fu).
  2. See what has changed, using git status. Look for features which have not been cleanly exported and any other files which may have changed unexpectedly. Using the command git diff, you can review each of the files to see what has changed with the command git diff.
  3. If everything looks in order, add your changes git add -a, and commit your changes to the repository with git commit. Your text editor will open. Write a detailed commit message outlining why you made the changes you did.
  4. Upload your changes to the server:
    git push remote_name XXX-terse-description. The name of your remote is probably origin. To get a list of all remotes connected to your local repository, use: git remote show.

You are now ready to ask for a review of your code.

Importing Code for Review

So you have been asked to review code. Fantastic! The first thing you need is a copy of the branch which contains the new code:

  1. Download the latest version of all remote branches:
    git fetch.
  2. List all remote branches: git branch --remote.
  3. Create a local review copy of the relevant branch:
    git checkout --track remote_name/XXX-terse-description
  4. If there are features, activate them in your database:
    drush fra --force --yes.
  5. Ensure everything displaying in the browser is up-to-date by clearing Drupal's cache: drush cc all.

Conducting Your Review

Take a look at the proposed changes with the following commands:

  • Review the log of commit messages:
    git log --oneline --graph.
  • Compare the code against the current version of the development branch: git diff dev.

When conducting a review, you should ensure that the new code:

  1. Solves the problem identified in the ticket (or adds the Feature as described);
  2. Conforms to relevant coding standards (including spell-checked against the correct dictionary);
  3. Is limited to the scope identified in the ticket;
  4. Does not introduce any new regressions (including performance regressions, layout bugs, etc).

Hopefully, the code passes your inspection. If it doesn't, it's a good thing you were there to catch things!

Report any problems back to the original coder via your bug tracker. You may report your findings as a comment on the original issue, or via an inline code review if that functionality is supported by your bug tracker of choice.

Accepting and Merging a Branch

Assuming the code passed review, you may now merge the approved code back into the shared development branch.

  1. Check out a local copy of the shared development branch. git checkout dev.
  2. Ensure your local copy of the development branch is up-to-date: git pull --rebase=preserve.

Ensure that the work you're about to merge into the development branch is also up-to-date.

  • git checkout XXX-terse-description
  • git rebase dev

Assuming there were no rebasing errors, you are now ready to merge the two branches together. (If there are errors, the original developer is the most qualified to fix the problem. Ask them to bring their work up-to-date.) The final merge is done from the destination branch.

  • git checkout dev
  • git merge --no-ff XXX-terse-description

The updated branch can now be pushed back up to the server for others to base their work on.

  • git push

Once the work has been merged in, you can clean up the local and remote copies of the ticket branches.

  • git branch -d XXX-terse-description
  • git push remote_name --delete XXX-terse-description

Customize to Suit Your Needs

This process was generalised for the article. You may add a few extra steps, especially regarding who is responsible for merging in the code (an automated gatekeeper? the original developer?).

Please do copy this article into your local documentation! Hack it up and customise it to suit your needs.

[1] The Features module allows you to export database settings to code. This allows you to easily transfer configuration information for Views, Content Types, and more from one environment to another.

Image: "I scribble a lot" by Nic McPhee is licensed under CC BY 2.0