N+1 queries, missing indexes, unvalidated inputs, hardcoded values, untested edge cases, and the 11 other things that slip through code review when you don’t have a checklist. Steal this. Your future self will thank you.
Most code reviews catch the obvious things. The typo in the variable name. The missing return type. The method that’s doing three jobs at once. What they don’t catch reliably — and what shows up in production three weeks after merge — is the structural class of problem: the query that runs 50 times instead of 1, the input that reaches the database unvalidated, the authorization check that got skipped in the middle of a refactor, the job that fires correctly in the test suite and fails silently in production because the queue connection is different.
Our team has a checklist. It’s not long — 14 points. But it’s the 14 things that have actually caused incidents, regressions, or support tickets after being missed in review. Every item is here because something slipped through without it. Steal the whole thing or adapt it; either way, start running it on every PR before you click Merge.
1. N+1 Queries
The most expensive thing you can ship in a Laravel application and the easiest to miss in code review, because it looks like clean Eloquent code.
// Runs one query per order — 100 orders = 101 queries
$orders = Order::all();
foreach ($orders as $order) {
echo $order->customer->name;
}
// 2 queries regardless of result count
$orders = Order::with('customer')->get();
What to look for: any loop that accesses a relationship on a model. Any ->get() or ->all() not followed by an eager load when the view or logic accesses related data.
The tool that catches it automatically:
// AppServiceProvider::boot() — throws in local when N+1 is detected
if (app()->isLocal()) {
Model::preventLazyLoading();
}
preventLazyLoading() throws an exception when a relationship is accessed without being eager-loaded. Put this in local and staging. Never miss an N+1 in development again.
Review question: Does every collection loop have the right with() call? Is preventLazyLoading() enabled in staging?
2. Missing Database Indexes
The query works. It runs in 2ms on 50 rows in development. It runs in 4 seconds in production with 2 million rows because the column has no index.
// This query
User::where('email', $email)->first();
// Needs this index
$table->unique('email');
Compound queries need compound indexes in the right column order:
User::where('tenant_id', $tenantId)
->where('status', 'active')
->orderBy('created_at', 'desc')
->get();
// Compound index — column order matters
$table->index(['tenant_id', 'status', 'created_at']);
tenant_id first because it eliminates the most rows, then status, then created_at for the sort.
Review question: Does every new where() on a non-primary-key column have a corresponding migration that adds an index?
3. Unvalidated or Insufficiently Validated Input
Missing validation entirely:
// No validation — $request->all() goes directly into the model
public function store(Request $request): RedirectResponse
{
Product::create($request->all());
return redirect()->route('products.index');
}
// Every input validated before use
public function store(StoreProductRequest $request): RedirectResponse
{
Product::create($request->validated());
return redirect()->route('products.index');
}
Insufficient validation — type not enforced:
// 'price' is required but a string "free" or "-1" would pass
'price' => ['required'],
// Type, range, and format enforced
'price' => ['required', 'integer', 'min:0'],
'email' => ['required', 'email:rfc,dns', 'max:255'],
'status' => ['required', Rule::in(['draft', 'published', 'archived'])],
'starts_at' => ['required', 'date', 'after_or_equal:today'],
'ends_at' => ['required', 'date', 'after:starts_at'],
Review question: Does every controller method that accepts input use a Form Request or explicit $request->validate()? Does validation enforce types and ranges, not just presence?
4. Missing Authorization Checks
Authorization that’s missing is invisible in the happy path. It only shows up when someone calls an endpoint they shouldn’t be able to call.
// No authorization — any authenticated user can delete any order
public function destroy(Order $order): RedirectResponse
{
$order->delete();
return redirect()->route('orders.index');
}
// Policy check before action
public function destroy(Order $order): RedirectResponse
{
$this->authorize('delete', $order);
$order->delete();
return redirect()->route('orders.index');
}
The multi-tenant version of this bug:
// Authenticated, but not scoped to the current tenant
$order = Order::findOrFail($id);
// Any tenant can access any order if they know the ID
// Scoped to the current tenant
$order = Order::where('tenant_id', auth()->user()->tenant_id)
->findOrFail($id);
Review question: Does every controller method that accesses or modifies a specific resource call $this->authorize() or a Policy check? In multi-tenant apps, is every query scoped to the current tenant?
5. Hardcoded Values That Should Be Config or Constants
// Hardcoded — different value needed in staging vs production
if ($order->total > 10000) {
FlagForReview::dispatch($order);
}
// Config-driven
if ($order->total > config('orders.review_threshold')) {
FlagForReview::dispatch($order);
}
// Magic strings
if ($user->status === 'active') { }
// Enum — self-documenting, refactorable
if ($user->status === UserStatus::ACTIVE) { }
Values that belong in config:
API keys and secrets → env('SERVICE_API_KEY')
URLs to external services → env('STRIPE_WEBHOOK_URL')
Feature flag thresholds → config('features.threshold')
Retry counts and limits → config('queues.max_attempts')
Email addresses for alerts → config('alerts.email')
Review question: Are there magic strings or numbers that represent business rules or environment-specific values? Should they be in a config file or an enum?
6. Missing or Shallow Test Coverage for Changed Code
The checklist isn’t about coverage percentage — it’s about whether the specific behaviour changed in this PR has a test that would fail if it broke.
// The code added in this PR
public function applyDiscount(Order $order, string $code): void
{
$discount = DiscountCode::where('code', $code)
->where('expires_at', '>', now())
->firstOrFail();
$order->update(['total' => $order->total - $discount->amount]);
DiscountRedemption::create([
'order_id' => $order->id,
'discount_code_id' => $discount->id,
]);
}
// Tests this PR should include
it('applies a valid discount code to the order total');
it('throws an exception for an expired discount code');
it('throws an exception for a non-existent code');
it('creates a redemption record when the discount is applied');
it('does not create a redemption record if the order update fails'); // atomicity
That last test — atomicity — is the one PRs almost never include. If there’s a sequence of database writes, there should be a test that simulates one failing and asserts the others are rolled back.
Review question: Is there a test for the happy path? For each negative path? Are there database state assertions, not just response status assertions?
7. Sensitive Data in Logs
// Logs the full request — includes password, card number, SSN
Log::info('User registration', $request->all());
// Log only what's needed for debugging
Log::info('User registration attempt', [
'email' => $request->email,
'ip' => $request->ip(),
'user_agent' => $request->userAgent(),
]);
// For exceptions from external services — message and code, not the full object
Log::error('Stripe charge failed', [
'order_id' => $order->id,
'error_code' => $e->getStripeCode(),
'message' => $e->getMessage(),
]);
Review question: Does any new logging call include $request->all(), API tokens, or full exception objects from external services? Are sensitive model attributes listed in $hidden?
8. Jobs Without Proper Failure Handling
A job that fails silently is worse than a job that doesn’t run — you think the work was done when it wasn’t.
// No failure configuration — fails once and is gone
class SendInvoiceEmail implements ShouldQueue
{
public function handle(): void
{
Mail::to($this->order->customer->email)
->send(new InvoiceMailable($this->order));
}
}
// Retry configuration, failure handling, and timeout
class SendInvoiceEmail implements ShouldQueue
{
public int $tries = 3;
public int $timeout = 60;
public int $backoff = 30;
public function handle(): void
{
Mail::to($this->order->customer->email)
->send(new InvoiceMailable($this->order));
}
public function failed(\Throwable $exception): void
{
Log::error('Invoice email failed after all retries', [
'order_id' => $this->order->id,
'error' => $exception->getMessage(),
]);
$this->order->update(['email_status' => 'failed']);
Notification::route('mail', config('alerts.ops_email'))
->notify(new InvoiceEmailFailedNotification($this->order));
}
}
Review question: Does every new job have $tries, $timeout, and a failed() method? Does failed() log enough context to investigate and take an appropriate action?
9. Race Conditions in Multi-Step Operations
Any operation that reads a value, decides based on it, then writes back is vulnerable when two requests run concurrently.
// Race condition — two concurrent requests both read seats = 1
// Both see 1 > 0, both decrement, result: seats = -1
public function bookSeat(Event $event, User $user): void
{
if ($event->available_seats > 0) {
$event->decrement('available_seats');
Booking::create(['event_id' => $event->id, 'user_id' => $user->id]);
}
}
// Pessimistic locking — only one request holds the lock at a time
public function bookSeat(Event $event, User $user): void
{
DB::transaction(function () use ($event, $user) {
$locked = Event::lockForUpdate()->findOrFail($event->id);
if ($locked->available_seats <= 0) {
throw new NoSeatsAvailableException();
}
$locked->decrement('available_seats');
Booking::create(['event_id' => $locked->id, 'user_id' => $user->id]);
});
}
Patterns that signal race condition risk:
Read a count, check it, update it → inventory, seats, quota
Check for uniqueness, then create → prevent duplicate records
Read a status, act on it, update it → job claiming, state machines
Generate a sequential ID outside a transaction
Review question: Does this PR touch any counter, quota, or status field that concurrent requests could read and write? Is there a transaction and lockForUpdate() protecting the critical section?
10. External API Calls Without Rate Limit Handling
Every external API call without 429 handling is a ticking clock.
// No rate limit handling — unhandled exception on 429
$response = Http::withToken(config('services.stripe.key'))
->post('https://api.stripe.com/v1/charges', $payload);
// Handle rate limits explicitly
$response = Http::withToken(config('services.stripe.key'))
->retry(3, 100, function (\Exception $e) {
return $e instanceof RequestException
&& in_array($e->response?->status(), [429, 503]);
})
->post('https://api.stripe.com/v1/charges', $payload);
if ($response->status() === 429) {
$retryAfter = $response->header('Retry-After', 60);
RateLimitedJob::dispatch($payload)->delay($retryAfter);
return;
}
$response->throw();
Review question: Does every external HTTP call handle 429 responses? Is there a retry policy that respects the Retry-After header?
11. Missing Transactions Around Multi-Step Database Operations
Any operation that performs two or more database writes should be in a transaction. If one write succeeds and the next fails, the database is in an inconsistent state.
// No transaction — if creating OrderLines fails, the Order exists without lines
public function createOrder(array $data): Order
{
$order = Order::create($data['order']);
foreach ($data['lines'] as $line) {
OrderLine::create([...$line, 'order_id' => $order->id]);
}
return $order;
}
// Transaction — either everything succeeds or nothing does
public function createOrder(array $data): Order
{
return DB::transaction(function () use ($data) {
$order = Order::create($data['order']);
foreach ($data['lines'] as $line) {
OrderLine::create([...$line, 'order_id' => $order->id]);
}
return $order;
});
}
Review question: Does this PR contain any multi-step database operation? Is it wrapped in DB::transaction()? Is rollback behaviour tested?
12. Eloquent Mass Assignment Without $fillable
A model without $fillable accepts any attribute from mass assignment. If populated from user input, the user controls which database fields get written.
// $guarded = [] means everything is mass-assignable
class User extends Model
{
protected $guarded = [];
}
// POST {"name": "Sadique", "is_admin": true}
User::create($request->validated()); // is_admin is written
// Explicit allowlist
class User extends Model
{
protected $fillable = ['name', 'email', 'password'];
// is_admin not in $fillable — silently ignored
}
Review question: Does every new model have $fillable defined? Does every ::create() and ->update() use $request->validated() or an explicit array?
13. Caching Without a Clear Invalidation Strategy
Cached data that becomes stale silently is one of the hardest bugs to trace. Users see old data. The database has correct data. The fix is php artisan cache:clear, which tells you something went wrong but not what or when.
// Cache without invalidation — when does this get cleared?
$settings = Cache::remember('tenant:settings:' . $tenantId, 86400, function () use ($tenantId) {
return TenantSettings::where('tenant_id', $tenantId)->get();
});
// Cache with explicit invalidation when data changes
class TenantSettings extends Model
{
protected static function booted(): void
{
static::saved(function (TenantSettings $settings) {
Cache::forget('tenant:settings:' . $settings->tenant_id);
});
static::deleted(function (TenantSettings $settings) {
Cache::forget('tenant:settings:' . $settings->tenant_id);
});
}
}
Review question: Is any new cached data invalidated when the underlying data changes? Does the cache key include enough context (tenant ID, user ID) to prevent bleeding between users?
14. Deployment-Unsafe Code
Some code works in isolation and fails after deployment because of environment assumptions.
// env() returns null after config:cache is run
$key = env('STRIPE_SECRET');
// Always access via config()
$key = config('services.stripe.secret');
// Migration that drops a column while in-flight jobs still reference it
// The safe approach:
// Stage 1: Add new column, keep old, deploy code that writes to both
// Stage 2: Migrate data from old to new column
// Stage 3: Drop old column in a later deployment
# Artisan command that modifies data with no preview mode
php artisan recalculate:totals
# With --dry-run support
php artisan recalculate:totals --dry-run
Review question: Does any code call env() directly? Does any migration drop or rename a column that in-flight jobs reference? Do new data-modifying commands have a --dry-run flag?
The Checklist in One Place
Before every merge, verify:
1. N+1 queries — every collection loop has the right with() call
2. Missing indexes — every new where() clause has an index if needed
3. Input validation — Form Request used, types and ranges enforced
4. Authorization — authorize() or Policy on every resource action
5. Hardcoded values — business rules in config or enums, not inline
6. Test coverage — happy path, negative paths, DB state assertions
7. Log safety — no passwords, tokens, or full request dumps
8. Job failure — $tries, $timeout, and failed() method defined
9. Race conditions — lockForUpdate() and transactions on contested writes
10. API rate limits — retry logic and 429 handling on external calls
11. Transactions — DB::transaction() around multi-step writes
12. Mass assignment — $fillable defined, $request->validated() used
13. Cache strategy — stale data cleared on model save/delete
14. Deploy safety — no env() calls, no column drops without staging
Not every item applies to every PR. A PR that adds a Blade view doesn’t need a race condition review. A PR that adds a job definitely does. The checklist is a prompt, not a penalty — it catches the things that are easy to forget because they’re not in the code you’re looking at, they’re in the code you’d need to imagine.
The incidents that came from not having a checklist were cheaper than the checklist feels. The incidents prevented by having one were invisible — which is exactly the point.