Magento 2 Code Review Checklist for Pull Requests

Code review in a Magento project has to catch more than style issues. A pull request that looks clean can still disable full page cache for the whole catalog, run a query per product in a listing, or open an XSS hole in a template. Most of those problems are invisible in a diff unless you know where to look.

This is the checklist I use when reviewing Magento 2 code. It assumes the boring parts are automated, so the review can focus on what tools do not catch.

Before reading the code

Automate everything that can be automated. A reviewer should not be pointing out indentation or missing type hints. In CI, before anybody looks at the pull request:

• PHPCS with the Magento2 standard on changed files, see the PHPCS article
• PHPStan at an agreed level, see the PHPStan article
• Unit tests and, where they exist, integration tests
• setup:di:compile, which catches broken constructor arguments and missing classes

Then read the description. A good pull request says what changes, why, and how to test it. If the description is missing, ask for it first. Reviewing code without knowing its goal is guessing.

Architecture and DI

• No direct ObjectManager. ObjectManager::getInstance() in a module hides dependencies and breaks testing. Ask for constructor injection.
• Interfaces over concrete classes. Depend on ProductRepositoryInterface, not on the product resource model, when the repository does the job.
• Right extension point. A preference that replaces a whole core class for one method change is a red flag. A plugin or an observer is usually enough. The tradeoffs are in plugins vs observers vs preferences.
• Around plugins only when needed. An around plugin that always calls $proceed and changes the result should be an after plugin. Around plugins make stack traces longer and are easier to break.
• No logic in constructors. Constructors assign dependencies and nothing else. Work in a constructor runs every time the object is created, including during compilation and in unrelated requests.
• Heavy dependencies behind proxies. CLI commands and plugins on common classes that inject something expensive should use a proxy in di.xml.
• Area specific DI. A plugin that only matters on the storefront belongs in etc/frontend/di.xml, not the global one.

Performance

This is where most expensive mistakes hide.

Loading inside loops

foreach ($order->getAllItems() as $item) {
    $product = $this->productRepository->getById($item->getProductId());
    $brands[] = $product->getData('brand');
}

One load per item, each with all attributes. Ask for a single collection with only the needed attribute and a filter on the IDs, or for the data to be read from the order item if it is already there.

Collections without limits

A collection without a page size, used in a cron or an admin action, works with fifty records in development and runs out of memory with fifty thousand in production. Ask for batching with setPageSize() and setCurPage(), or a query that reads only IDs.

Full page cache

Search the diff for cacheable="false" in layout XML. A single block with that attribute disables full page cache for every page that uses the handle. On default.xml or catalog_product_view.xml that means the whole store. Customer specific content should come from customer data sections or a separate AJAX request instead.

Observers on hot events

An observer on catalog_product_load_after or controller_action_predispatch runs constantly. Check what it does. If it loads data or calls an external API, it needs a cheap early return or a different event.

External calls in the request

An HTTP call to an ERP or a shipping API during checkout makes the checkout as slow and as unreliable as that API. Ask for timeouts, error handling, and a queue when the result is not needed immediately.

Security

• Escaping in templates. Every dynamic value in a .phtml file goes through $escaper: escapeHtml, escapeHtmlAttr, escapeUrl or escapeJs depending on context. A plain echo $block->getSomething() is a finding.
• SQL with bound values. Values go through placeholders or quoteInto, never string concatenation.
• ACL on admin controllers. Admin controllers define ADMIN_RESOURCE with a resource declared in acl.xml. Missing or generic ACL means any admin user can run the action.
• Form keys and HTTP methods. Controllers that change data implement HttpPostActionInterface and validate the form key.
• Web API permissions. A new endpoint in webapi.xml with anonymous access needs a very good reason. Check that customer endpoints use self and read the customer ID from the token, not from the request.
• Secrets. API keys belong in encrypted config or env.php, never in code or in plain config.xml.
• Logging. Logs must not contain card data, passwords, tokens or full customer records.

A small example of the SQL point:

$select = $connection->select()
    ->from($this->resource->getTableName('sales_order'), ['entity_id'])
    ->where('customer_email = ?', $email)
    ->where('created_at >= ?', $from);

Database and data changes

• Declarative schema. Schema changes go in db_schema.xml with an updated db_schema_whitelist.json. No new InstallSchema or UpgradeSchema scripts. See declarative schema and data patches.
• Data patches are idempotent. Running a patch twice, or on a store where the data already exists, must not fail or duplicate data.
• Indexes for new queries. A new filter on a large table needs an index, and a new index on a big table needs a plan for deploy time.
• Backward compatible changes. Dropping or renaming a column while old code may still be running breaks rollbacks and zero downtime deploys.

Configuration

• New settings have defaults in config.xml and the right showInDefault, showInWebsite and showInStore flags in system.xml.
• Code reads configuration with the right scope, usually store scope on the storefront.
• app/etc/config.php changes match the module changes. A new module that is missing from config.php will not be enabled in production.
• Environment specific values are not committed in config.php.

Frontend

• Template overrides copy only what is needed. Copying a large core template to change one line means losing every future core fix to it. A layout change, a plugin on the block or a small child template is usually better.
• JavaScript changes to core components use RequireJS mixins instead of replacing whole files.
• New JS and CSS are loaded only on pages that need them, not in default.xml.
• Text shown to customers is translatable with __() in PHP and $t in JS.
• For Hyva projects, Alpine components follow the theme conventions and do not pull in jQuery or RequireJS based code.

Tests and deploy impact

• Business logic has unit tests. Wiring that matters, such as a plugin that changes a price, has an integration test or a clear manual test plan.
• The pull request states whether it needs setup:upgrade, a reindex, a cache flush or a queue consumer restart. These change how it is deployed.
• New cron jobs and consumers are documented with their schedule and expected run time.
• Removed modules or features have a plan for leftover data and configuration.

How to give the feedback

Separate blocking issues from suggestions. Security problems, data loss risks and performance regressions are blocking. Naming preferences are not. Explain why something is a problem with a concrete scenario, such as what happens with a large catalog or a second store view. And when the code is good, say so. A review that only lists problems teaches people to dread the next one.

FAQ

What should be automated instead of reviewed by hand?

Coding standard, static analysis and tests. Run PHPCS with the Magento2 standard, PHPStan and the unit tests in CI so the reviewer can spend time on design, performance and security.

What is the most common Magento performance problem in pull requests?

Loading models or collections inside loops, and cacheable=false in a layout handle used on many pages. Both look harmless in a small diff and hurt a lot in production.

Is using the ObjectManager directly always wrong?

In application code, yes, with very few exceptions such as some factories and backward compatible constructor changes in core. In a module, ask for constructor injection instead.

How big should a Magento pull request be?

Small enough that the reviewer can understand every file. One feature or fix per pull request, with schema changes, configuration and code together so the change can be deployed and reverted as a unit.

I am available for freelance Magento and Adobe Commerce work, including code reviews and audits of existing modules. If you need help with this, get in touch.