Return empty array instead of null when plugin update checking fails#2364
Return empty array instead of null when plugin update checking fails#2364Boy132 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR changes ChangesPlugin update data exception handling
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Models/Plugin.php (1)
262-265: ⚡ Quick winConsider making the "no data" return values consistent.
Line 265 returns
nullwhenupdate_urlis not set, while line 282 now returns[]when an exception occurs. Both represent "no update data available" but use different values. For better type consistency and predictability, consider either:
- Returning
[]in both cases and changing the return type annotation toarray, or- Explicitly documenting why these cases use different return values
The downstream methods handle both correctly since they check truthiness, but consistent return values improve maintainability and type safety.
♻️ Proposed refactor for consistency
- /** `@return` null|array<string, array{version: string, download_url: string}> */ - private function getUpdateData(): ?array + /** `@return` array<string, array{version: string, download_url: string}> */ + private function getUpdateData(): array { if (!$this->update_url) { - return null; + return []; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/Plugin.php` around lines 262 - 265, getUpdateData currently returns null when $this->update_url is falsy but returns [] on exceptions, causing inconsistent "no data" return values; change getUpdateData to always return an array (use [] instead of null) and update the method signature/return type annotation from ?array to array, ensuring callers still handle empty arrays as "no update data" (refer to getUpdateData, $this->update_url and the exception return), and update any docblocks or tests expecting null accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/Models/Plugin.php`:
- Around line 262-265: getUpdateData currently returns null when
$this->update_url is falsy but returns [] on exceptions, causing inconsistent
"no data" return values; change getUpdateData to always return an array (use []
instead of null) and update the method signature/return type annotation from
?array to array, ensuring callers still handle empty arrays as "no update data"
(refer to getUpdateData, $this->update_url and the exception return), and update
any docblocks or tests expecting null accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 247695bb-7d2b-4b69-acb5-38101441845b
📒 Files selected for processing (1)
app/Models/Plugin.php
With
nullthe update check is repeated each request which can slow down the panel.