Code-Review-Checkliste für Magento-2-Pull-Requests

1. Warum Magento-Pull-Requests eine eigene Checkliste brauchen

Ein generisches PHP-Code-Review prüft Lesbarkeit, Namenskonventionen und offensichtliche Bugs, übersieht aber die Fallen, die spezifisch für Magento sind. Eine Preference, die eine Core-Klasse überschreibt, sieht im Diff harmlos aus, kann aber mit jedem anderen Modul kollidieren, das dieselbe Klasse ebenfalls überschreiben will. Ein Code-Review ohne Magento-Kontext lässt solche Konflikte durchgehen, bis sie erst in der Integration mit einem Drittanbieter-Modul sichtbar werden.

Eine Magento-spezifische Code-Review-Checkliste ergänzt die generischen Qualitätskriterien um Punkte, die nur in diesem Framework relevant sind: deklaratives Schema statt imperativer InstallScripts, Plugin-Sortierreihenfolge, Backward-Compatibility-Grenzen der public API und typische Performance-Fallen wie N+1-Queries in Collections. Diese Punkte lassen sich in eine feste Checkliste gießen, die jedes Teammitglied bei jedem Pull Request konsequent durchgeht.

Dieser Artikel definiert genau diese Checkliste, Punkt für Punkt, mit konkreten Diff-Beispielen für jede Kategorie. Ziel ist ein Code-Review-Prozess, der Magento-typische Fehler zuverlässig vor dem Merge abfängt, statt sie erst in Produktion zu entdecken.

2. Deklaratives Schema statt InstallScripts prüfen

Der erste Punkt jeder Code-Review-Checkliste für Magento: Enthält der PR eine neue InstallSchema.php oder UpgradeSchema.php, obwohl deklaratives Schema über db_schema.xml seit Magento 2.3 der empfohlene Weg ist? Imperative Scripts sind schwerer zu diffen, laufen nur einmal und hinterlassen keine deklarative Quelle der Wahrheit für den aktuellen Tabellenzustand. Ein Reviewer sollte bei jedem neuen Scripts-Ordner nachfragen, warum deklaratives Schema hier nicht ausreicht.

Der zweite Punkt: Wurde nach einer Änderung an db_schema.xml auch die zugehörige db_schema_whitelist.json aktualisiert? Diese Datei wird von bin/magento setup:db-declaration:generate-whitelist generiert und muss im selben PR committet werden, sonst wird die Schema-Änderung beim nächsten setup:upgrade in einer anderen Umgebung nicht angewendet. Dieser Punkt der Code-Review-Checkliste wird erfahrungsgemäß am häufigsten vergessen.

<!-- app/code/Vendor/Module/etc/db_schema.xml -->
<!-- Review checklist: does this table addition have a matching whitelist entry? -->
<schema xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="urn:magento:framework:Setup/Declaration/Schema/etc/schema.xsd">
    <table name="vendor_custom_entity" resource="default" engine="innodb" comment="Custom Entity">
        <column xsi:type="int" name="entity_id" padding="10" unsigned="true" nullable="false"
                identity="true" comment="Entity ID"/>
        <column xsi:type="varchar" name="code" nullable="false" length="64" comment="Unique Code"/>
        <constraint xsi:type="primary" referenceId="PRIMARY">
            <column name="entity_id"/>
        </constraint>
        <constraint xsi:type="unique" referenceId="VENDOR_CUSTOM_ENTITY_CODE">
            <column name="code"/>
        </constraint>
    </table>
</schema>

3. Plugins vs. Preferences im Diff erkennen

Eine der wichtigsten Prüfungen in jeder Code-Review-Checkliste: Ersetzt der PR eine Core-Klasse per preference in di.xml, obwohl ein plugin denselben Effekt erzielen würde? Preferences erlauben genau eine Implementierung pro Interface, während Plugins von mehreren Modulen unabhängig voneinander registriert werden können. Ein Reviewer, der eine neue Preference sieht, sollte immer fragen, ob stattdessen ein before-, after- oder around-Plugin ausreichen würde.

Bei Plugins selbst ist die Sortierreihenfolge (sortOrder) der zweite kritische Punkt. Zwei Plugins auf derselben Methode ohne explizite sortOrder-Werte verhalten sich abhängig von der Modul-Ladereihenfolge nicht deterministisch. Eine gute Code-Review-Checkliste verlangt, dass jedes neue Plugin einen expliziten sortOrder setzt und im PR-Kommentar begründet, warum diese Reihenfolge relativ zu bestehenden Plugins gewählt wurde.

<?php
declare(strict_types=1);

// WRONG in review: preference replaces the entire class,
// blocking any other module from customizing the same behavior
// <preference for="Magento\Catalog\Model\Product" type="Vendor\Module\Model\Product" />

