Code review best practices for Adobe Commerce

Last update: September 29, 2023

CREATED FOR:

  • Experienced
  • Developer

The main goal of code review is to validate that implemented functionality meets the requirements in an optimal way from performance, architectural, and security perspectives.

In addition, code review is intended to make sure code meets Adobe Commerce development best practices, is easy to understand and maintain, and follows coding standards. Most coding standards should be checked automatically by appropriate tools.

At a high level, the code review process should consist of the following steps:

  1. Checkout feature branch on local development environment.
  2. If it’s been awhile since code was submitted for review, merge the latest changes from the target branch (usually QA branch), and push updates to the remote feature branch (if any).
  3. Do a high-level review to validate that the code implements all requirements. If there are major discrepancies, contact the developer for clarification.
  4. Running the code is optional, but if you have doubts that the code works as expected or if it facilitates the efficiency of the process, test that the implemented functionality works as expected for main usage scenarios. If there are major issues, stop the review process, and reopen the ticket with appropriate comments.
  5. Do a thorough review to validate that the code implements all requirements. If there are any issues, add appropriate comments to the pull request and reopen the ticket.
  6. If everything looks good, merge the pull request (or pass it to next code review level if one has been established).

Also, consider the following points when when implementing code review processes:

  • Code review is a primary tool for communication and knowledge transfer within a team. Even if a pull request doesn’t contain major issues and implements the required business logic, you can use it as an opportunity to suggest more improvements after it’s been merged.

  • On average, code review shouldn’t take more than 10% to 20% of development effort. That’s assuming that the development team consists of senior engineers that work well together. If code review takes longer, it can affect project budget and timeline.

  • Address issues with excessive effort required for code reviews by identifying the root cause. Usually, it’s because either requirements are poorly translated into development tickets (due to communication issues, poor user stories) or it’s a coaching issue. In the first case, the recommended mitigation is to make sure that tickets have enough information for team members to efficiently deliver the requirements. In the second case, you might need to adjust the team structure to include more senior engineers or get approval outside mentoring and coaching.

Affected products and versions

All supported versions of:

  • Adobe Commerce on cloud infrastructure
  • Adobe Commerce on-premises

What to look for in code review

Style

Style can be tested automatically by running the PhpStorm inspection (see below).

Make sure to configure PHPMD and PHPCS and to run the Coding Standard tool from the CLI (also below). There is some overlap, but both also have unique tests.

Convention and structure

Reviews for convention and structure are done manually.

  • Is class functionality limited to a single responsibility?
  • Does the directory structure make sense?
  • Is functionality performed at the right level (server, client, CSS, JS, database, framework, infrastructure).
  • Is the versioning correct?
  • Does the code look unconventional or like it is trying to get around a problem in the wrong way?
  • Is the naming convention for the module name, package name, and repository name correctly applied?
  • Verify that global CSS styles are applied thoughtfully and are not overused.

Completeness

Reviews for completeness are done manually.

  • Can the code be enable or disabled by configuration and does all necessary code behave as expected?
  • Is all configuration present that is mentioned in the ticket? Check scope, data type, validation, translation, and default values.
  • Is configuration always retrieved at the lowest possible level (store view level, website level, or global level)? Configuration retrieval must match the definition of scope in the system.xml file.
  • Are all paths in the flow chart of the technical specification covered? Are all other technical specifications covered?
  • Are ACLs defined for the new functionality?
  • Are PhpDocs clear? Are commit messages clear?
  • Is any code commented out or do you see debug code?

Performance

Reviews for performance are done manually, which can be aided by code execution when in doubt.

  • Are queries executed in a loop? This loop can be outside of the edited files.
  • Can you spot any cachable="false" attributes? Are they applied correctly?

Security

Reviews for security are done manually, which can be aided by text search. Part of the security check is taken care of by automated tests.

  • Are exceptions logged when needed? Are the right types of exceptions used?
  • Can around plugins be avoided?
  • Do plugins return the correct types of data?
  • Can you find any raw SQL queries that should be built using the database abstraction layer?
  • Is any new type of data exposed to any type of user, admin, or frontend? Is that exposure a security risk?
  • Is user-generated data validated? Everything that comes from the browser is considered user generated, including cookie values and server headers.

Privacy and GDPR

Reviews for privacy and GDPR are done manually.

  • Does the code handle customer data or emails? Pay special attention.
  • If this code can be executed in a loop, can it leak customer data from one loop cycle to another?
  • Indicators of a risk are imports, cron jobs, transactional emails, and batch queue handlers.
  • Ensure the isolation of user data in loops. Adobe advises using factories or repositories to create models in the loop cycle, which are not accessible outside of the loop.