From 8ec79097bbdbcbce622518ef61c068f20e0ef74e Mon Sep 17 00:00:00 2001
From: Yassine Doghri <yassine@doghri.fr>
Date: Thu, 16 May 2024 15:53:42 +0000
Subject: [PATCH] feat(plugins): display errors when plugin is invalid instead
 of crashing

---
 modules/Plugins/Core/BasePlugin.php         | 53 ++++++++++++---------
 modules/Plugins/Core/PluginStatus.php       | 12 +++++
 modules/Plugins/Core/Plugins.php            | 10 ++--
 modules/Plugins/Language/en/Plugins.php     |  6 +++
 modules/Plugins/Manifest/Manifest.php       |  4 +-
 modules/Plugins/Manifest/ManifestObject.php | 28 +++++++----
 modules/Plugins/Manifest/Person.php         |  4 +-
 themes/cp_admin/plugins/_plugin.php         | 20 ++++++--
 themes/cp_admin/plugins/view.php            | 30 ++++++++++--
 9 files changed, 119 insertions(+), 48 deletions(-)
 create mode 100644 modules/Plugins/Core/PluginStatus.php

diff --git a/modules/Plugins/Core/BasePlugin.php b/modules/Plugins/Core/BasePlugin.php
index 7eafb3ed..70877548 100644
--- a/modules/Plugins/Core/BasePlugin.php
+++ b/modules/Plugins/Core/BasePlugin.php
@@ -21,7 +21,6 @@ use Modules\Plugins\Manifest\Manifest;
 use Modules\Plugins\Manifest\Person;
 use Modules\Plugins\Manifest\Repository;
 use Modules\Plugins\Manifest\Settings;
-use RuntimeException;
 
 /**
  * @property string $key
@@ -33,7 +32,12 @@ abstract class BasePlugin implements PluginInterface
 
     protected string $iconSrc;
 
-    protected bool $active;
+    /**
+     * @var array<string,string>
+     */
+    protected array $errors = [];
+
+    protected PluginStatus $status;
 
     protected Manifest $manifest;
 
@@ -51,20 +55,30 @@ abstract class BasePlugin implements PluginInterface
         $manifestContents = file_get_contents($manifestPath);
 
         if (! $manifestContents) {
-            throw new RuntimeException(sprintf('Plugin manifest "%s" is missing!', $manifestPath));
+            $manifestContents = '{}';
+            $this->errors['manifest'] = lang('Plugins.errors.manifestMissing', [
+                'manifestPath' => $manifestPath,
+            ]);
         }
 
         /** @var array<mixed>|null $manifestData */
         $manifestData = json_decode($manifestContents, true);
 
         if ($manifestData === null) {
-            throw new RuntimeException(sprintf('Plugin manifest "%s" is not a valid JSON', $manifestPath), 1);
+            $manifestData = [];
+            $this->errors['manifest'] = lang('Plugins.errors.manifestJsonInvalid', [
+                'manifestPath' => $manifestPath,
+            ]);
         }
 
-        $this->manifest = new Manifest($manifestData);
+        $this->manifest = new Manifest($this->key, $manifestData);
+        $this->errors = [...$this->errors, ...Manifest::getPluginErrors($this->key)];
 
-        // check that plugin is active
-        $this->active = get_plugin_option($this->key, 'active') ?? false;
+        if ($this->errors !== []) {
+            $this->status = PluginStatus::INVALID;
+        } else {
+            $this->status = get_plugin_option($this->key, 'active') ? PluginStatus::ACTIVE : PluginStatus::INACTIVE;
+        }
 
         $this->iconSrc = $this->loadIcon($directory . '/icon.svg');
 
@@ -98,9 +112,17 @@ abstract class BasePlugin implements PluginInterface
     {
     }
 
-    final public function isActive(): bool
+    final public function getStatus(): PluginStatus
     {
-        return $this->active;
+        return $this->status;
+    }
+
+    /**
+     * @return array<string,string>
+     */
+    final public function getErrors(): array
+    {
+        return $this->errors;
     }
 
     final public function isHookDeclared(string $name): bool
@@ -144,19 +166,6 @@ abstract class BasePlugin implements PluginInterface
         return $this->iconSrc;
     }
 
-    final public function doesManifestHaveErrors(): bool
-    {
-        return $this->getManifestErrors() !== [];
-    }
-
-    /**
-     * @return array<string,string>
-     */
-    final public function getManifestErrors(): array
-    {
-        return $this->manifest::$errors;
-    }
-
     /**
      * @return Field[]
      */
