Conversation
WalkthroughMoved authentication checks from around event listener registrations into the closures for created, updated, and deleted events in HasLog.php. Listeners are always registered; log creation executes only when auth()->check() is true. Log payload structure remains the same. Changes
Sequence Diagram(s)sequenceDiagram
participant Model as Eloquent Model (uses HasLog)
participant Listener as Event Listener (closure)
participant Auth as Auth
participant Log as Log Creator
Model->>Listener: created/updated/deleted
Listener->>Auth: check()
alt Authenticated
Listener->>Log: create(action, ip, device, user_id[, old_data, changed_values])
Log-->>Listener: saved
else Not authenticated
Listener-->>Model: no-op
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dipesh79
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Traits/HasLog.php (3)
19-25: Don't define boot() in a trait; use bootHasLog() to avoid collisions and guarantee execution.A trait-level boot() can be overridden by the model’s own boot(), silently disabling your listeners. Prefer Eloquent’s trait boot hook.
- public static function boot() + protected static function bootHasLog(): void { - /** - * The parent boot method is called to ensure that the parent boot method is not overridden. - */ - parent::boot(); + // Booted via Eloquent's bootTraits; don't override the model's boot().
10-12: Remove $listeners; it’s not an Eloquent construct and is unused here.This property is for Livewire/components, not models; keeping it can confuse readers.
- protected $listeners = [ - 'log' => 'log', - ];
43-54: Capture pre-update snapshot and stop logging full originals (PII risk)src/Traits/HasLog.php — the updated handler currently calls json_encode($model->getRawOriginal()) inside static::updated; that both risks leaking sensitive attributes and may not capture the true "before" state because originals are synced during save.
- Capture a sanitized pre-save snapshot in static::updating (use getOriginal() and getDirty(), then Arr::except to remove sensitive keys) and persist that sanitized snapshot in static::updated()->afterCommit() when creating the Log.
- Use optional(request())->ip(), optional(request())->userAgent(), and auth()->id() instead of request()/auth()->user()->id to avoid null errors.
- Prefer a whitelist ($logOnly) or blacklist ($logExcept) on models and a central masker for secrets (password, remember_token, api_token, access_token, etc.) — do not log full originals.
Example pattern:
static::updating(function($m){ if (! auth()->check()) return; $sensitive = ['password','remember_token','api_token','access_token']; $m->__log_original = Arr::except($m->getOriginal(), $sensitive); $m->__log_dirty = Arr::except($m->getDirty(), $sensitive); }); static::updated(function($m){ if (! auth()->check()) return; $m->logs()->create([ 'action'=>'Update', 'ip_address'=>optional(request())->ip(), 'device'=>optional(request())->userAgent(), 'user_id'=>auth()->id(), 'old_data'=>json_encode($m->__log_original ?? []), 'changed_values'=>json_encode($m->__log_dirty ?? $m->getChanges()), ]); })->afterCommit();
🧹 Nitpick comments (3)
src/Traits/HasLog.php (3)
29-38: Auth check move is good; tighten helpers and write after commit.Use auth()->id(), early return, null-safe request(), and run after DB commit to avoid logging rolled-back ops.
- static::created(function ($model) { - if (auth()->check()) { - $model->logs()->create([ - 'action' => 'Create', - 'ip_address' => request()->ip(), - 'device' => request()->userAgent(), - 'user_id' => auth()->user()->id - ]); - } - }); + static::created(function ($model) { + if (! auth()->check()) return; + $model->logs()->create([ + 'action' => 'Create', + 'ip_address' => optional(request())->ip(), + 'device' => optional(request())->userAgent(), + 'user_id' => auth()->id(), + ]); + })->afterCommit();
59-68: Apply the same helper/after-commit improvements here; consider forceDeleted as well.Use auth()->id(), optional(request()), early return, and ->afterCommit(). If you use soft deletes, also log forceDeleted.
- static::deleted(function ($model) { - if (auth()->check()) { - $model->logs()->create([ - 'action' => 'Delete', - 'ip_address' => request()->ip(), - 'device' => request()->userAgent(), - 'user_id' => auth()->user()->id - ]); - } - }); + static::deleted(function ($model) { + if (! auth()->check()) return; + $model->logs()->create([ + 'action' => 'Delete', + 'ip_address' => optional(request())->ip(), + 'device' => optional(request())->userAgent(), + 'user_id' => auth()->id(), + ]); + })->afterCommit();
29-68: DRY up repeated payload construction.Extract a small helper to build and persist the log entry; closures call it with just the action.
Example (outside the shown ranges):
protected function writeUserLog(string $action, array $extra = []): void { if (! auth()->check()) return; $base = [ 'action' => $action, 'ip_address' => optional(request())->ip(), 'device' => optional(request())->userAgent(), 'user_id' => auth()->id(), ]; $this->logs()->create($base + $extra); }Then:
- created: $model->writeUserLog('Create');
- updated: $model->writeUserLog('Update', ['old_data' => json_encode(...), 'changed_values' => json_encode(...)]); etc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Traits/HasLog.php(2 hunks)
🔇 Additional comments (1)
src/Traits/HasLog.php (1)
43-54: Gate ->afterCommit usage — verify framework supportcomposer.json has no laravel/illuminate requirement and there's no composer.lock, so the repo's Laravel version is unknown. Do not add ->afterCommit unconditionally in src/Traits/HasLog.php (the updated listener). Either require & document a minimum Laravel version that supports ->afterCommit, or add a runtime guard/fallback (detect support or use DB::afterCommit/polyfill) before using ->afterCommit.
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor