From d86ec5ce3ce7613b033bb60b49c48f4596a600ae Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Mon, 14 Oct 2024 12:55:22 +0300 Subject: [PATCH 1/7] pkp/pkp-lib#10514 Patched Hook class to ignore exceptions coming from plugins --- classes/plugins/Hook.php | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/classes/plugins/Hook.php b/classes/plugins/Hook.php index f0145b33318..76bc8dd9e1d 100644 --- a/classes/plugins/Hook.php +++ b/classes/plugins/Hook.php @@ -17,6 +17,7 @@ namespace PKP\plugins; use PKP\core\Registry; +use Throwable; class Hook { @@ -60,7 +61,7 @@ public static function &getHooks(?string $hookName = null): ?array */ public static function addUnsupportedHooks(...$hookNames): void { - self::$unsupportedHooks = array_merge(self::$unsupportedHooks, array_flip($hookNames)); + static::$unsupportedHooks = array_merge(static::$unsupportedHooks, array_flip($hookNames)); } /** @@ -81,7 +82,7 @@ public static function clear(string $hookName): void */ public static function add(string $hookName, callable $callback, int $hookSequence = self::SEQUENCE_NORMAL): void { - if (isset(self::$unsupportedHooks[$hookName])) { + if (isset(static::$unsupportedHooks[$hookName])) { throw new \Exception("Hook {$hookName} is not supported (possibly removed) and callbacks should not be added to it!"); } $hooks = & static::getHooks(); @@ -96,7 +97,7 @@ public static function add(string $hookName, callable $callback, int $hookSequen */ public static function register(string $hookName, callable $callback, int $hookSequence = self::SEQUENCE_NORMAL): void { - self::add($hookName, $callback, $hookSequence); + static::add($hookName, $callback, $hookSequence); } /** @@ -118,7 +119,7 @@ public static function call(string $hookName, array $args = []): mixed // Called only by Unit Test // This behaviour is DEPRECATED and not replicated in the preferred // Hook::call function. - if (self::rememberCalledHooks(true)) { + if (static::rememberCalledHooks(true)) { // Remember the called hooks for testing. $calledHooks = & static::getCalledHooks(); $calledHooks[] = [ @@ -126,7 +127,7 @@ public static function call(string $hookName, array $args = []): mixed ]; } - return self::run($hookName, [$args]); + return static::run($hookName, [$args]); } /** @@ -146,7 +147,7 @@ public static function run(string $hookName, array $args = []): bool { $hooks = & static::getHooks(); if (!isset($hooks[$hookName])) { - return self::CONTINUE; + return static::CONTINUE; } // Sort callbacks if the list is dirty @@ -155,15 +156,25 @@ public static function run(string $hookName, array $args = []): bool $hooks[$hookName]['dirty'] = false; } - foreach ($hooks[$hookName]['hooks'] as $priority => $hookList) { + foreach ($hooks[$hookName]['hooks'] as $hookList) { foreach ($hookList as $callback) { - if (call_user_func_array($callback, [$hookName, ...$args]) === self::ABORT) { - return self::ABORT; + try { + if (call_user_func_array($callback, [$hookName, ...$args]) === static::ABORT) { + return static::ABORT; + } + } catch (Throwable $e) { + foreach ($e->getTrace() as $stackFrame) { + if (is_subclass_of($stackFrame['class'] ?? null, Plugin::class)) { + error_log("Hook handler failure detected at {$stackFrame['class']}\n{$e}"); + continue 2; + } + } + throw $e; } } } - return self::CONTINUE; + return static::CONTINUE; } From 24241f0927f36f1b3f7bc99409237eb440fafb52 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Mon, 14 Oct 2024 13:03:15 +0300 Subject: [PATCH 2/7] pkp/pkp-lib#10514 Patched PluginRegistry to ignore plugin exceptions --- classes/plugins/PluginRegistry.php | 63 ++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 17 deletions(-) diff --git a/classes/plugins/PluginRegistry.php b/classes/plugins/PluginRegistry.php index db507a554fb..eae056792af 100644 --- a/classes/plugins/PluginRegistry.php +++ b/classes/plugins/PluginRegistry.php @@ -24,6 +24,7 @@ use Illuminate\Support\Arr; use PKP\core\Registry; use ReflectionObject; +use Throwable; class PluginRegistry { @@ -69,11 +70,21 @@ public static function getAllPlugins(): array */ public static function register(string $category, Plugin $plugin, string $path, ?int $mainContextId = null): bool { - $pluginName = $plugin->getName(); + try { + $pluginName = $plugin->getName(); + } catch (Throwable $e) { + error_log("Plugin getName() call has failed\n{$e}"); + return false; + } $plugins = & static::getPlugins(); // If the plugin is already loaded or failed/refused to register - if (isset($plugins[$category][$pluginName]) || !$plugin->register($category, $path, $mainContextId)) { + try { + if (isset($plugins[$category][$pluginName]) || !$plugin->register($category, $path, $mainContextId)) { + return false; + } + } catch (Throwable $e) { + error_log("Plugin failed to be registered\n{$e}"); return false; } @@ -111,8 +122,8 @@ public static function loadCategory(string $category, bool $enabledOnly = false, static $cache; $key = implode("\0", func_get_args()); $plugins = $cache[$key] ??= $enabledOnly && Application::isInstalled() - ? static::_loadFromDatabase($category, $mainContextId) - : static::_loadFromDisk($category); + ? static::loadFromDatabase($category, $mainContextId) + : static::loadFromDisk($category); // Fire a hook prior to registering plugins for a category // n.b.: this should not be used from a PKPPlugin::register() call to "jump categories" @@ -130,7 +141,7 @@ public static function loadCategory(string $category, bool $enabledOnly = false, Hook::call("PluginRegistry::categoryLoaded::{$category}", [&$plugins]); // Sort the plugins by priority before returning. - uasort($plugins, fn (Plugin $a, Plugin $b) => $a->getSeq() - $b->getSeq()); + uasort($plugins, fn (Plugin $a, Plugin $b) => static::getPluginSeq($a) - static::getPluginSeq($b)); return $plugins; } @@ -148,7 +159,7 @@ public static function loadCategory(string $category, bool $enabledOnly = false, */ public static function loadPlugin(string $category, string $pluginName, ?int $mainContextId = null): ?Plugin { - if ($plugin = static::_instantiatePlugin($category, $pluginName)) { + if ($plugin = static::instantiatePlugin($category, $pluginName)) { static::register($category, $plugin, self::PLUGINS_PREFIX . "{$category}/{$pluginName}", $mainContextId); } return $plugin; @@ -184,7 +195,7 @@ public static function loadAllPlugins(bool $enabledOnly = false): array /** * Instantiate a plugin. */ - private static function _instantiatePlugin(string $category, string $pluginName, ?string $classToCheck = null): ?Plugin + private static function instantiatePlugin(string $category, string $pluginName, ?string $classToCheck = null): ?Plugin { if (!preg_match('/^[a-z0-9]+$/i', $pluginName)) { throw new Exception("Invalid product name \"{$pluginName}\""); @@ -192,9 +203,14 @@ private static function _instantiatePlugin(string $category, string $pluginName, // First, try a namespaced class name matching the installation directory. $pluginClassName = "\\APP\\plugins\\{$category}\\{$pluginName}\\" . ucfirst($pluginName) . 'Plugin'; - $plugin = class_exists($pluginClassName) - ? new $pluginClassName() - : static::_deprecatedInstantiatePlugin($category, $pluginName); + try { + $plugin = class_exists($pluginClassName) + ? new $pluginClassName() + : static::deprecatedInstantiatePlugin($category, $pluginName); + } catch (Throwable $e) { + error_log("Plugin instantiation has failed\n{$e}"); + return null; + } $classToCheck = $classToCheck ?: Plugin::class; $isObject = is_object($plugin); @@ -213,15 +229,15 @@ private static function _instantiatePlugin(string $category, string $pluginName, /** * Attempts to retrieve plugins from the database. */ - private static function _loadFromDatabase(string $category, ?int $mainContextId = null): array + private static function loadFromDatabase(string $category, ?int $mainContextId = null): array { $plugins = []; $categoryDir = static::PLUGINS_PREFIX . $category; $products = Application::get()->getEnabledProducts("plugins.{$category}", $mainContextId); foreach ($products as $product) { $name = $product->getProduct(); - if ($plugin = static::_instantiatePlugin($category, $name, $product->getProductClassname())) { - $plugins[$plugin->getSeq()]["{$categoryDir}/{$name}"] = $plugin; + if ($plugin = static::instantiatePlugin($category, $name, $product->getProductClassname())) { + $plugins[static::getPluginSeq($plugin)]["{$categoryDir}/{$name}"] = $plugin; } } return $plugins; @@ -230,7 +246,7 @@ private static function _loadFromDatabase(string $category, ?int $mainContextId /** * Get all plug-ins from disk without querying the database, used during installation. */ - private static function _loadFromDisk(string $category): array + private static function loadFromDisk(string $category): array { $categoryDir = static::PLUGINS_PREFIX . $category; if (!is_dir($categoryDir)) { @@ -242,8 +258,8 @@ private static function _loadFromDisk(string $category): array continue; } $pluginName = $path->getFilename(); - if ($plugin = static::_instantiatePlugin($category, $pluginName)) { - $plugins[$plugin->getSeq()]["{$categoryDir}/{$pluginName}"] = $plugin; + if ($plugin = static::instantiatePlugin($category, $pluginName)) { + $plugins[static::getPluginSeq($plugin)]["{$categoryDir}/{$pluginName}"] = $plugin; } } return $plugins; @@ -254,7 +270,7 @@ private static function _loadFromDisk(string $category): array * * @deprecated 3.4.0 Old way to instantiate a plugin */ - private static function _deprecatedInstantiatePlugin(string $category, string $pluginName): ?Plugin + private static function deprecatedInstantiatePlugin(string $category, string $pluginName): ?Plugin { $pluginPath = static::PLUGINS_PREFIX . "{$category}/{$pluginName}"; // Try the plug-in wrapper for backwards compatibility. @@ -272,6 +288,19 @@ private static function _deprecatedInstantiatePlugin(string $category, string $p return null; } + + /** + * Retrieves the plugin's getSeq() or 0 on failure + */ + private static function getPluginSeq(Plugin $plugin): int + { + try { + return $plugin->getSeq(); + } catch (Throwable $e) { + error_log("Plugin getSeq() call has failed\n{$e}"); + return 0; + } + } } if (!PKP_STRICT_MODE) { From 55a5bf1011b0ab78dae321fcb0aed6f448f55565 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Tue, 14 Jan 2025 19:44:04 +0300 Subject: [PATCH 3/7] pkp/pkp-lib#10514 Added tests --- tests/classes/plugins/PluginTest.php | 128 +++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 tests/classes/plugins/PluginTest.php diff --git a/tests/classes/plugins/PluginTest.php b/tests/classes/plugins/PluginTest.php new file mode 100644 index 00000000000..733da90e250 --- /dev/null +++ b/tests/classes/plugins/PluginTest.php @@ -0,0 +1,128 @@ + Date: Tue, 14 Jan 2025 21:06:04 +0300 Subject: [PATCH 4/7] pkp/pkp-lib#10514 Reuse value --- classes/plugins/Hook.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/classes/plugins/Hook.php b/classes/plugins/Hook.php index 76bc8dd9e1d..91dfb32b495 100644 --- a/classes/plugins/Hook.php +++ b/classes/plugins/Hook.php @@ -164,8 +164,8 @@ public static function run(string $hookName, array $args = []): bool } } catch (Throwable $e) { foreach ($e->getTrace() as $stackFrame) { - if (is_subclass_of($stackFrame['class'] ?? null, Plugin::class)) { - error_log("Hook handler failure detected at {$stackFrame['class']}\n{$e}"); + if (is_subclass_of($class = $stackFrame['class'] ?? null, Plugin::class)) { + error_log("Hook handler failure detected at {$class}\n{$e}"); continue 2; } } From 252f21c3069f57b502cdbc0027478d5a26008304 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Tue, 14 Jan 2025 22:11:15 +0300 Subject: [PATCH 5/7] pkp/pkp-lib#10514 Improved messages --- classes/plugins/Hook.php | 4 ++-- classes/plugins/PluginRegistry.php | 13 ++++++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/classes/plugins/Hook.php b/classes/plugins/Hook.php index 91dfb32b495..7ef758d8beb 100644 --- a/classes/plugins/Hook.php +++ b/classes/plugins/Hook.php @@ -164,8 +164,8 @@ public static function run(string $hookName, array $args = []): bool } } catch (Throwable $e) { foreach ($e->getTrace() as $stackFrame) { - if (is_subclass_of($class = $stackFrame['class'] ?? null, Plugin::class)) { - error_log("Hook handler failure detected at {$class}\n{$e}"); + if (is_subclass_of($pluginClass = $stackFrame['class'] ?? null, Plugin::class)) { + error_log("Plugin {$pluginClass} failed to handle the hook {$hookName}\n{$e}"); continue 2; } } diff --git a/classes/plugins/PluginRegistry.php b/classes/plugins/PluginRegistry.php index eae056792af..c25bd9fbf5f 100644 --- a/classes/plugins/PluginRegistry.php +++ b/classes/plugins/PluginRegistry.php @@ -73,7 +73,8 @@ public static function register(string $category, Plugin $plugin, string $path, try { $pluginName = $plugin->getName(); } catch (Throwable $e) { - error_log("Plugin getName() call has failed\n{$e}"); + $pluginClass = $plugin::class; + error_log("Plugin {$pluginClass} getName() call has failed\n{$e}"); return false; } $plugins = & static::getPlugins(); @@ -84,7 +85,8 @@ public static function register(string $category, Plugin $plugin, string $path, return false; } } catch (Throwable $e) { - error_log("Plugin failed to be registered\n{$e}"); + $pluginClass = $plugin::class; + error_log("Plugin {$pluginClass} failed to be registered\n{$e}"); return false; } @@ -208,7 +210,7 @@ private static function instantiatePlugin(string $category, string $pluginName, ? new $pluginClassName() : static::deprecatedInstantiatePlugin($category, $pluginName); } catch (Throwable $e) { - error_log("Plugin instantiation has failed\n{$e}"); + error_log("Instantiation of the plugin {$category}/{$pluginName} has failed\n{$e}"); return null; } @@ -220,7 +222,7 @@ private static function instantiatePlugin(string $category, string $pluginName, } if ($plugin !== null && !($plugin instanceof $classToCheck)) { $type = $isObject ? $plugin::class : gettype($plugin); - error_log(new Exception("Plugin {$pluginName} expected to inherit from {$classToCheck}, actual type {$type}")); + error_log(new Exception("Plugin {$category}/{$pluginName} expected to inherit from {$classToCheck}, actual type {$type}")); return null; } return $plugin; @@ -297,7 +299,8 @@ private static function getPluginSeq(Plugin $plugin): int try { return $plugin->getSeq(); } catch (Throwable $e) { - error_log("Plugin getSeq() call has failed\n{$e}"); + $pluginClass = $plugin::class; + error_log("Plugin {$pluginClass} getSeq() call has failed\n{$e}"); return 0; } } From ed1711c6d02e4620c3a24e14436acd81e1af949d Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Wed, 15 Jan 2025 19:19:37 +0300 Subject: [PATCH 6/7] pkp/pkp-lib#10514 Intercept hooks handlers defined out of the main plugin class --- classes/plugins/Hook.php | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/classes/plugins/Hook.php b/classes/plugins/Hook.php index 7ef758d8beb..b40f8323f59 100644 --- a/classes/plugins/Hook.php +++ b/classes/plugins/Hook.php @@ -17,6 +17,8 @@ namespace PKP\plugins; use PKP\core\Registry; +use ReflectionClass; +use ReflectionFunction; use Throwable; class Hook @@ -163,10 +165,24 @@ public static function run(string $hookName, array $args = []): bool return static::ABORT; } } catch (Throwable $e) { + $pluginDirectories = [realpath(BASE_SYS_DIR . '/' . PKP_LIB_PATH . '/plugins'), realpath(BASE_SYS_DIR . '/plugins')]; foreach ($e->getTrace() as $stackFrame) { - if (is_subclass_of($pluginClass = $stackFrame['class'] ?? null, Plugin::class)) { - error_log("Plugin {$pluginClass} failed to handle the hook {$hookName}\n{$e}"); - continue 2; + $filename = (($class = $stackFrame['class'] ?? null) ? (new ReflectionClass($class))->getFileName() : null) + ?? (($function = $stackFrame['function'] ?? null) ? (new ReflectionFunction($function))->getFileName() : null) + ?? $stackFrame['file'] + ?? null; + + if (!$filename) { + continue; + } + + $filename = realpath($filename); + foreach ($pluginDirectories as $pluginDirectory) { + if (strpos($filename, $pluginDirectory) === 0) { + $pieces = explode(DIRECTORY_SEPARATOR, substr($filename, strlen($pluginDirectory) + 1)); + error_log("Plugin {$pieces[0]}/{$pieces[1]} failed to handle the hook {$hookName}\n{$e}"); + continue 3; + } } } throw $e; From c2521bb03088123b5c46189f6ab2efaea7902347 Mon Sep 17 00:00:00 2001 From: Jonas Raoni Soares da Silva Date: Thu, 16 Jan 2025 14:02:54 +0300 Subject: [PATCH 7/7] pkp/pkp-lib#10514 Included callbacks implemented in classes which are derived from the Plugin class and added comments --- classes/plugins/Hook.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/classes/plugins/Hook.php b/classes/plugins/Hook.php index b40f8323f59..902c863f2ab 100644 --- a/classes/plugins/Hook.php +++ b/classes/plugins/Hook.php @@ -165,17 +165,29 @@ public static function run(string $hookName, array $args = []): bool return static::ABORT; } } catch (Throwable $e) { + /** + * This is an attempt to improve the application's resilience against errors inside plugins + * If an exception happens at a hook handler which was implemented inside a plugin (callbacks implemented inside a plugin-derived class or located inside the folders "lib/pkp/plugins" or "plugins"), the exception will be logged and the hook flow will continue + * @see https://github.com/pkp/pkp-lib/discussions/9083 + */ $pluginDirectories = [realpath(BASE_SYS_DIR . '/' . PKP_LIB_PATH . '/plugins'), realpath(BASE_SYS_DIR . '/plugins')]; foreach ($e->getTrace() as $stackFrame) { - $filename = (($class = $stackFrame['class'] ?? null) ? (new ReflectionClass($class))->getFileName() : null) + // If the code was implemented inside a plugin class, let the hook flow continue + if (is_subclass_of($class = $stackFrame['class'] ?? null, Plugin::class)) { + error_log("Plugin {$class} failed to handle the hook {$hookName}\n{$e}"); + continue 2; + } + + // Attempt to recover the file where the callback was implemented + $filename = ($class ? (new ReflectionClass($class))->getFileName() : null) ?? (($function = $stackFrame['function'] ?? null) ? (new ReflectionFunction($function))->getFileName() : null) ?? $stackFrame['file'] ?? null; - if (!$filename) { continue; } + // If the code was implemented inside a plugin folder, let the hook flow continue $filename = realpath($filename); foreach ($pluginDirectories as $pluginDirectory) { if (strpos($filename, $pluginDirectory) === 0) { @@ -185,6 +197,7 @@ public static function run(string $hookName, array $args = []): bool } } } + throw $e; } }