// RIGHT: plugin targets only the specific method, coexists with other modules
namespace Vendor\Module\Plugin;

use Magento\Catalog\Model\Product;

/**
 * Adjusts product price display for custom business logic.
 */
class ProductPricePlugin
{
    /**
     * Applies a surcharge to the final price for flagged products.
     *
     * @param Product $subject Product being processed
     * @param float $result Original price
     * @return float Adjusted price
     */
    public function afterGetFinalPrice(Product $subject, float $result): float
    {
        if ($subject->getData('requires_surcharge')) {
            return $result * 1.05;
        }
        return $result;
    }
}

4. Backward Compatibility und public API Grenzen

Magento markiert Interfaces und Klassen im Namespace Api sowie mit @api-Annotation als öffentliche API, für die eine Backward-Compatibility-Garantie gilt. Ein Code-Review muss prüfen, ob eine Methodensignatur in einer solchen Klasse geändert, ein Parameter hinzugefügt oder ein Rückgabetyp verschärft wurde, denn das sind alles Breaking Changes für Drittmodule, die gegen diese API entwickeln.

Ein häufiger Fehler in Pull Requests: Ein neuer Pflichtparameter wird einer bestehenden öffentlichen Methode hinzugefügt, ohne einen Standardwert zu setzen. Das kompiliert lokal problemlos, bricht aber jede externe Implementierung, die diese Methode aufruft oder überschreibt. Die Code-Review-Checkliste sollte an dieser Stelle explizit verlangen, dass neue Parameter in öffentlichen Interfaces immer einen Standardwert erhalten oder über ein neues, separates Interface eingeführt werden.

Auch das Entfernen von als @deprecated markiertem Code gehört in diese Prüfung: Magentos eigene Policy verlangt mindestens zwei Minor-Versionen Übergangszeit, bevor deprecated Code tatsächlich entfernt werden darf. Ein PR, der diese Frist unterschreitet, sollte im Review zurückgewiesen werden, unabhängig davon, wie sauber der restliche Code ist.

5. Performance-Red-Flags im Code-Review

Bestimmte Code-Muster tauchen in Magento-Pull-Requests immer wieder auf und sind fast immer ein Performance-Problem: Collection::load() innerhalb einer Schleife erzeugt eine neue Datenbankabfrage pro Iteration, statt die Filterung vor der Schleife einmalig auf der Collection selbst durchzuführen. Ein Reviewer, der eine Collection-Instanziierung innerhalb eines foreach sieht, sollte das als klaren Kandidaten für ein N+1-Problem markieren.

Ein zweiter Red Flag für die Code-Review-Checkliste: Direkte ObjectManager::getInstance()-Aufrufe außerhalb von Factories oder Test-Bootstrapping. Das umgeht Dependency Injection vollständig, macht die Klasse untestbar und versteckt echte Abhängigkeiten. Jeder Fund sollte im Review zu einer verpflichtenden Umstellung auf Constructor Injection führen, bevor der PR gemergt wird.

<?php
declare(strict_types=1);

// WRONG: N+1 query pattern flagged in review
foreach ($orderIds as $orderId) {
    $order = $this->orderCollectionFactory->create()
        ->addFieldToFilter('entity_id', $orderId)
        ->getFirstItem();
    // process $order
}

// RIGHT: single collection query, filtered once with an array
$orders = $this->orderCollectionFactory->create()
    ->addFieldToFilter('entity_id', ['in' => $orderIds]);
foreach ($orders as $order) {
    // process $order
}

6. Sicherheits-Checks: SQL Injection, XSS, ACL

SQL-Injection-Risiken entstehen in Magento fast immer durch String-Konkatenation in Direct-SQL-Aufrufen statt über den Query-Builder mit gebundenen Parametern. Eine Code-Review-Checkliste muss jeden Aufruf von getConnection()->query() mit direkt eingebetteten Variablen als kritischen Fund behandeln und die Umstellung auf bind-Parameter oder den Zend-Framework-Query-Builder verlangen.

Bei phtml-Templates ist ungeprüfte Ausgabe von Benutzereingaben der häufigste XSS-Vektor. Magentos $escaper-ViewModel bietet escapeHtml(), escapeUrl() und escapeJs() für genau diesen Zweck, und ein Review sollte jede direkte echo- oder Kurzschreibweisen-Ausgabe ohne Escaping-Aufruf zurückweisen. Zusätzlich gehört die ACL-Prüfung in jeden Adminbereich-Controller: Fehlt ein _isAllowed()-Override oder ein passender adminhtml_acl-Eintrag, kann jeder Admin-Nutzer unabhängig von seiner Rolle auf die neue Funktion zugreifen.

