Your Team's Technical Guide to Drupal Code Reviews
Drupal code reviews are an important part of the software development process. If you've recently started working with Drupal or you are looking to improve your codebase and processes, you may be wondering what you should consider when reviewing changes to your Drupal codebase. We'll describe how to get the most out of your Drupal code reviews by providing a checklist of important steps and criteria for you to return to and reference as you open and review new pull requests.
Creating A Pull Request
A well-formatted and qualified pull request helps streamline the code review process. It's also a good point for an author to pause and consider the change they've made. Before submitting a pull request, the author should consider the change from the perspective of other team members, such as the designer, the reviewer, and the QA team. Reflecting on the change can offer additional insight into the problem as well as reduce cycle time as the change moves through different stages in the development process. Consider if the change meets the requirements in the most efficient manner.
Before Opening A Pull Request
The code review process begins before the pull request is even opened. The author should review the requirements and ensure the change appropriately addresses them. A developer doesn't need to be as thorough as the QA team members but a basic level of testing should be applied, including verification in multiple browsers to catch inconsistencies.
Additionally, this is a good time to help out future developers on the project. Make sure the change meets any coding standards that have been established, is well commented on, and the commits show a clear story. The Coder project provides a set of rules for PHP_CodeSniffer to check and apply Drupal coding standards automatically.
▢ Sync the latest changes from the develop branch
▢ Make sure the change meets the documented requirements
▢ Perform appropriate testing and review in multiple browsers
▢ Run a test build to make sure the project builds correctly
▢ Run any linting or standards tools set up for the project
▢ Ensure that commits have useful messages
Opening A Pull Request
The pull request will set the stage for the reviewer. When opening a new pull request, set the reviewer up for success. Provide any relevant information to establish the context of the change. You don't need to copy and paste the ticket description but provide enough information for the reviewer to understand the change (and maybe a link to the full description). If your commit messages are clear and your repository automatically includes commits in the description, this will help provide a starting point.
Additionally, review the diff before opening the pull request. It's important to make sure no unexpected or unintended changes were included in the change. Formatting differences between IDEs are a common issue. These should be resolved before opening the pull request. When making changes to Drupal's configuration, make sure you didn't accidentally capture incorrect values. If you're using config splits, double-check that split config has been applied to the default and vice-versa.
▢ Review the diff to make sure only intended changes were included
▢ Write a useful pull request description including context, questions, or discussion points
Reviewing With Empathy
First and foremost, a code review is about feedback. It's a learning opportunity for both the author and the reviewer. When it goes well, we all become better developers and the codebase becomes stronger as a result. Keep this in mind when reviewing code and review with empathy. In general, assume good intentions.
The advice included in the section is applicable to all code reviews, not just Drupal reviews. For a deeper dive into positive code reviews, check out these resources:
For The Author
Authors should approach the review as a discussion. Pull requests are an opportunity to discuss your change with other developers on the team. Explain why your approach was used and reference edge cases or other considerations that were factors in the solution. In turn, listen to the reviewer's comments and consider how they might alter your approach.
Remember, you are not your code. Suggestions are meant to be constructive and helpful. Try not to take comments personally. Code reviews are a great way to grow your knowledge and learn new patterns and considerations when developing software.
For The Reviewer
Reviewers should approach the review as a discussion. Code reviews are an opportunity to discuss a solution among peers. Many developers assume the reviewer has absolute authority over the change—try to avoid this trap. Instead, ask open questions to facilitate discussion. Offer explanations for your suggestions and include examples. If you disagree with the approach, pause and consider if your solution is better or just different. Remember you can learn as much from the author as they can learn from you.
Remember, your words have power. Try to stay humble and keep your ego in check while reviewing code. Consider how your comments might be taken, particularly if they are only offered in impersonal text on a pull request. Don't request changes directly unless there is a convincing argument for a different approach. Give kudos as well as critiques.
Ultimately, stay humble and take your time. Make sure you understand the goal of the change and the approach the author chose to take. Consider the change from multiple directions. Acknowledge your shortcomings and note when an author has done something interesting or clever (plus, this can also help fight imposter syndrome on the team!).
Performing A Review
Code review is also a technical discussion of the change. It's important to understand the common pitfalls in order to keep the codebase in good shape. Performing a technical review of the code helps address architecture issues and may reduce bugs before they become a problem.
General Approach
In general, the goal of a code review is to facilitate discussion of changes and make sure the development team is creating the best solution possible. As such, there are a lot of components to consider when reviewing a change. During the code review, we want to understand what the change is accomplishing, verify that the change is doing what it's intended to do, and fits in with the overall architecture.
▢ Do you understand the solution (does the solution "make sense")?
▢ Does the solution accomplish the goal?
▢ Does the change include any unrelated functionality?
▢ Does the solution support the overall project strategy?
▢ Do commits contain appropriate information (ticket number, description of change, additional context)?
Drupal Way
Drupal provides a toolkit for content-based website development. Each tool has an intended usage and sometimes more than one tool may be used to solve a problem. Some of these solutions will be better than others and the best solution may not always be obvious.
Consider the requirements and the intended use of a given system API, configuration, or application layer. The solution should make use of the correct APIs and occur in the correct part of the application. For example, is any front-end work implemented within the theme? Is shared business logic created as a service? Make sure the solution makes use of existing functionality where possible, usually in order by priority: Drupal core, contrib modules, custom code.
▢ Does the solution make use of existing functionality?
▢ Does the solution make the best use of Drupal's tools?
▢ Is the solution implemented in the appropriate application layer?
▢ Does the solution support a content management strategy?
Security
Drupal is a generally secure CMS and extensive efforts have been made by the community to support out-of-the-box security. However, any time custom code or community-contributed features may be involved, the security of the customizations should be considered. The change should make use of the appropriate features provided by Drupal to maintain the security of your application. Reference Writing Secure Code on Drupal.org for more information.
▢ Are Drupal's APIs being used correctly?
▢ Do SQL queries use the Database API?
▢ Are the text APIs such as t() applied to safely escape text and other scripting attacks?
▢ Is content rendered through Twig whenever possible? (Twig automatically escapes rendered content)
▢ Are permissions and access controls implemented properly?
▢ If a new entity or feature is implemented, does it have appropriate permissions applied?
▢ If a new permission is created, is it checked correctly?
▢ If JSON:API or other APIs are implemented, do API endpoints have appropriate access control and data filters in place?
API-first
Code should be written in a composable way using Drupal's API-first approach. Use services, plugins, etc. to create reusable functions that can be repurposed and edited easily. Drupal offers a powerful but complex dependency injection system. Make sure that functionality is accessed correctly to help keep the codebase DRY.
▢ Are the components of the solution appropriately (de-)coupled?
▢ Is dependency injection used correctly and are all dependencies necessary?
▢ Are new components abstracted to an interface, supporting dependency injection?
▢ Is the legacy hook system used when a more modern approach is available?
Documentation
Documentation is important for both future developers working on the project and existing developers who may need to reference a dependency while developing a separate feature. For architecture level changes, make sure patterns, strategies, and other important aspects are documented in an appropriate README. They may also be documented elsewhere but the codebase should at least contain a reference to external documentation.
Comments are also a key aspect of documentation. In particular, PHPDoc comments should be added to files, classes, functions, and properties to support IDEs that may automatically make this information available to developers. Make sure that complex code blocks have appropriate documentation to clarify the processes the code is following.
For example, complex algorithms or code flow conditions should include enough information that a new developer can understand the logic that was implemented. If the change applies a workaround or specific logic changed, reference appropriate documentation, such as a task ticket or external documentation link.
Drupal offers some specific opportunities to document functionality. For changes that don't include custom code, help text and README files should be included to document the functionality. When creating new content types and other entity bundles, include a useful description documenting the purpose of the new bundle. Any custom code should have appropriate descriptions, grouping, and dependencies listed in the module's info file.
▢ Are PHPDoc comments added to each file, class, method, and property?
▢ Are PHPDoc comments correctly formatted and complete (include all necessary properties)?
▢ Are complex code blocks explained with appropriate comments?
▢ Do workarounds or specific logic changes reference appropriate documentation?
▢ Are new variables or configuration values documented in the Drupal UI via help text and descriptions or in a README file included with the module?
▢ Do new modules have appropriate values in the .info.yml file?
▢ Do new modules include a README, if necessary, describing aspects of the module?
Code Style and Standards
Code should follow PHP and Drupal coding standards as well as any team- or project-specific standards the team has agreed upon. Following code standards help with code readability and integration. Code style and standards should generally be enforceable via automated analysis tools such as PHP_CodeSniffer.
Drupal.org provides a helpful guide for configuring this tool under Installing Code Sniffer. Usually, these should be run via Git hooks as well as validated by the CI/CD pipeline at appropriate stages but it's important to make sure that these tools were run correctly. Additionally, confirm that no unexpected but valid formatting changes have accidentally been applied. Examples include a change in indentation or quote style through an entire file.
▢ Does the code generally follow code style conventions established by the team?
▢ Are variables, functions, classes, etc. named appropriately?
▢ Did the automated validation tools run correctly? Did they note any errors?
▢ Does the change include any unexpected formatting changes?
Improve Your Codebase & Processes
For those working with Drupal and looking to improve their codebase and processes, this technical guide is the perfect starting point for your team. Whether you were wondering what you should consider when reviewing changes to your Drupal codebase or how you can make the most out of your Drupal code reviews, this guide provides you with an understanding of how to create useful pull requests and perform code reviews in a way that improves your team, as well as your codebase.
Please feel free to return to this guide as a reference for important review points as you review changes to your Drupal project. Try integrating these guidelines into your code review process to facilitate healthy conversations amongst your development team. By following these important checkpoints, your Drupal codebase will be more secure, more efficient, and easier to work with.
