Fix AdminNotificationService crashing on string throttle_minutes config
env('ERROR_NOTIFICATION_THROTTLE_MINUTES', 15) returns the literal string
"15" from production's .env (env() doesn't cast numeric strings), which
Carbon 3.11's addMinutes() rejects with a TypeError. That crash happened
before the throttle-dedup cache write, so it silently masked the *original*
exception on every notify-worthy error, indefinitely (discovered while
investigating a missing workshop-registration email — the real SMTP error
was hidden behind this crash). Cast to (int).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -23,7 +23,7 @@ class AdminNotificationService
|
|||||||
}
|
}
|
||||||
|
|
||||||
$errorKey = $this->getErrorKey($exception);
|
$errorKey = $this->getErrorKey($exception);
|
||||||
$throttleMinutes = config('libra.notifications.throttle_minutes', 15);
|
$throttleMinutes = (int) config('libra.notifications.throttle_minutes', 15);
|
||||||
|
|
||||||
if (Cache::has($errorKey)) {
|
if (Cache::has($errorKey)) {
|
||||||
return;
|
return;
|
||||||
|
|||||||
@@ -0,0 +1,93 @@
|
|||||||
|
---
|
||||||
|
id: 23
|
||||||
|
title: 23 Fix AdminNotificationService Carbon TypeError Masking Exceptions
|
||||||
|
status: Done
|
||||||
|
date: 2026-09-28
|
||||||
|
parent: null
|
||||||
|
---
|
||||||
|
|
||||||
|
## Description
|
||||||
|
|
||||||
|
Investigating a client report of a missing workshop-registration email led to discovering a real
|
||||||
|
bug: `App\Services\AdminNotificationService::notifyError()` throws its own `TypeError` whenever
|
||||||
|
it runs, which **masks the original exception** it was trying to report and prevents the admin
|
||||||
|
notification from ever being sent. Root cause: `config('libra.notifications.throttle_minutes', 15)`
|
||||||
|
resolves to `env('ERROR_NOTIFICATION_THROTTLE_MINUTES', 15)` (see `config/libra.php` line 14).
|
||||||
|
Production's `.env` has `ERROR_NOTIFICATION_THROTTLE_MINUTES=15` set — Laravel's `env()` helper
|
||||||
|
does not cast a numeric-looking string to an int, so this returns the **string** `"15"`, not the
|
||||||
|
int `15`. That string is then passed to `now()->addMinutes($throttleMinutes)` in
|
||||||
|
`AdminNotificationService::notifyError()` (line 32), and Carbon 3.11 (this project's installed
|
||||||
|
version, per `composer.lock`) requires `addMinutes()`'s argument to be `int|float` — passing a
|
||||||
|
string throws `TypeError: Carbon\Carbon::rawAddUnit(): Argument #3 ($value) must be of type
|
||||||
|
int|float, string given`.
|
||||||
|
|
||||||
|
This is a live production bug, confirmed via `storage/logs/laravel.log` on h1
|
||||||
|
(`/home/libra/public_html/storage/logs/laravel.log`, entries timestamped `2026-09-27 21:35:26`
|
||||||
|
and `21:40:31`) — every exception that reaches `Handler->report()` and is judged
|
||||||
|
notify-worthy by `AdminNotificationService::shouldNotifyAdmin()` (e.g. any
|
||||||
|
`Symfony\Component\Mailer\Exception\TransportExceptionInterface`, or a critical
|
||||||
|
`QueryException`) currently crashes this method instead of sending the notification, and the
|
||||||
|
crash happens **before** `Cache::put()` on line 32 ever executes, so the throttle-dedup cache key
|
||||||
|
is never written — meaning this crash also can't self-heal or throttle, it fires on every single
|
||||||
|
qualifying exception, indefinitely, until fixed.
|
||||||
|
|
||||||
|
**File to modify:** `app/Services/AdminNotificationService.php`
|
||||||
|
|
||||||
|
Current (lines 25-32):
|
||||||
|
```php
|
||||||
|
$errorKey = $this->getErrorKey($exception);
|
||||||
|
$throttleMinutes = config('libra.notifications.throttle_minutes', 15);
|
||||||
|
|
||||||
|
if (Cache::has($errorKey)) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
Cache::put($errorKey, true, now()->addMinutes($throttleMinutes));
|
||||||
|
```
|
||||||
|
|
||||||
|
Change to cast the config value to an int before use:
|
||||||
|
```php
|
||||||
|
$errorKey = $this->getErrorKey($exception);
|
||||||
|
$throttleMinutes = (int) config('libra.notifications.throttle_minutes', 15);
|
||||||
|
|
||||||
|
if (Cache::has($errorKey)) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
Cache::put($errorKey, true, now()->addMinutes($throttleMinutes));
|
||||||
|
```
|
||||||
|
|
||||||
|
This is the only line that changes. No other method, no other file.
|
||||||
|
|
||||||
|
## Acceptance Criteria
|
||||||
|
|
||||||
|
- [x] `app/Services/AdminNotificationService.php`'s `$throttleMinutes` assignment casts the config
|
||||||
|
value with `(int)`
|
||||||
|
- [x] A test constructs `AdminNotificationService` and calls `notifyError()` with a throwable that
|
||||||
|
makes `shouldNotifyAdmin()` return true (e.g. a mock/stub implementing
|
||||||
|
`Symfony\Component\Mailer\Exception\TransportExceptionInterface`, or simply an instance of a
|
||||||
|
class implementing `App\Contracts\ShouldNotifyAdmin`), with
|
||||||
|
`config(['libra.notifications.throttle_minutes' => '15'])` explicitly set as a **string** in
|
||||||
|
the test (reproducing production's actual condition — `env()` never returns a string this
|
||||||
|
way in tests, so it must be set explicitly to catch this class of bug) — the call completes
|
||||||
|
without throwing, and a mail notification is sent (assert via `Notification::fake()` /
|
||||||
|
`Mail::fake()` as appropriate for however `SystemErrorNotification` delivers)
|
||||||
|
- [x] The same test, run against the **old** code (temporarily, to confirm the test actually
|
||||||
|
catches the bug) throws a `TypeError` — state in the completion report that this was
|
||||||
|
confirmed, per this task's RED step
|
||||||
|
|
||||||
|
## QA Verification (2026-09-28)
|
||||||
|
|
||||||
|
All 3 acceptance criteria verified directly:
|
||||||
|
1. Confirmed `(int)` cast present at `app/Services/AdminNotificationService.php:26`.
|
||||||
|
2. Ran `tests/Unit/Services/AdminNotificationServiceTest.php`'s new test
|
||||||
|
(`does not throw and still sends notification when throttle_minutes config is a string`) —
|
||||||
|
passed.
|
||||||
|
3. Temporarily removed the `(int)` cast, re-ran the same test — reproduced the exact TypeError
|
||||||
|
(`Carbon\Carbon::rawAddUnit(): Argument #3 ($value) must be of type int|float, string given`),
|
||||||
|
confirming RED. Restored the `(int)` cast. Full suite re-run: 17/17 passed. `git diff` confirms
|
||||||
|
only the single intended line differs from the pre-fix version.
|
||||||
|
|
||||||
|
## Sub-tasks
|
||||||
|
|
||||||
|
None.
|
||||||
@@ -134,3 +134,4 @@
|
|||||||
| 20 | 20 Nablus Workshop Form - Match Site Background and Card Styling | Done |
|
| 20 | 20 Nablus Workshop Form - Match Site Background and Card Styling | Done |
|
||||||
| 21 | 21 Nablus Workshop Form - Flyer Size Adjustment | Done |
|
| 21 | 21 Nablus Workshop Form - Flyer Size Adjustment | Done |
|
||||||
| 22 | 22 Fix Footer Address - Add Missing Street Name | Done |
|
| 22 | 22 Fix Footer Address - Add Missing Street Name | Done |
|
||||||
|
| 23 | 23 Fix AdminNotificationService Carbon TypeError Masking Exceptions | Done |
|
||||||
|
|||||||
@@ -184,4 +184,27 @@ describe('notifyError', function () {
|
|||||||
|
|
||||||
Notification::assertSentOnDemandTimes(SystemErrorNotification::class, 2);
|
Notification::assertSentOnDemandTimes(SystemErrorNotification::class, 2);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('does not throw and still sends notification when throttle_minutes config is a string', function () {
|
||||||
|
// Reproduces a live production bug: env('ERROR_NOTIFICATION_THROTTLE_MINUTES', 15) returns
|
||||||
|
// the string "15" (env() never casts numeric-looking strings to int), which used to be
|
||||||
|
// passed straight into now()->addMinutes(), and Carbon 3.11 throws a TypeError for a
|
||||||
|
// string argument there. That TypeError masked the original exception being reported and
|
||||||
|
// notifyError() never actually sent the admin notification.
|
||||||
|
Notification::fake();
|
||||||
|
Cache::flush();
|
||||||
|
config(['libra.notifications.throttle_minutes' => '15']);
|
||||||
|
|
||||||
|
$service = new AdminNotificationService;
|
||||||
|
$exception = new TransportException('SMTP error');
|
||||||
|
|
||||||
|
$service->notifyError($exception);
|
||||||
|
|
||||||
|
Notification::assertSentOnDemand(
|
||||||
|
SystemErrorNotification::class,
|
||||||
|
function ($notification, $channels, $notifiable) {
|
||||||
|
return $notifiable->routes['mail'] === 'admin@test.com';
|
||||||
|
}
|
||||||
|
);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user