diff --git a/classes/plugins/Hook.php b/classes/plugins/Hook.php index f0145b33318..902c863f2ab 100644 --- a/classes/plugins/Hook.php +++ b/classes/plugins/Hook.php @@ -17,6 +17,9 @@ namespace PKP\plugins; use PKP\core\Registry; +use ReflectionClass; +use ReflectionFunction; +use Throwable; class Hook { @@ -60,7 +63,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 +84,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 +99,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 +121,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 +129,7 @@ public static function call(string $hookName, array $args = []): mixed ]; } - return self::run($hookName, [$args]); + return static::run($hookName, [$args]); } /** @@ -146,7 +149,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 +158,52 @@ 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) { + /** + * 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) { + // 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) { + $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; } } } - return self::CONTINUE; + return static::CONTINUE; } diff --git a/classes/plugins/PluginRegistry.php b/classes/plugins/PluginRegistry.php index db507a554fb..c25bd9fbf5f 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,23 @@ 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) { + $pluginClass = $plugin::class; + error_log("Plugin {$pluginClass} 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) { + $pluginClass = $plugin::class; + error_log("Plugin {$pluginClass} failed to be registered\n{$e}"); return false; } @@ -111,8 +124,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 +143,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 +161,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 +197,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 +205,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("Instantiation of the plugin {$category}/{$pluginName} has failed\n{$e}"); + return null; + } $classToCheck = $classToCheck ?: Plugin::class; $isObject = is_object($plugin); @@ -204,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; @@ -213,15 +231,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 +248,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 +260,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 +272,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 +290,20 @@ 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) { + $pluginClass = $plugin::class; + error_log("Plugin {$pluginClass} getSeq() call has failed\n{$e}"); + return 0; + } + } } if (!PKP_STRICT_MODE) { 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 @@ +