feat(plugins): List plugins with errors

[nfebe]

Before now, plugins with errors would not appear on the plugin:list, now such plugins would appear with status error and user can use --debug flag to get more information.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (6)

app/Services/PluginManager.php (5)

465-474: Consider handling undefined provider more explicitly.

While the current implementation works (class_exists on empty string returns false), consider being more explicit:

-        if (isset($plugin['provider']) && ! class_exists($plugin['provider'])) {
+        if (isset($plugin['provider']) && (!empty($plugin['provider']) && !class_exists($plugin['provider']))) {

---

131-133: Consider using Log::error for missing manifest files.

A missing manifest file is a critical issue that prevents the plugin from loading. This warrants an error-level log rather than a warning.

-            $error = "Plugin manifest not found: {$manifestPath}";
-            Log::warning($error);
+            $error = "Plugin manifest not found: {$manifestPath}";
+            Log::error($error);

---

229-240: Consider the implications of catching all Throwable exceptions.

While catching Throwable prevents a single bad plugin from breaking plugin discovery, it might mask critical system errors that should bubble up (e.g., out of memory errors, assertion failures). Consider catching only \Exception instead, or at least ensure critical errors are properly logged.

---

470-474: Consider if the provider class check is necessary.

The provider class existence is already checked in loadPlugin() and would set an error. This check might be redundant, though it does provide defense-in-depth.

If you want to keep this check, consider adding a comment explaining why both checks are needed:

        // If the provider class doesn't exist, it's not enabled
+       // This provides an additional safety check even if loadPlugin didn't catch it
        if (isset($plugin['provider']) && ! class_exists($plugin['provider'])) {
            return false;
        }

---

495-507: Redundant plugin validation checks.

The checks at lines 495-505 are redundant because isPluginEnabled() at line 507 already performs these same checks (error state and provider existence). Consider removing the redundant checks to simplify the code.

-            if (isset($plugin['error'])) {
-                Log::warning("Skipping plugin with errors: {$plugin['id']} - {$plugin['error']}");
-
-                return;
-            }
-
-            if (! isset($plugin['provider']) || ! class_exists($plugin['provider'])) {
-                Log::warning("Skipping plugin with missing or invalid provider: {$plugin['id']}");
-
-                return;
-            }
-
             if ($this->isPluginEnabled($plugin['id'])) {
                 try {
                     $this->app->register($plugin['provider']);
                     Log::debug("Registered plugin service provider: {$plugin['provider']}");
                 } catch (\Exception $e) {
                     Log::error('Failed to register plugin service provider', [
                         'plugin' => $plugin['id'],
                         'provider' => $plugin['provider'],
                         'error' => $e->getMessage(),
                     ]);
                     // Don't throw the exception to allow other plugins to load
                 }
             } else {
-                Log::debug("Skipping disabled plugin: {$plugin['id']}");
+                $reason = isset($plugin['error']) ? "error: {$plugin['error']}" : "disabled";
+                Log::debug("Skipping plugin {$plugin['id']}: {$reason}");
             }

app/Console/Commands/Plugin/ListCommand.php (1)

52-59: Consider removing redundant provider class check.

The PluginManager already checks for missing provider classes and sets the error accordingly. This additional check in debug mode might be redundant.

You could simplify by relying solely on the error set by PluginManager:

-            } elseif ($debug) {
-                // Additional debug checks
-                if (! class_exists($plugin['provider'] ?? '')) {
-                    $error = "Provider class not found: {$plugin['provider']}";
-                    $status = '<fg=yellow>Error</>';
-                    $errorMessages[] = $error;
-                }
             }

However, if you want to keep this as an extra validation layer, consider adding it to the hasErrors flag:

                 if (! class_exists($plugin['provider'] ?? '')) {
+                    $hasErrors = true;
                     $error = "Provider class not found: {$plugin['provider']}";

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2606f40 and 8b4fecc.

📒 Files selected for processing (2)

• app/Console/Commands/Plugin/ListCommand.php (1 hunks)
• app/Services/PluginManager.php (4 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: Run Tests

🔇 Additional comments (8)

app/Services/PluginManager.php (6)

128-141: Good error handling improvement!

The change from returning null to a structured array with error information improves error visibility and debugging capabilities.

---

148-158: Consistent error handling for invalid manifests.

The structured error response maintains consistency across different error scenarios.

---

189-207: Proper namespace formatting and provider validation.

Good defensive programming to ensure namespace ends with a single backslash and validate provider class existence.

---

218-240: Comprehensive exception handling added.

Good addition of specific and generic exception handlers. The structured error responses maintain consistency.

---

495-518: Excellent defensive programming in plugin registration.

The changes ensure that problematic plugins don't prevent other plugins from loading, while providing clear logging for debugging.

---

189-189: Good namespace normalization!

Ensuring the namespace always ends with exactly one backslash prevents potential autoloading issues.

app/Console/Commands/Plugin/ListCommand.php (2)

7-20: Well-documented command signature.

The --debug option is clearly documented and follows Laravel command conventions.

---

78-90: Excellent error reporting implementation.

The error handling provides clear feedback with appropriate verbosity levels and follows Unix conventions with exit codes.