laravel-code-review

Laravel Code Review

Purpose

Catch Laravel-specific issues before they reach production. Run this after the generic superpowers:requesting-code-review flow, or as a standalone review for Laravel-specific concerns.

Review Checklist

Work through each section. For every [ ] item, check the actual code — don't assume.

---

1. Database & Eloquent

N+1 Queries

# Check for relationship access inside loops
grep -rn "foreach\|->each(" app/Http/ app/Services/ app/Actions/
# Then verify: is `with()` used before the loop?

• All hasMany/belongsToMany relationships loaded inside loops use eager loading (with())
• $collection->count() called on already-loaded collection (OK), not ->count() (fires query) — distinguish $user->posts->count() from $user->posts()->count()
• ->exists() or ->doesntExist() used instead of counting for existence checks

Mass Assignment

• Every model that is mass-assigned has $fillable defined (prefer) or $guarded = [] (intentional)
• $request->all() is NOT used for create()/fill() — use $request->validated() or $request->only([...])
• New columns in migrations are reflected in the model's $fillable

Migrations

• New migration does not break existing data (nullable or has default if column is added to existing table)
• Foreign keys have constrained() and appropriate onDelete behavior
• No php artisan migrate:fresh assumption in production migrations

Queries

• No Model::all() without pagination or explicit small-dataset justification
• env() is NOT called in application code — only config() (env() bypasses config cache)

---

2. Authorization

• Every controller method or route that accesses user-specific data calls authorize() or uses a Policy
• Policies cover both the "allowed" case and the "forbidden" case (tested in Pest)
• authorize() is called before querying the database (fail fast)
• Routes that should be guest-only are protected by guest middleware
• Routes that require auth have auth middleware (not just relying on authorize() inside)

# Check for missing authorization
grep -rn "public function " app/Http/Controllers/ | grep -v "__construct\|middleware"
# For each method, verify it calls $this->authorize() or has a policy

---

3. Validation

• All user input is validated before use
• Form Requests used for complex validation (not inline $request->validate() if reused)
• Validation covers ALL fields (check for $request->input('field') without prior validation)
• Nested/array validation uses dot notation: 'items.*.price' => 'required|numeric'
• $request->validated() used (not $request->all() or $request->input()) after validation

---

4. Security

• No raw SQL with user input: DB::select("... WHERE id = '$id'") → use bindings
• {!! $var !!} (unescaped Blade) only used for explicitly trusted HTML
• File uploads: MIME type validated, stored outside public dir or via Storage::disk()
• Sensitive data (passwords, tokens) not logged
• API tokens / secrets come from config() not hardcoded
• CSRF protection: POST/PUT/DELETE forms have @csrf or API routes use Sanctum/Passport

---

5. Performance

• Pagination used for any list endpoint (->paginate(), ->simplePaginate(), ->cursorPaginate())
• Heavy operations dispatched as Jobs, not run synchronously in the request
• Expensive computed values cached where appropriate (cache()->remember(...))
• API Resources / Transformers used instead of ->toArray() for response shaping

---

6. Error Handling

• External API calls wrapped in try/catch with meaningful error handling
• Queue Jobs implement $tries, $backoff, and failed() method where appropriate
• HTTP client failures (Http::get(...)) check ->failed() or ->throw()
• Custom exceptions are used for domain errors (not raw Exception or RuntimeException)

---

7. Test Coverage

• Happy path has a Pest Feature test
• Authorization: both allowed and forbidden cases tested
• Validation: at least one "missing required field" or "invalid format" case tested
• Facade fakes used for Mail, Queue, Event, Notification where relevant (Mail::fake() etc.)
• N+1 regression: if a relationship is eager-loaded, a test verifies query count

php artisan test --coverage   # needs XDEBUG_MODE=coverage

---

8. Code Style & Conventions

• Follows the project's existing pattern (Actions vs Services vs fat controllers)
• php artisan pint (Laravel Pint) runs clean — no style violations
• Model relationships follow naming: hasMany → plural method name (posts()), belongsTo → singular (user())
• Controller methods follow resource naming: index, create, store, show, edit, update, destroy
• New Artisan commands follow module:action naming (orders:process-overdue)

./vendor/bin/pint --test   # check style without fixing
./vendor/bin/pint          # fix style

---

Summary Report Format

After reviewing, report findings grouped by severity:

Blockers (must fix before merge):

• [File:Line] — [issue] — [why it matters]

Should fix (important but not blocking):

• [File:Line] — [issue]

Nice to have (optional improvements):

• [File:Line] — [suggestion]

Looks good (explicit sign-off areas):

• Auth: covered
• Validation: complete
• Tests: green and meaningful

---

Quick Commands

php artisan test              # run full test suite
./vendor/bin/pint --test      # style check
php artisan route:list        # verify routes
php artisan model:show Model  # check schema/relationships
php artisan queue:failed      # check for failed jobs