Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve plugin resilience #10798

Merged
merged 7 commits into from
Jan 16, 2025
Merged

Improve plugin resilience #10798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d86ec5c
pkp/pkp-lib#10514 Patched Hook class to ignore exceptions coming from…
jonasraoni Oct 14, 2024
24241f0
pkp/pkp-lib#10514 Patched PluginRegistry to ignore plugin exceptions
jonasraoni Oct 14, 2024
55a5bf1
pkp/pkp-lib#10514 Added tests
jonasraoni Jan 14, 2025
5fbb8ba
pkp/pkp-lib#10514 Reuse value
jonasraoni Jan 14, 2025
252f21c
pkp/pkp-lib#10514 Improved messages
jonasraoni Jan 14, 2025
ed1711c
pkp/pkp-lib#10514 Intercept hooks handlers defined out of the main pl…
jonasraoni Jan 15, 2025
c2521bb
pkp/pkp-lib#10514 Included callbacks implemented in classes which are…
jonasraoni Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 50 additions & 10 deletions classes/plugins/Hook.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
namespace PKP\plugins;

use PKP\core\Registry;
use ReflectionClass;
use ReflectionFunction;
use Throwable;

class Hook
{
Expand Down Expand Up @@ -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));
}

/**
Expand All @@ -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();
Expand All @@ -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);
}

/**
Expand All @@ -118,15 +121,15 @@ 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[] = [
$hookName, $args
];
}

return self::run($hookName, [$args]);
return static::run($hookName, [$args]);
}

/**
Expand All @@ -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
Expand All @@ -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)) {
jonasraoni marked this conversation as resolved.
Show resolved Hide resolved
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;
}


Expand Down
68 changes: 50 additions & 18 deletions classes/plugins/PluginRegistry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use Illuminate\Support\Arr;
use PKP\core\Registry;
use ReflectionObject;
use Throwable;

class PluginRegistry
{
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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"
Expand All @@ -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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -184,17 +197,22 @@ 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}\"");
}

// 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);
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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)) {
Expand All @@ -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;
Expand All @@ -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.
Expand All @@ -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) {
Expand Down
Loading