Code Review in Magento: What to Actually Look For

The Non-Negotiables: Security

These must be caught in every review. No exceptions, no "we'll fix it later."

Input sanitization and output escaping. Any value that comes from user input, URL parameters, or the database and is rendered in HTML must be escaped. In Magento templates, use `$block->escapeHtml()`, `$block->escapeUrl()`, `$block->escapeJs()` — the correct method depends on context. Generic `htmlspecialchars()` is not always correct.

SQL injection via raw queries. Any custom SQL should use Magento's Zend_Db adapter with parameterized queries, not string concatenation. If you see `$connection->query("SELECT * FROM " . $tableName . " WHERE id = " . $id)`, reject it.

Mass assignment in custom forms. Custom form handlers that use `$this->getRequest()->getParams()` without whitelisting the accepted fields are vulnerable to mass assignment. The handler should explicitly list which fields are processed.

Admin action CSRF protection. Custom admin actions must validate the form key. Magento's `_processUrlKeys()` handles this for standard controllers — verify it's not been disabled or bypassed.

ACL on custom API endpoints and admin routes. New admin routes need explicit ACL rules. New REST API endpoints need authentication requirements. The default is not always restrictive enough.

The Non-Negotiables: Performance

Performance problems from code review are the ones that look fine in development and catastrophic under real load. Catch them in review, not in production.

N+1 queries in collection loops. The classic Magento performance bug: iterating over a collection and loading a related model for each item inside the loop. The fix is always to join or preload. Review any code that calls `->load()` inside a foreach loop over a collection.

Attribute loading without joins. `$product->getData('custom_attribute')` after loading a product by ID only gives you the attribute if it was joined in the collection load. Calling `getAttribute()` individually inside a loop produces N queries. The correct pattern uses `addAttributeToSelect()` on the collection.

Synchronous external API calls in the storefront critical path. Any call to an external service (shipping API, tax service, ERP) in the checkout critical path must have a timeout, a circuit breaker, and a fallback. A blocking call with no timeout will eventually hang a checkout session. Review any `file_get_contents`, `curl_exec`, or Guzzle call in storefront code.

Missing cache declarations on blocks. Custom blocks that extend `AbstractBlock` are cached by default. Blocks that extend `Template` with dynamic content need explicit cache declarations or cache disable. Verify the `getCacheKeyInfo()` implementation returns unique keys for blocks that vary by user or session context.

Architecture Signals to Catch in Review

These aren't immediate production risks but are the decisions that produce year-2 problems. Catching them in review is much cheaper than refactoring them later.

ObjectManager direct usage. `\Magento\Framework\App\ObjectManager::getInstance()` is an anti-pattern in all contexts except factory classes, test setup, and integration points where DI is not available. Any use outside these contexts is a code smell that breaks testability. In some cases, the code reaching for ObjectManager is doing work that doesn't belong in Magento at all — heavy integration logic, data transformation pipelines, or real-time API orchestration. When you see this pattern, it's worth asking whether a dedicated microservice is the right architecture instead of forcing more logic into Magento's PHP runtime.

Around plugins used for simple modifications. Around plugins (`aroundMethod`) are the most powerful and the most expensive plugin type — they wrap the entire original method, preventing Magento's optimization pass. Before/after plugins are correct for most use cases. An around plugin is justified when you need to conditionally suppress the original method or modify both the input and output. "I used around because it was easier" is not sufficient.

Magento event observers that do too much. Observers should be thin. If an observer contains significant business logic, that logic should be in a separate service class that the observer calls. This makes the logic testable and the observer readable.

Direct database access bypassing the repository pattern. Custom code that uses `$connection->query()` directly instead of Magento repositories and collections breaks Magento's transaction management and makes the code dependent on the specific schema rather than the abstract interface.

Magento-Specific Anti-Patterns

These are patterns that are specific to Magento and won't be caught by a reviewer who knows PHP but doesn't know Magento.

Preference injection for interface implementations that aren't custom. Using preferences in `di.xml` to override a Magento class (rather than a plugin) is usually wrong. Preferences are appropriate for implementing a new class for an interface; they're wrong for intercepting behavior in an existing class. Plugins are correct for interception.

Custom modules that load all attribute sets for every product page. A common pattern in catalog customizations is loading all attribute sets to determine product configuration — often unnecessary if the relevant attributes are available on the loaded product.

Admin grids built with a direct collection instead of a search criteria + repository. Admin grids that use raw collections bypass Magento's search engine abstraction and produce queries that don't work correctly with third-party search (Elasticsearch) or future Magento versions.

Cron jobs with unconstrained scope. A cron job that processes all orders, all products, or all customers in a single run without batching or locking will eventually OOM or deadlock under real data volumes. Every cron job should have a batch size limit and a mutex.

Building a Code Review Culture

Consistent code review requires more than a checklist — it requires norms about what makes a review good and what makes it useful to the developer receiving it.

Reviews should explain, not just reject. "N+1 query here" is less useful than "This loads the shipping address inside the order loop. Use `$order->getShippingAddress()` once before the loop and reference it inside." The explanation teaches; the rejection only corrects.

Separate blocking issues from suggestions. Use a clear convention: blocking comments (the PR cannot merge without addressing this) vs suggestions (improvements worth considering). Mixing them makes it unclear what's required.

Performance and security reviews are not optional for senior developers. The pattern where only the most senior reviewer catches performance issues means the performance knowledge is concentrated. Code review is a knowledge transfer mechanism — use it as one.

Review the tests, not just the code. A PR that adds a feature without tests, or adds tests that only test the happy path, has incomplete review. The tests are part of the deliverable.