<?php
declare(strict_types=1);

namespace Vendor\Module\Controller\Adminhtml\Report;

use Magento\Backend\App\Action;

/**
 * Displays the custom report grid, restricted to a dedicated ACL resource.
 */
class Index extends Action
{
    /**
     * ACL resource required to access this controller.
     */
    public const ADMIN_RESOURCE = 'Vendor_Module::custom_report';

    /**
     * Checks whether the current admin user is allowed to view this page.
     *
     * @return bool
     */
    protected function _isAllowed(): bool
    {
        return $this->_authorization->isAllowed(self::ADMIN_RESOURCE);
    }
}

7. Coding Standards und statische Analyse im PR

Der Magento Coding Standard (magento/magento-coding-standard) und PHPStan auf mindestens Level 5 gehören als automatisierte Vorprüfung vor jedes manuelle Code-Review. Ein Reviewer sollte niemals Zeit mit Formatierungsdiskussionen verbringen, die ein CI-Check bereits automatisch erzwingen könnte. Die Code-Review-Checkliste sollte deshalb explizit voraussetzen, dass bin/phpcs und bin/analyse ohne Fehler durchlaufen, bevor der PR überhaupt zur manuellen Review freigegeben wird.

Für Teams, die diese Prüfungen noch nicht in CI automatisiert haben, ist ein Pre-Merge-Gate mit GitHub Actions oder GitLab CI der pragmatischste erste Schritt. Der manuelle Reviewer kann sich dann vollständig auf die Magento-spezifischen Punkte dieser Checkliste konzentrieren, statt Einrückung und Namenskonventionen von Hand zu prüfen.

#!/usr/bin/env bash
# Pre-review automated gate — run before requesting a manual code review
set -euo pipefail

echo "[1/3] Magento Coding Standard"
bin/phpcs --standard=Magento2 app/code/Vendor/Module

echo "[2/3] PHPStan static analysis"
bin/analyse app/code/Vendor/Module --level=5

echo "[3/3] Declarative schema whitelist check"
bin/magento setup:db-declaration:generate-whitelist --module-name=Vendor_Module --dry-run

echo "[OK] All automated checks passed — ready for manual review"

8. Testabdeckung und PR-Größe

Ein Pull Request ohne Unit- oder Integrationstests für neue Business-Logik sollte in jeder Code-Review-Checkliste automatisch als unvollständig gelten, unabhängig davon, wie gut der restliche Code aussieht. Besonders Plugins und Observer, die produktionskritisches Verhalten verändern, brauchen mindestens einen Test, der das erwartete Verhalten vor und nach der Änderung dokumentiert.

Die PR-Größe selbst ist ein oft unterschätzter Reviewfaktor. Ein Pull Request mit über fünfhundert geänderten Zeilen wird in der Praxis seltener gründlich gelesen als ein fokussierter PR mit unter hundert Zeilen. Teams, die große Features entwickeln, sollten diese in mehrere kleinere, unabhängig review- und mergefähige Pull Requests aufteilen, statt ein einziges riesiges Review-Paket zu erzeugen, das am Ende nur oberflächlich geprüft wird.

9. Review-Kategorien und Werkzeuge im Überblick

Die folgende Tabelle ordnet jede Kategorie der Code-Review-Checkliste dem passenden Werkzeug oder Prüfschritt zu.

Je mehr Punkte der Code-Review-Checkliste automatisiert in CI laufen, desto mehr Zeit bleibt dem Reviewer für die Punkte, die zwingend menschliches Urteilsvermögen erfordern: Architekturentscheidungen, Backward-Compatibility-Risiken und Sicherheitsfragen, die kein Linter zuverlässig erkennt.

10. Zusammenfassung

Eine belastbare Code-Review-Checkliste für Magento-2-Pull-Requests deckt vier Kategorien ab, die ein generisches PHP-Review übersieht: deklaratives Schema statt InstallScripts, Plugin- statt Preference-Nutzung mit korrekter Sortierreihenfolge, Backward-Compatibility-Grenzen der public API und Performance-Red-Flags wie Collection-Queries in Schleifen. Ergänzt um Sicherheitschecks für SQL Injection, XSS und ACL wird daraus eine vollständige Prüfung.

Der größte Hebel liegt in der Automatisierung: Alles, was ein CI-Gate mit phpcs, PHPStan und dem Whitelist-Check zuverlässig prüfen kann, sollte nie manuell im Review diskutiert werden. Das schafft Raum für die Punkte der Code-Review-Checkliste, die tatsächlich menschliches Urteilsvermögen brauchen, allen voran Architekturentscheidungen und Backward-Compatibility-Risiken.

11. FAQ: Code-Review-Checkliste für Magento-2-Pull-Requests