Fix boolean validator on startup value#1830
Conversation
📝 WalkthroughWalkthroughThe Changes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Server/Pages/Startup.php (1)
188-192: Consider improving notification message for empty original values.When
$originalis empty or null, the notification body displays " -> 1" which could be clearer. Consider formatting the message to handle this case more gracefully.Apply this diff to improve the notification message:
Notification::make() ->title(trans('server/startup.update', ['variable' => $serverVariable->variable->name])) - ->body(fn () => $original . ' -> ' . $stateForValidation) + ->body(fn () => ($original ?: trans('server/startup.empty')) . ' -> ' . $stateForValidation) ->success() ->send();Note: This assumes a translation key exists or you can use a literal like
'(empty)'instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Filament/Server/Pages/Startup.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Server/Pages/Startup.php (4)
app/Http/Controllers/Api/Client/Servers/StartupController.php (1)
update(63-108)app/Models/ServerVariable.php (2)
ServerVariable(21-65)variable(61-64)app/Facades/Activity.php (1)
Activity(8-14)app/Services/Activity/ActivityLogService.php (3)
event(48-53)property(109-117)log(136-155)
🔇 Additional comments (2)
app/Filament/Server/Pages/Startup.php (2)
150-150: Signature change correctly addresses the boolean handling issue.The updated signature now accepts boolean values directly, which aligns with the PR objective to fix boolean startup value updates.
155-155: Inconsistency found: API controller does not normalize booleans like Filament page does.The Filament page (line 155) correctly normalizes boolean values to
'1'and'0'strings before validation and storage. However, the API controller (app/Http/Controllers/Api/Client/Servers/StartupController.phplines 63-90) passes values directly without normalization:$request->validate(['value' => $variable->rules])then stores$request->input('value') ?? ''.This creates inconsistent behavior:
- Filament:
true→'1'(validated and stored as string)- API:
true→ stored as-is (could be boolean or depend on JSON encoding)The Filament normalization logic itself is correct and consistently applied throughout. However, verify whether:
- The API should also normalize booleans for consistency
- The egg variable validation rules can handle raw boolean inputs from the API
- This difference is intentional
|
I have read the CLA Document and I hereby sign the CLA |
|
Thanks but forcing a boolean to always be |
fixing #1829