diff --git a/modules/Plugins/Core/PluginStatus.php b/modules/Plugins/Core/PluginStatus.php
new file mode 100644
index 00000000..8057b0f0
--- /dev/null
+++ b/modules/Plugins/Core/PluginStatus.php
@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Modules\Plugins\Core;
+
+enum PluginStatus: string
+{
+    case INVALID = 'invalid';
+    case INACTIVE = 'inactive';
+    case ACTIVE = 'active';
+}
diff --git a/modules/Plugins/Core/Plugins.php b/modules/Plugins/Core/Plugins.php
index 8919957f..321e904f 100644
--- a/modules/Plugins/Core/Plugins.php
+++ b/modules/Plugins/Core/Plugins.php
@@ -96,7 +96,7 @@ class Plugins
     {
         $activePlugins = [];
         foreach (static::$plugins as $plugin) {
-            if ($plugin->isActive()) {
+            if ($plugin->getStatus() === PluginStatus::ACTIVE) {
                 $activePlugins[] = $plugin;
             }
         }
@@ -111,7 +111,7 @@ class Plugins
     {
         $pluginsWithPodcastSettings = [];
         foreach (static::$plugins as $plugin) {
-            if (! $plugin->isActive()) {
+            if ($plugin->getStatus() !== PluginStatus::ACTIVE) {
                 continue;
             }
 
@@ -132,7 +132,7 @@ class Plugins
     {
         $pluginsWithEpisodeSettings = [];
         foreach (static::$plugins as $plugin) {
-            if (! $plugin->isActive()) {
+            if ($plugin->getStatus() !== PluginStatus::ACTIVE) {
                 continue;
             }
 
@@ -183,7 +183,7 @@ class Plugins
     {
         foreach (static::$plugins as $plugin) {
             // only run hook on active plugins
-            if (! $plugin->isActive()) {
+            if ($plugin->getStatus() !== PluginStatus::ACTIVE) {
                 continue;
             }
 
@@ -282,7 +282,7 @@ class Plugins
             static::$pluginsByVendor[$vendor][] = $plugin;
             ++static::$installedCount;
 
-            if ($plugin->isActive()) {
+            if ($plugin->getStatus() === PluginStatus::ACTIVE) {
                 ++static::$activeCount;
             }
         }
diff --git a/modules/Plugins/Language/en/Plugins.php b/modules/Plugins/Language/en/Plugins.php
index b5e4dbd1..031e3271 100644
--- a/modules/Plugins/Language/en/Plugins.php
+++ b/modules/Plugins/Language/en/Plugins.php
@@ -28,6 +28,7 @@ return [
     'deactivate' => 'Deactivate',
     'active' => 'Active',
     'inactive' => 'Inactive',
+    'invalid' => 'Invalid',
     'uninstall' => 'Uninstall',
     'keywords' => [
         'podcasting20' => 'Podcasting 2.0',
@@ -40,4 +41,9 @@ return [
     'messages' => [
         'saveSettingsSuccess' => '{pluginName} settings were successfully saved!',
     ],
+    'errors' => [
+        'manifestError' => 'Plugin manifest has errors',
+        'manifestMissing' => 'Plugin manifest "{manifestPath}" is missing.',
+        'manifestJsonInvalid' => 'Plugin manifest "{manifestPath}" is not a valid JSON.',
+    ],
 ];
diff --git a/modules/Plugins/Manifest/Manifest.php b/modules/Plugins/Manifest/Manifest.php
index 07323135..1d83ba0f 100644
--- a/modules/Plugins/Manifest/Manifest.php
+++ b/modules/Plugins/Manifest/Manifest.php
@@ -46,9 +46,9 @@ class Manifest extends ManifestObject
         'repository' => Repository::class,
     ];
 
-    protected string $name;
+    protected ?string $name = '???';
 
-    protected string $version;
+    protected ?string $version = 'X.Y.Z';
 
     protected ?string $description = null;
 
diff --git a/modules/Plugins/Manifest/ManifestObject.php b/modules/Plugins/Manifest/ManifestObject.php
index 5be54363..3dc26a47 100644
--- a/modules/Plugins/Manifest/ManifestObject.php
+++ b/modules/Plugins/Manifest/ManifestObject.php
@@ -17,16 +17,19 @@ abstract class ManifestObject
     protected const CASTS = [];
 
     /**
-     * @var array<string,string>
+     * @var array<string,array<string,string>>
      */
-    public static array $errors = [];
+    protected static array $errors = [];
 
     /**
      * @param mixed[] $data
      */
     public function __construct(
-        private readonly array $data
+        protected readonly string $pluginKey,
+        private readonly array $data,
     ) {
+        self::$errors[$pluginKey] = [];
+
         $this->load();
     }
 
@@ -52,7 +55,11 @@ abstract class ManifestObject
         $validation->setRules($this::VALIDATION_RULES);
 
         if (! $validation->run($this->data)) {
-            static::$errors = [...static::$errors, ...$validation->getErrors()];
+            foreach ($validation->getErrors() as $key => $message) {
+                $this->addError($key, $message);
+            }
+
+            $validation->reset();
         }
 
         foreach ($validation->getValidated() as $key => $value) {
@@ -62,11 +69,11 @@ abstract class ManifestObject
                 if (is_array($cast)) {
                     if (is_array($value)) {
                         foreach ($value as $valueKey => $valueElement) {
-                            $value[$valueKey] = new $cast[0]($valueElement);
+                            $value[$valueKey] = new $cast[0]($this->pluginKey, $valueElement);
                         }
                     }
                 } else {
-                    $value = new $cast($value);
+                    $value = new $cast($this->pluginKey, $value ?? []);
                 }
             }
 
@@ -77,8 +84,13 @@ abstract class ManifestObject
     /**
      * @return array<string,string>
      */
-    public function getErrors(): array
+    public static function getPluginErrors(string $pluginKey): array
     {
-        return $this->errors;
+        return self::$errors[$pluginKey];
+    }
+
+    protected function addError(string $errorKey, string $errorMessage): void
+    {
+        self::$errors[$this->pluginKey][$errorKey] = $errorMessage;
     }
 }
diff --git a/modules/Plugins/Manifest/Person.php b/modules/Plugins/Manifest/Person.php
index 2c2cd74c..14514c46 100644
--- a/modules/Plugins/Manifest/Person.php
+++ b/modules/Plugins/Manifest/Person.php
@@ -35,7 +35,7 @@ class Person extends ManifestObject
 
     protected ?URI $url = null;
 
-    public function __construct(array|string $data)
+    public function __construct(string $pluginKey, array|string $data)
     {
         if (is_string($data)) {
             $result = preg_match(self::AUTHOR_STRING_PATTERN, $data, $matches);
@@ -51,6 +51,6 @@ class Person extends ManifestObject
             ];
         }
 
-        parent::__construct($data);
+        parent::__construct($pluginKey, $data);
     }
 }
diff --git a/themes/cp_admin/plugins/_plugin.php b/themes/cp_admin/plugins/_plugin.php
index 0fd1f6c2..b58d4fe1 100644
--- a/themes/cp_admin/plugins/_plugin.php
+++ b/themes/cp_admin/plugins/_plugin.php
@@ -1,9 +1,19 @@
-<article class="flex flex-col p-4 rounded-xl relative bg-elevated border-3 <?= $plugin->isActive() ? 'border-accent-base' : 'border-subtle' ?>">
+<?php
+
+use Modules\Plugins\Core\PluginStatus;
+
+?>
+<article class="flex flex-col p-4 rounded-xl relative bg-elevated border-3 <?= $plugin->getStatus() === PluginStatus::ACTIVE ? 'border-accent-base' : 'border-subtle' ?>">
     <div class="self-end -mb-6">
-    <?php if($plugin->isActive()): ?>
+    <?php if($plugin->getStatus() === PluginStatus::ACTIVE): ?>
+        <?php // @icon('check-fill')?>
         <x-Pill variant="success" icon="check-fill" class="lowercase" size="small"><?= lang('Plugins.active') ?></x-Pill>
-    <?php else: ?>
+    <?php elseif($plugin->getStatus() === PluginStatus::INACTIVE): ?>
+        <?php // @icon('close-fill')?>
         <x-Pill variant="default" icon="close-fill" class="lowercase" size="small"><?= lang('Plugins.inactive') ?></x-Pill>
+    <?php elseif($plugin->getStatus() === PluginStatus::INVALID): ?>
+        <?php // @icon('alert-fill')?>
+        <x-Pill variant="warning" icon="alert-fill" class="lowercase" size="small"><?= lang('Plugins.invalid') ?></x-Pill>
     <?php endif; ?>
     </div>
     <img class="rounded-full min-w-16 max-w-16 aspect-square" src="<?= $plugin->getIconSrc() ?>">
@@ -30,12 +40,12 @@
             <?php endif; ?>
         </div>
         <div class="flex gap-x-2">
-        <?php if($plugin->isActive()): ?>
+        <?php if($plugin->getStatus() === PluginStatus::ACTIVE): ?>
             <form class="flex justify-end" method="POST" action="<?= route_to('plugins-deactivate', $plugin->getVendor(), $plugin->getPackage()) ?>">
                 <?= csrf_field() ?>
                 <x-Button type="submit" variant="danger" size="small"><?= lang('Plugins.deactivate') ?></x-Button>
             </form>
-        <?php else: ?>
+        <?php elseif($plugin->getStatus() === PluginStatus::INACTIVE): ?>
             <form class="flex flex-col items-end justify-end gap-2" method="POST" action="<?= route_to('plugins-activate', $plugin->getVendor(), $plugin->getPackage()) ?>">
                 <?= csrf_field() ?>
                 <x-Button type="submit" variant="secondary" size="small"><?= lang('Plugins.activate') ?></x-Button>
diff --git a/themes/cp_admin/plugins/view.php b/themes/cp_admin/plugins/view.php
index 68e94e41..47702922 100644
--- a/themes/cp_admin/plugins/view.php
+++ b/themes/cp_admin/plugins/view.php
@@ -1,3 +1,7 @@
+<?php use Modules\Plugins\Core\PluginStatus;
+
+?>
+
 <?= $this->extend('_layout') ?>
 
 <?= $this->section('title') ?>
@@ -9,15 +13,20 @@
 <?= $this->endSection() ?>
 
 <?= $this->section('headerLeft') ?>
-<?php if($plugin->isActive()): ?>
+<?php if($plugin->getStatus() === PluginStatus::ACTIVE): ?>
+    <?php // @icon('check-fill')?>
     <x-Pill variant="success" icon="check-fill" class="lowercase"><?= lang('Plugins.active') ?></x-Pill>
-<?php else: ?>
+<?php elseif($plugin->getStatus() === PluginStatus::INACTIVE): ?>
+    <?php // @icon('close-fill')?>
     <x-Pill variant="default" icon="close-fill" class="lowercase"><?= lang('Plugins.inactive') ?></x-Pill>
+<?php elseif($plugin->getStatus() === PluginStatus::INVALID): ?>
+    <?php // @icon('alert-fill')?>
+    <x-Pill variant="warning" icon="alert-fill" class="lowercase"><?= lang('Plugins.invalid') ?></x-Pill>
 <?php endif; ?>
 <?= $this->endSection() ?>
 
 <?= $this->section('headerRight') ?>
-<?php if($plugin->isActive()): ?>
+<?php if($plugin->getStatus() === PluginStatus::ACTIVE): ?>
     <form class="flex justify-end gap-x-2" method="POST" action="<?= route_to('plugins-deactivate', $plugin->getVendor(), $plugin->getPackage()) ?>">
         <?= csrf_field() ?>
         <x-Button type="submit" variant="danger"><?= lang('Plugins.deactivate') ?></x-Button>
@@ -26,7 +35,7 @@
         <x-Button class="ring-2 ring-inset ring-gray-600" iconLeft="equalizer-fill" uri="<?= route_to('plugins-settings-general', $plugin->getVendor(), $plugin->getPackage()) ?>"><?= lang('Plugins.settings') ?></x-Button>
         <?php endif; ?>
     </form>
-<?php else: ?>
+<?php elseif($plugin->getStatus() === PluginStatus::INVALID): ?>
     <form class="flex justify-end gap-x-2" method="POST" action="<?= route_to('plugins-activate', $plugin->getVendor(), $plugin->getPackage()) ?>">
         <?= csrf_field() ?>
         <x-Button type="submit" variant="secondary"><?= lang('Plugins.activate') ?></x-Button>
@@ -39,6 +48,16 @@
 <?= $this->endSection() ?>
 
 <?= $this->section('content') ?>
+<?php if ($plugin->getStatus() === PluginStatus::INVALID): ?>
+    <x-Alert title="<?= lang('Plugins.errors.manifestError') ?>" variant="warning" class="mb-12">
+        <ul>
+            <?php foreach ($plugin->getErrors() as $key => $error): ?>
+                <li><?= $error ?></li>
+            <?php endforeach; ?>
+        </ul>
+    </x-Alert>
+<?php endif; ?>
+
 <div class="flex flex-col items-start justify-center gap-8 mx-auto xl:flex-row-reverse">
     <aside class="w-full pb-8 border-b xl:sticky xl:max-w-xs top-28 border-subtle xl:border-none">
         <h2 class="mb-2 text-2xl font-bold font-display"><?= lang('Plugins.about') ?></h2>
@@ -54,6 +73,9 @@
             </div>
         <?php endif; ?>
         <ul class="flex flex-col gap-2 mt-4">
+            <li class="inline-flex items-center font-mono text-sm gap-x-2"><?= icon('box-2-line', [
+                'class' => 'text-gray-500 text-xl',
+            ]) . $plugin->getVersion() ?></li>
             <?php if ($plugin->getRepository()): ?>
                 <li><a href="<?= $plugin->getRepository()->url ?>" class="inline-flex items-center text-sm gap-x-2 hover:underline" target="_blank" rel="noopener noreferrer"><?= icon('git-repository-fill', [
                     'class' => 'text-gray-500 text-xl',