Reviewing a Magento Change

Five checks for every Magento diff: the object manager, the plugin target, a query in a template, a missing escape, a forgotten whitelist.

Syntaxbin/magento setup:upgrade && bin/magento setup:di:compile && bin/magento cache:flush

Magento hides its failures behind commands. A change can read correctly, work in developer mode, and then break after setup:di:compile or in production mode. Reviewing a Magento diff therefore means looking for a specific short list of mistakes rather than reading for general quality.

The five

The object manager. A direct ObjectManager::getInstance() call, or an injected ObjectManagerInterface, hides the real dependency from DI. Inject the interface through the constructor. For an object DI cannot build — one that needs runtime data — inject its factory.
The plugin target. Interception cannot reach a final method, a final class, a private or protected method, a static method, __construct, __destruct, a virtual type, or a class implementing Magento\Framework\ObjectManager\NoninterceptableInterface. A plugin declared on any of them is simply never called, with no error. Check also that an around plugin is doing something before or after could not, and that it calls $proceed.
A query in a template. A .phtml should ask a view model for data it already has. Magento2.Templates.ObjectManager and Magento2.Templates.ThisInTemplate catch the common forms.
Escaping. Every value printed from a template goes through $escaper, with the method matching the context: escapeHtml, escapeHtmlAttr, escapeUrl, escapeJs.
The whitelist. A column, index or constraint added to db_schema.xml needs a matching entry in etc/db_schema_whitelist.json.

Then run it

Confirm the author ran the commands their change needs: setup:upgrade for schema and data patches, setup:di:compile for anything touching a constructor or di.xml, cache:flush for configuration and layout.

Example

<?php
// app/code/Vendor/Loyalty/ViewModel/RewardedProducts.php
declare(strict_types=1);

namespace Vendor\Loyalty\ViewModel;

use Magento\Catalog\Api\Data\ProductInterface;
use Magento\Catalog\Api\ProductRepositoryInterface;
use Magento\Framework\Api\SearchCriteriaBuilder;
use Magento\Framework\View\Element\Block\ArgumentInterface;

/**
 * Where the query belongs: a view model the template asks for data.
 */
class RewardedProducts implements ArgumentInterface
{
    public function __construct(
        private readonly ProductRepositoryInterface $productRepository,
        private readonly SearchCriteriaBuilder $searchCriteriaBuilder
    ) {
    }

    /**
     * @return ProductInterface[]
     */
    public function getProducts(int $limit = 4): array
    {
        // addFilter($field, $value, $conditionType = 'eq') binds the value.
        // Never build the condition by concatenating it into a string.
        $searchCriteria = $this->searchCriteriaBuilder
            ->addFilter('loyalty_reward', 1, 'eq')
            ->setPageSize($limit)
            ->create();

        return $this->productRepository->getList($searchCriteria)->getItems();
    }
}

// Wired into a template through layout XML as a block argument, so no
// controller or route is involved. On the storefront the block renders the
// matching products; an empty result renders nothing rather than erroring.
// After adding this class and its layout argument:
//   bin/magento setup:di:compile && bin/magento cache:flush

When to use it

Review a pull request that adds a plugin, before it is merged and silently does nothing.
Decide which bin/magento commands a change requires before writing the deployment note.
Explain to a contributor why their db_schema.xml column never appeared in the database.
Audit an inherited module for direct object manager use before taking it into production.

More examples

The same constructor written long

Worth knowing in both directions, because core and most third-party modules use the long form while new code tends to use promotion. DI sees no difference: it reads the parameter types either way. In review the thing to check is the same in both forms, which is that every parameter is an interface or a factory and none of them is ObjectManagerInterface.

<?php
// app/code/Vendor/Loyalty/ViewModel/RewardedProducts.php (constructor only)
declare(strict_types=1);

namespace Vendor\Loyalty\ViewModel;

use Magento\Catalog\Api\ProductRepositoryInterface;
use Magento\Framework\Api\SearchCriteriaBuilder;
use Magento\Framework\View\Element\Block\ArgumentInterface;

class RewardedProducts implements ArgumentInterface
{
    private ProductRepositoryInterface $productRepository;

    private SearchCriteriaBuilder $searchCriteriaBuilder;

    public function __construct(
        ProductRepositoryInterface $productRepository,
        SearchCriteriaBuilder $searchCriteriaBuilder
    ) {
        $this->productRepository = $productRepository;
        $this->searchCriteriaBuilder = $searchCriteriaBuilder;
    }
}

// Identical to the promoted form in the primary example. Constructor
// property promotion (PHP 8.0+) declares the property and assigns it from
// the parameter list; these nine lines are what it desugars to. Magento
// core is still written in the long form, so you will read both.

A plugin declaration a reviewer can check

Reading the plugin's target class before reading the plugin itself is the fastest check in Magento review, because a plugin on an unreachable method is not a bug report about wrong output, it is a bug report about nothing happening at all. Note that the plugin class is not listed anywhere else: this file is what makes it load. The declaration takes effect after bin/magento setup:di:compile followed by bin/magento cache:flush.

<!-- app/code/Vendor/Loyalty/etc/di.xml -->
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">

    <!-- The target is an interface, so every implementation is intercepted.
         getList() is public and not final, so interception can reach it. -->
    <type name="Magento\Catalog\Api\ProductRepositoryInterface">
        <plugin name="vendor_loyalty_flag_rewarded_products"
                type="Vendor\Loyalty\Plugin\Catalog\FlagRewardedProducts"
                sortOrder="10"/>
    </type>

    <!-- Rejected in review, for reasons a reviewer must know by sight:
         - a plugin on __construct is never called
         - a plugin on a final or static method is never called
         - a plugin on a protected or private method is never called
         - a plugin on a virtual type is never called
         An `around` method here would also need to call $proceed, and only
         earns its place if it must skip the original call entirely. -->
</config>

The whitelist entry that goes with a schema change

The file is a record of everything the module has ever declared, and it is what allows a later release to drop a column it owns. In review, a diff that adds tier_code to db_schema.xml and does not change this file is incomplete: the column is created on setup:upgrade, but the module can never drop it again until the entry exists. Check the two files move together, and that the whitelist was regenerated by the command rather than typed.

// app/code/Vendor/Loyalty/etc/db_schema_whitelist.json
{
    "vendor_loyalty_balance": {
        "column": {
            "entity_id": true,
            "customer_id": true,
            "points": true,
            "tier_code": true,
            "updated_at": true
        },
        "index": {
            "VENDOR_LOYALTY_BALANCE_CUSTOMER_ID": true
        },
        "constraint": {
            "PRIMARY": true,
            "VENDOR_LOYALTY_BALANCE_CUSTOMER_ID_UNIQUE": true
        }
    }
}

// Never hand-edited. Regenerate after every db_schema.xml change:
//   bin/magento setup:db-declaration:generate-whitelist --module-name=Vendor_Loyalty
//   bin/magento setup:upgrade

Discussion

Be the first to comment on this lesson.