How to review Drupal code
If you're interested in code quality and providing a means by which to bring Drupal beginners up-to-speed on the coding standards, I recommend reviewing code from all developers. I say "all" developers because everyone needs an editor.
The best way to force code reviews is to bake it into your development process. Use a tool like Gitlab (free hosted version) to prevent developers from committing code to authoritative branches. Instead, have them fork the project repository, and submit merge requests. Someone else can then review them. The reviewer can add in-line comments, wait for the developer to make changes, and then accept the request.
Here are some things to look for when reviewing Drupal code submissions. For some of these, we're assuming Git is being used for version control.
- Read, understand and follow the Coding standards.
- Install, enable, and use the Coder module on your development sites.
- For the purists out there, use the Coder Tough Love module as well.
- If you're running a continuous integration (CI) system like Jenkins, check the logs for new errors or warnings on new commits. Either way, make sure your development sandboxes have errors being reported to the screen so that developers can see any new errors that they generate. You'll find a lot of errors in your Drupal log if you're not doing this. (Make them refresh their DBs from your Dev site which already has this enabled.)
- Speaking of CI, add one or more code quality inspection tools to the mix such as SonarQube. There's actually a pre-configured Vagrant profile to build a VM with everything already set up. See CI: Deployments and Static Code Analysis with Drupal/PHP for details.
- Look for unrelated code reversions in merge requests. That is, if you see code changes that aren't related to what the developer is trying to do, there's something wrong. In most cases, this means the developer's branch is out-of-date with the main development branch. He or she should fetch and merge that branch from from the origin repository, fix any conflicts, and then add it to the merge request.
- Look for debugging code that wasn't removed such as dd(), drupal_debug() and other output functions.
- Look for Git conflict symbols such as "<<<", ">>>" and "===". These usually indicate a botched conflict resolution.
- Notice any lack of comments. Stanzas (small blocks of code that do little things) should be separated by blank lines, each with a comment explaining what it does. It may be clear to the original developer, but that doesn't help anybody else.
- Make sure that modules are installed in the right place. This is usually sites/all/modules/contrib (for upstream modules coming from drupal.org) or sites/all/modules/custom (for modules written specifically for the project).
- In theme files, usually somewhere under sites/all/themes, look for any functionality that is not theme-specific. Functionality should always be in modules, not themes, so that if the theme is changed, the site still works as expected. This is an extremely common error for beginners. For example, JavaScript files related to modules shouldn't be in the theme directory, but the module itself.
- Ensure consistency in module package names. For custom modules, it's advisable to give the package name the name of the project so that it's clear that these are site-specific. For contributed modules, use what others are using; don't arbitrarily make one up. This helps keep your list of modules organized.
These are the most common issues I've discovered while reviewing code. If you have any others, feel free to add them as comments. I can add them to the list here.
Happy reviewing!