Code Review Process for Magento

Review Purpose

• Catch bugs before they reach production
• Share knowledge across the team
• Ensure code quality and consistency
• Improve maintainability

Review Checklist Template

## Code Review Checklist

### Functionality
- [ ] Code does what it claims
- [ ] Edge cases handled
- [ ] Error handling is appropriate

### Code Quality
- [ ] Follows PSR-12 coding standard
- [ ] No code duplication
- [ ] Meaningful variable/function names
- [ ] Functions are single-purpose

### Security
- [ ] Input validation present
- [ ] SQL injection prevention
- [ ] XSS prevention
- [ ] CSRF protection
- [ ] No hardcoded secrets

### Performance
- [ ] No N+1 queries
- [ ] Proper indexing used
- [ ] No unnecessary loops
- [ ] Cache considered

### Testing
- [ ] Unit tests included
- [ ] Edge cases tested
- [ ] Integration tests if needed

Review Workflow

# 1. Developer creates PR
git push origin feature/cart-widget
# Creates PR targeting develop

# 2. Assign reviewers
# Request 1-2 team members

# 3. Reviewers provide feedback
# Inline comments on specific lines
# General summary comment

# 4. Developer addresses feedback
# Push additional commits

# 5. Re-review if needed

# 6. Approve and merge

Key Takeaway

Code reviews catch bugs, share knowledge, and maintain quality. Use checklists for consistency and provide constructive, specific feedback.

PHP Code Issues

// BAD: Unused variables
$order = $this->orderFactory->create();
$customer = $this->customerFactory->create(); // unused

// BAD: Deep nesting
if ($condition1) {
    if ($condition2) {
        if ($condition3) {
            // Hard to read
        }
    }
}

// GOOD: Early returns
if (!$condition1) {
    return;
}
if (!$condition2) {
    return;
}
// Main logic at normal indentation

Security Issues to Flag

// BAD: SQL injection vulnerable
$orderId = $_GET['id'];
$connection->query("SELECT * FROM orders WHERE id = {$orderId}");

// GOOD: Parameterized query
$orderId = $this->request->getParam('id');
$connection->query(
    "SELECT * FROM orders WHERE id = ?",
    [$orderId]
);

// BAD: XSS vulnerable
echo $product->getName();

// GOOD: Escaped output
echo $this->escapeHtml($product->getName());

// BAD: No CSRF protection
// Controller accepts POST without form key validation

// GOOD: CSRF validation
$formKey = $this->formKey->getFormKey();
// Validate form key in controller

Performance Issues

// BAD: N+1 query
$orders = $orderCollection->load();
foreach ($orders as $order) {
    $customer = $order->getCustomer(); // separate query each time
}

// GOOD: Join or preload
$orders = $orderCollection->join(
    ['customer' => 'customer_entity'],
    'customer_id = entity_id',
    ['customer_email']
);

// BAD: Loading collection in loop
foreach ($productIds as $id) {
    $product = $this->productRepository->getById($id);
}

// GOOD: Load all at once
$products = $this->productRepository->getByIds($productIds);

Key Takeaway

Look for unused code, deep nesting, security vulnerabilities, N+1 queries, and missing input validation. Flag issues with specific suggestions.

DI and Configuration

<!-- BAD: Wrong argument type -->
<type name="Vendor\Module\Model\Order">
    <arguments>
        <argument name="orderRepository" xsi:type="object">
            Vendor\Module\Model\OrderRepository
        </argument>
    </arguments>
</type>

<!-- GOOD: Use interface -->
<type name="Vendor\Module\Model\Order">
    <arguments>
        <argument name="orderRepository" xsi:type="object">
            Magento\Sales\Api\OrderRepositoryInterface
        </argument>
    </arguments>
</type>

Observer Review

// BAD: Heavy logic in observer
class SaveOrder implements ObserverInterface
{
    public function execute(EventObserver $observer)
    {
        // 200 lines of processing
        // Database queries
        // API calls
    }
}

// GOOD: Observer delegates to service
class SaveOrder implements ObserverInterface
{
    public function execute(EventObserver $observer)
    {
        $order = $observer->getEvent()->getOrder();
        $this->orderProcessor->process($order);
    }
}

Plugin Review

// BAD: Plugin too broad
class ProductPlugin
{
    public function aroundGetPrice($subject, $proceed)
    {
        // Modifies ALL product prices
    }
}

// GOOD: Specific plugin with condition
class ProductPlugin
{
    public function aroundGetPrice($subject, $proceed)
    {
        if ($this->isSpecialPromotion($subject)) {
            return $this->calculateSpecialPrice($proceed());
        }
        return $proceed();
    }
}

GraphQL Review

# BAD: Exposing too much
type Query {
    orders(filters: OrderFilterInput): OrderConnection
    customers: CustomerConnection  # Expose all customers
}

# GOOD: Scoped access
type Query {
    customerOrders: OrderConnection  # Current user's orders only
}

Block Template Review

// BAD: Business logic in template
echo $block->getPrice() * $block->getQuantity();

// GOOD: Logic in block class
public function getFormattedTotal(): string
{
    return $this->priceFormatter->format(
        $this->getPrice() * $this->getQuantity()
    );
}

Key Takeaway

Review Magento-specific patterns: use interfaces in DI, keep observers thin, limit plugin scope, secure GraphQL queries, and keep logic out of templates.

Feedback Categories

### Blocking Issues (Must Fix)
- Security vulnerabilities
- Data loss risks
- Breaking changes without migration
- Critical bugs

### Suggestions (Should Consider)
- Performance improvements
- Code clarity
- Better naming
- Test coverage gaps

### Nits (Optional)
- Style preferences
- Minor refactoring
- Documentation improvements

Comment Examples

# BAD: Vague
"This is wrong"
"Fix this"
"Bad code"

# GOOD: Specific and helpful
"This query may cause N+1 issue. Consider using a join here to load customer data with the order collection."

"Consider adding input validation for this parameter. Currently a malicious value could cause unexpected behavior."

"This method does too many things. Consider extracting the tax calculation into a separate service class."

# GOOD: Positive feedback
"Nice use of the repository pattern here. Makes testing much easier."

Review Metrics

- Review turnaround: < 24 hours
- Review size: < 400 lines changed
- Comments per review: 3-10 average
- Re-review rate: < 30% (first review quality)

Magneto Code Sniffer Integration

# Run before review
vendor/bin/phpcs --standard=PSR12 app/code/Vendor/Module/

# Fix auto-fixable issues
vendor/bin/phpcbf --standard=PSR12 app/code/Vendor/Module/

# PHPStan for static analysis
vendor/bin/phpstan analyse app/code/Vendor/Module/ --level=6

Key Takeaway

Provide specific, actionable feedback. Categorize issues as blocking, suggestions, or nits. Keep reviews under 400 lines. Use automated tools for style checks.