|
1 | 1 | # Laravel — Code Review Checklist |
2 | 2 |
|
| 3 | +> Check sibling files for existing patterns before flagging any violation (see Consistency Check step). |
| 4 | +
|
3 | 5 | ## Eloquent & Database |
| 6 | + |
4 | 7 | - **Eloquent best practices**: use scopes, accessors, mutators; avoid raw queries unless necessary |
5 | 8 | - **Model casting**: casts defined for dates, enums, booleans, and serialized objects via the `casts()` method |
6 | 9 | - **Chunking large datasets**: `chunk()`, `cursor()`, or `lazy()` used when processing large volumes of records to avoid memory exhaustion |
7 | 10 | - **Aggregate helpers**: `withCount()`, `withSum()`, `withAvg()` used instead of loading collections just to aggregate |
8 | 11 | - **preventLazyLoading**: `Model::preventLazyLoading()` enabled in development to catch N+1 at dev time |
9 | 12 | - **Soft deletes**: `withTrashed()` / `onlyTrashed()` used intentionally; deleted records not accidentally leaked in queries |
| 13 | +- **Select only needed columns**: `SELECT *` avoided; specify columns explicitly in complex queries |
| 14 | +- **`whereBelongsTo()`**: used for cleaner relationship queries instead of manual `where('model_id', $model->id)` |
| 15 | +- **`whereIn + pluck`**: preferred over `whereHas()` for better index usage when filtering by related model IDs |
| 16 | + |
| 17 | +## Advanced Query Patterns |
| 18 | + |
| 19 | +- **`addSelect()` subqueries**: used over eager-loading entire has-many relations just to get a single aggregate value |
| 20 | +- **Dynamic relationships via subquery**: subquery FK + `belongsTo` used to avoid N+1 when accessing a single "latest" or "first" related record |
| 21 | +- **Conditional aggregates**: `CASE WHEN` in `selectRaw()` used instead of multiple count queries |
| 22 | +- **`setRelation()`**: used to prevent circular N+1 when manually constructing relationships |
| 23 | +- **Compound indexes**: match `orderBy` column order; correlated subqueries in `orderBy` for has-many sorting (avoid joins that multiply rows) |
| 24 | + |
| 25 | +## Caching |
| 26 | + |
| 27 | +- **`Cache::remember()`**: used over manual get/put pairs |
| 28 | +- **`Cache::flexible()`**: used for stale-while-revalidate on high-traffic data that can serve slightly stale content |
| 29 | +- **`Cache::memo()` / `once()`**: avoid redundant cache hits within a single request; `once()` for per-object memoization |
| 30 | +- **Cache tags**: used to invalidate groups of related cache entries atomically |
| 31 | +- **`Cache::add()`**: used for atomic conditional writes (set if not exists) |
| 32 | +- **`Cache::lock()`**: used for race conditions; `lockForUpdate()` for DB-level locking |
| 33 | +- **Failover stores**: `Cache::store()` with fallback configured in production |
| 34 | + |
| 35 | +## HTTP Client |
| 36 | + |
| 37 | +- **Explicit timeouts**: every outbound HTTP call has `timeout()` and `connectTimeout()`; no fire-and-forget calls without timeouts |
| 38 | +- **Retry with backoff**: `retry()` with exponential delays for external APIs; non-retriable errors (4xx) not retried |
| 39 | +- **Status check**: `throw()` or explicit status check after every response; no silent failures |
| 40 | +- **`Http::pool()`**: used for concurrent independent HTTP requests instead of sequential calls |
| 41 | +- **Testing**: `Http::fake()` + `Http::preventStrayRequests()` in all tests touching HTTP calls |
| 42 | + |
| 43 | +## Queues & Jobs |
| 44 | + |
| 45 | +- **`retry_after` > `timeout`**: `retry_after` in queue config must exceed the job's `$timeout`; prevents duplicate processing |
| 46 | +- **Exponential backoff**: `backoff()` returns `[1, 5, 10]` or similar; not relying on uniform retries |
| 47 | +- **`ShouldBeUnique`**: implemented on jobs that must not run concurrently; `WithoutOverlapping::untilProcessing()` for overlap prevention |
| 48 | +- **`failed()` always present**: every job implements `failed(Throwable $e)`; failures not silently discarded |
| 49 | +- **`retryUntil()` + `$tries = 0`**: when using `retryUntil()`, set `$tries = 0` to avoid early bail-out |
| 50 | +- **`RateLimited` middleware**: used for jobs calling external APIs; `Bus::batch()` for related parallel jobs |
| 51 | +- **`Bus::chain()`**: used correctly for sequential job dependencies |
| 52 | + |
| 53 | +## Events & Notifications |
| 54 | + |
| 55 | +- **`ShouldDispatchAfterCommit`**: events dispatching side effects (emails, notifications, external calls) use this to prevent firing before the DB transaction commits |
| 56 | +- **Event discovery**: manual registration avoided; `event:cache` run in production |
| 57 | +- **`ShouldQueue`**: notifications and mailables are queued; synchronous sending only for critical flows |
| 58 | +- **`assertQueued()`**: used in tests for queued mailables/notifications, not `assertSent()` |
10 | 59 |
|
11 | 60 | ## Controllers & Actions |
12 | | -- **Form Requests**: validation logic lives in Form Request classes, not controllers; `authorize()` method implemented correctly — not returning `true` by default on all protected requests |
13 | | -- **Single Action Controllers**: `__invoke` controllers used for simple, single-purpose actions instead of generic multi-method controllers |
14 | | -- **Action classes**: reusable business logic that doesn't belong to a Service extracted to invokable Action classes |
| 61 | + |
| 62 | +- **Form Requests**: validation in Form Request classes; `authorize()` not returning `true` by default on all protected requests |
| 63 | +- **Single Action Controllers**: `__invoke` controllers for simple single-purpose actions |
| 64 | +- **Action classes**: invokable Actions for reusable business logic that doesn't belong to a Service |
| 65 | +- **Controller methods**: under 10 lines; complex logic extracted to services or actions |
15 | 66 |
|
16 | 67 | ## Collections & Helpers |
17 | | -- **Collection pipelines**: `map`, `filter`, `reduce`, `groupBy`, etc. used instead of manual loops where clearer |
18 | | -- **Laravel helpers**: `Str::` and `Arr::` helpers used instead of raw PHP functions when they offer more clarity or safety |
19 | 68 |
|
20 | | -## Queues & Jobs |
21 | | -- **Unique jobs**: `ShouldBeUnique` implemented on jobs that must not be enqueued more than once simultaneously |
22 | | -- **Job batching & chaining**: `Bus::chain()` / `Bus::batch()` used correctly when jobs have sequential or parallel dependencies |
23 | | -- **Job configuration**: `$tries`, `$timeout`, and `backoff()` defined explicitly; not relying solely on global defaults |
| 69 | +- **Collection pipelines**: `map`, `filter`, `reduce`, `groupBy` used over manual loops |
| 70 | +- **`cursor()` vs `lazy()`**: `cursor()` for memory-efficient iteration without relationships; `lazy()` when relationship loading needed; `lazyById()` when updating records while iterating |
| 71 | +- **`toQuery()`**: converts a collection back to a query builder for bulk operations (update/delete) without loading models |
| 72 | +- **Laravel helpers**: `Str::` and `Arr::` over raw PHP functions; `Str::of()`, `$request->string()` for fluent string operations |
24 | 73 |
|
25 | 74 | ## Security & Auth |
26 | | -- **Policy enforcement**: `$this->authorize()` or `Gate::authorize()` called in controllers; not relying solely on route middleware |
27 | | -- **Sanctum token scopes**: API tokens issued with minimal required abilities; abilities checked explicitly in guarded routes |
| 75 | + |
| 76 | +- **Policy enforcement**: `$this->authorize()` or `Gate::authorize()` called in controllers; not relying solely on route middleware for authorization |
| 77 | +- **Sanctum token scopes**: API tokens issued with minimal required abilities; abilities checked explicitly |
| 78 | +- **Mass assignment**: `$fillable` or `$guarded` defined on every model |
| 79 | +- **`$request->validated()`**: only validated data used — never `$request->all()` or `$request->input()` without validation |
| 80 | + |
| 81 | +## Validation & Forms |
| 82 | + |
| 83 | +- **Form Request classes**: `$request->validated()` only — never `$request->all()` |
| 84 | +- **`Rule::when()`**: conditional validation rules; not inline ternaries |
| 85 | +- **Array notation**: `['required', 'email']` for new code; follow existing convention if project uses string pipes |
| 86 | +- **`after()` hook**: used instead of `withValidator()` for post-validation logic |
28 | 87 |
|
29 | 88 | ## Testing |
30 | | -- **withoutExceptionHandling**: used in tests that need to inspect the real exception, not the formatted error response |
31 | | -- **Database assertions**: `assertDatabaseHas`, `assertDatabaseMissing`, `assertSoftDeleted` used for persistence assertions |
32 | | -- **Factory states**: existing factory states reused instead of manually configuring models in every test |
| 89 | + |
| 90 | +- **`LazilyRefreshDatabase`**: preferred over `RefreshDatabase` for speed in large test suites |
| 91 | +- **`assertModelExists()`**: over raw `assertDatabaseHas()` when asserting model persistence |
| 92 | +- **Factory states**: existing states reused; not manually configuring models in every test |
| 93 | +- **Fakes after factories**: `Event::fake()`, `Mail::fake()`, etc. called after factory setup (not before), otherwise factory observers/events may be silenced |
| 94 | +- **`recycle()`**: shares relationship instances across related factories to avoid orphaned records |
| 95 | +- **`withoutExceptionHandling()`**: used in tests that need to inspect the real exception |
| 96 | +- **`Http::fake()` + `preventStrayRequests()`**: always in tests touching external HTTP |
33 | 97 |
|
34 | 98 | ## Routing & Config |
35 | | -- **Route model binding**: used where appropriate instead of manual `find()` + 404 checks |
36 | | -- **Config & env**: no `env()` calls outside of config files; all config accessed via `config()` |
37 | | -- **Named routes**: `route()` helper used with named routes; no hardcoded URL strings |
| 99 | + |
| 100 | +- **Route model binding**: used instead of manual `find()` + 404 checks |
| 101 | +- **Scoped bindings**: `->scopeBindings()` for nested resources |
| 102 | +- **`env()` only in config files**: application code accesses values via `config()` — never `env()` directly |
| 103 | +- **Named routes**: `route()` helper used; no hardcoded URL strings |
| 104 | +- **`App::environment()`**: used for environment checks; not `app()->environment()` string comparison via `env()` |
| 105 | + |
| 106 | +## Migrations |
| 107 | + |
| 108 | +- **`constrained()`**: all foreign keys use `constrained()` for consistent cascades and index creation |
| 109 | +- **One concern per migration**: DDL and DML never mixed in the same migration |
| 110 | +- **Defaults mirrored in model**: column defaults in migration mirrored in `$attributes` array on the model |
| 111 | +- **Reversible `down()`**: `down()` correctly undoes `up()` without data loss; forward-fix migrations for intentionally irreversible changes |
| 112 | +- **Indexes added in migration**: not as an afterthought; never added manually in production without migration |
38 | 113 |
|
39 | 114 | ## Architecture |
40 | | -- **Service layer**: complex business logic extracted to services, not bloating controllers |
41 | | -- **API Resources**: responses use API Resources/Transformers for consistent formatting |
42 | | -- **Blade/views**: no business logic in Blade templates; use view composers or components |
43 | | -- **Events & Listeners**: side effects (emails, notifications, logs) triggered via events, not inline |
44 | | -- **Dependency injection**: use constructor injection via the service container; avoid facades in business logic when testability matters |
45 | | -- **Migrations**: schema changes use migrations; migrations are reversible (`down` method) |
| 115 | + |
| 116 | +- **Service layer**: complex business logic in services; controllers delegate, not implement |
| 117 | +- **API Resources**: responses formatted via `JsonResource`; no raw `$model->toArray()` in controllers |
| 118 | +- **Events & Listeners**: side effects triggered via events; not inline in controllers or services |
| 119 | +- **Dependency injection**: constructor injection; facades avoided in business logic when testability matters |
| 120 | +- **`defer()`**: post-response work deferred; `Context` for request-scoped shared data; `Concurrency::run()` for parallel execution |
| 121 | +- **Blade/views**: no business logic in templates; use view composers or components |
| 122 | + |
| 123 | +## Common Pitfalls |
| 124 | + |
| 125 | +- `env()` called directly in application code instead of `config()` |
| 126 | +- `$request->all()` used instead of `$request->validated()` |
| 127 | +- `authorize()` in Form Request returns `true` unconditionally |
| 128 | +- Job `$timeout` exceeds `retry_after`, causing duplicate processing |
| 129 | +- Events dispatched inside DB transactions without `ShouldDispatchAfterCommit` |
| 130 | +- `Http::fake()` missing in tests → real HTTP calls in CI |
| 131 | +- Fakes set up before factories, silencing model observers |
| 132 | +- `whereHas()` used where `whereIn + pluck()` would use an index |
| 133 | +- `Cache::remember()` without tags when cache invalidation is needed |
0 commit comments