From 41825fc01d7f62a2d26ec29b655a07155230f4f0 Mon Sep 17 00:00:00 2001 From: JJ Fullmer <7743340+darksidemilk@users.noreply.github.com> Date: Wed, 19 Aug 2026 18:46:50 -0500 Subject: [PATCH] A hook is not a kind of event, so stop saying it is Hook extended Event, which made `$listener instanceof Event` -- the only type check separating the two -- true for every hook in existence. #1194 closed the defect that fell out of it (a hook could register as an event listener and be dispatched through notify()) with a refusal in acceptListener(); it did not fix the modelling that made the wrong registration look reasonable. Hook now extends FOGBase. The boilerplate the two genuinely share -- $name, $description, $active, the three log settings, log() and the constructor -- moved to a Listener trait that both use, so nothing a hook relied on went away with the parent. What did go away is run() and onEvent(): both are the event dispatch surface, both were empty bodies, and no hook in core or in fog-plugins calls either. log() had to move rather than stay on Event. FOGBase declares a log() with the identical signature and a completely different job -- it writes a history row -- so a hook that lost Event's copy would not have failed, it would have quietly called that one. Two core hooks call self::log(). acceptListener() now tests for Hook before Event. Both arms refuse a hook once the inheritance is gone, and whichever runs first decides the message; "a hook is not an event listener" is the one that tells a plugin author what they did, where "Class must extend event" would send them off to add the very `extends` this removes. Blast radius, measured before the change: grep -rn 'instanceof Event' packages/ # 1, in eventmanager grep -rn 'instanceof Event' ~/fog-plugins # 0 across 72 hooks grep -rLE 'parent::__construct' **/*.hook.php # none grep -rl 'logLevel|logToFile|logToBrowser|::log(' # 2, both core, # both $active=false Verified by booting the app from a shadow tree against the live 1.6 database and constructing every listener the managers load: 52 hooks and 6 events, 0 problems, each checked for the four properties the parent used to supply and for log() resolving to the trait rather than FOGBase. The driver is scripts/background_scripts/boot_hooks_1203.php. hook-event-contract.test.php gains three guards, each mutation-verified: reverting the `extends` fails the first, dropping `use Listener` fails the other two. Its Event::onEvent() case now reflects on an event rather than a hook -- passing a hook was only possible while the inheritance stood, and the assertion is unchanged. Closes #1203. Co-Authored-By: Claude --- docs/adr/0017-hook-dispatch-contract.md | 18 +- docs/refactor-facts.md | 5 + packages/web/lib/fog/event.class.php | 136 +-------------- packages/web/lib/fog/eventmanager.class.php | 28 +-- packages/web/lib/fog/hook.class.php | 20 ++- packages/web/lib/fog/listener.class.php | 183 ++++++++++++++++++++ tests/hook-event-contract.test.php | 34 +++- 7 files changed, 272 insertions(+), 152 deletions(-) create mode 100644 packages/web/lib/fog/listener.class.php diff --git a/docs/adr/0017-hook-dispatch-contract.md b/docs/adr/0017-hook-dispatch-contract.md index a81a391510..62dd2156e8 100644 --- a/docs/adr/0017-hook-dispatch-contract.md +++ b/docs/adr/0017-hook-dispatch-contract.md @@ -101,9 +101,21 @@ name into the response. A `Hook` is now refused where an event listener is expected, and `Event::onEvent()`'s default does nothing. -Making `Hook` extend `FOGBase` directly, so the two are genuine peers, is the -honest modelling change and is **not** taken here: it changes the answer to -`$obj instanceof Event` for every hook in existence and needs its own issue. +**Superseded by #1203.** `Hook` extends `FOGBase` directly and the two are +genuine peers; the boilerplate they actually share — `$name`, `$description`, +`$active`, the three log settings, `log()` and the constructor — moved to the +`Listener` trait, which both use. `$obj instanceof Event` now answers what it +always meant to. + +The blast-radius argument that held this back was made on its own evidence +before the change landed: zero `instanceof Event` in core outside the two +dispatch classes, zero in `packages/service`, and zero across the 72 hooks and +15 events in `fog-plugins`; every hook in both trees declares its own `$active` +and calls `parent::__construct()`; and exactly two files in the estate use the +log settings, both core and both `$active = false`. `log()` went into the trait +rather than staying on `Event` because `FOGBase` declares a `log()` with an +identical signature and a different job, so a hook that lost `Event`'s copy +would not have failed — it would have quietly called that one. ### 5. `notify()` on a HookManager is an error diff --git a/docs/refactor-facts.md b/docs/refactor-facts.md index f6453a3b78..4ec5a1b0aa 100644 --- a/docs/refactor-facts.md +++ b/docs/refactor-facts.md @@ -355,6 +355,11 @@ third-party code only. ``` ### F-18 — `Hook extends Event` defeats the only type check separating the two +**Closed by #1203**: `Hook` extends `FOGBase` and both use the `Listener` +trait, so `instanceof Event` is false for a hook. The refusal below stays as +the specific diagnostic, and runs *before* the `instanceof Event` arm so a +plugin author is told what they actually did. Recorded as found: + `EventManager::register()` guards with `$listener instanceof Event`, which a `Hook` satisfies. A hook object can therefore be registered as an event listener, and `notify()` then calls `Event::onEvent()` — whose default body diff --git a/packages/web/lib/fog/event.class.php b/packages/web/lib/fog/event.class.php index 721a463da0..a64b62116b 100644 --- a/packages/web/lib/fog/event.class.php +++ b/packages/web/lib/fog/event.class.php @@ -30,140 +30,8 @@ */ abstract class Event extends FOGBase { - /** - * The name. - * - * @var string - */ - protected $name; - /** - * A description. - * - * @var string - */ - protected $description; - /** - * The active flag of this. - * - * @var bool - */ - public $active = true; - /** - * The log level. - * - * @var int - */ - public $logLevel = 0; - /** - * Whether to store log to file - * - * @var bool - */ - public $logToFile = false; - /** - * Whether to show log in browser - * - * @var bool - */ - public $logToBrowser = true; - /** - * Initializes the base elements of the event or hook - * - * @return void - */ - public function __construct() - { - parent::__construct(); - if (!self::$FOGUser->isValid()) { - self::$FOGUser =& $GLOBALS['currentUser']; - } - } - /** - * How to log this file. - * - * @param string $txt The text to log. - * @param int $curlog The logLevel setting. - * @param int $logfile The logToFile setting. - * @param int $logbrow The logToBrowser setting. - * @param object $obj The object. - * @param int $level The basic log level. - * - * @return void - */ - public static function log( - $txt, - $curlog, - $logfile, - $logbrow, - $obj, - $level = 1 - ) { - if (self::$ajax) { - return; - } - $findArr = [ - "#\r#", - "#\n#", - '#\s+#', - '# ,#', - ]; - $repArr = [ - '', - ' ', - ' ', - ',' - ]; - $txt = preg_replace($findArr, $repArr, $txt); - $txt = trim($txt); - if (empty($txt)) { - return; - } - $txt = sprintf( - '[%s] %s', - self::niceDate()->format('Y-m-d H:i:s'), - $txt - ); - $msg = '%s
' - . '' - . '%s
%s'; - if (!self::$post && $logbrow) { - if ($curlog >= $level) { - printf( - $msg, - "\n", - $txt, - "\n" - ); - } - } - $typePath = 'events'; - if ($obj instanceof Hook) { - $typePath = 'hooks'; - } - if ($logfile) { - $log = sprintf( - '%s%slib%s%s%s%s.log', - BASEPATH, - DS, - DS, - $typePath, - DS, - // Short name: this becomes a log filename. - self::shortName($obj) - ); - $logtxt = sprintf( - "[%s] %s\r\n", - self::niceDate()->format('d-m-Y H:i:s'), - $txt - ); - file_put_contents( - $log, - $logtxt, - FILE_APPEND | LOCK_EX - ); - } - } + use Listener; + /** * Simply adds the run method, though should be more defined. * diff --git a/packages/web/lib/fog/eventmanager.class.php b/packages/web/lib/fog/eventmanager.class.php index 757f6f8a0e..269262f5bb 100644 --- a/packages/web/lib/fog/eventmanager.class.php +++ b/packages/web/lib/fog/eventmanager.class.php @@ -173,23 +173,25 @@ private static function _recordEventName($event) */ protected function acceptListener($listener) { - if (!($listener instanceof Event)) { - throw new \Exception(_('Class must extend event')); - } - // Hook extends Event, so the guard above accepts a hook -- the only - // type check separating the two, and it does not. What follows from - // that is not theoretical: notify() would then call Event::onEvent() - // on it, and hooks do not implement onEvent(), so the inherited - // default ran. That default used to print the event name into the - // response, which on a client protocol endpoint is arbitrary text in - // front of a ##@GO reply. + // Hooks first, and it has to stay first. Both arms refuse a hook now + // that Hook no longer extends Event (#1203), so whichever runs first + // decides the message -- and "a hook is not an event listener" is the + // one that tells a plugin author what they did. Falling through to + // "Class must extend event" would send them off to add an `extends` + // that is exactly the thing #1203 removed. // - // The two have genuinely different dispatch contracts -- see - // HookManager::notify() -- so a hook is refused here rather than - // silently dispatched as something it is not. + // The refusal itself predates the hierarchy change and is not made + // redundant by it: the two have genuinely different dispatch + // contracts -- see HookManager::notify() -- and before #1194 a hook + // registered here would be handed to Event::onEvent(), whose default + // printed the event name into the response, which on a client + // protocol endpoint is arbitrary text in front of a ##@GO reply. if ($listener instanceof Hook) { throw new \Exception(_('A hook is not an event listener')); } + if (!($listener instanceof Event)) { + throw new \Exception(_('Class must extend event')); + } } /** * Renders an event name for a log line. diff --git a/packages/web/lib/fog/hook.class.php b/packages/web/lib/fog/hook.class.php index 4467840aba..abbce4778e 100644 --- a/packages/web/lib/fog/hook.class.php +++ b/packages/web/lib/fog/hook.class.php @@ -30,8 +30,26 @@ * @license http://opensource.org/licenses/gpl-3.0 GPLv3 * @link https://fogproject.org */ -abstract class Hook extends Event +abstract class Hook extends FOGBase { + /* + * NOT `extends Event`. A hook exists to change something in flight and is + * dispatched by HookManager::processEvent(), which calls a method the + * listener named and hands it a payload by reference; an event exists to + * be told something happened and is dispatched by EventManager::notify(), + * which calls a fixed onEvent() and discards the result. Neither is a kind + * of the other, and while the inheritance stood, `instanceof Event` -- the + * one type check separating them -- said they were. See #1203 and + * docs/adr/0017-hook-dispatch-contract.md. + * + * The boilerplate the two genuinely share ($name, $active, the log + * settings, log() itself) comes from the Listener trait, so nothing a hook + * relied on went away with the parent. What did go away is run() and + * onEvent(): both are the EVENT dispatch surface, both were empty, and no + * hook in core or in fog-plugins calls either. + */ + use Listener; + /** * Function enables reportTypes * to allow plugins, and all hooks really, to tie into diff --git a/packages/web/lib/fog/listener.class.php b/packages/web/lib/fog/listener.class.php new file mode 100644 index 0000000000..84b5199a90 --- /dev/null +++ b/packages/web/lib/fog/listener.class.php @@ -0,0 +1,183 @@ + + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ + +namespace FOG; + +/** + * What a hook and an event both are, and nothing else. + * + * @category Listener + * @package FOGProject + * @author Tom Elliott + * @license http://opensource.org/licenses/gpl-3.0 GPLv3 + * @link https://fogproject.org + */ +trait Listener +{ + /** + * The name. + * + * @var string + */ + protected $name; + /** + * The description. + * + * @var string + */ + protected $description; + /** + * Is this listener active? + * + * @var bool + */ + public $active = true; + /** + * Items log level. + * + * @var int + */ + public $logLevel = 0; + /** + * Log to file? + * + * @var bool + */ + public $logToFile = false; + /** + * Log to browser? + * + * @var bool + */ + public $logToBrowser = true; + /** + * Initializes the listener. + * + * @return void + */ + public function __construct() + { + parent::__construct(); + if (!self::$FOGUser->isValid()) { + self::$FOGUser =& $GLOBALS['currentUser']; + } + } + /** + * How to log this file. + * + * @param string $txt The text to log. + * @param int $curlog The logLevel setting. + * @param int $logfile The logToFile setting. + * @param int $logbrow The logToBrowser setting. + * @param object $obj The object. + * @param int $level The basic log level. + * + * @return void + */ + public static function log( + $txt, + $curlog, + $logfile, + $logbrow, + $obj, + $level = 1 + ) { + if (self::$ajax) { + return; + } + $findArr = [ + "#\r#", + "#\n#", + '#\s+#', + '# ,#', + ]; + $repArr = [ + '', + ' ', + ' ', + ',' + ]; + $txt = preg_replace($findArr, $repArr, $txt); + $txt = trim($txt); + if (empty($txt)) { + return; + } + $txt = sprintf( + '[%s] %s', + self::niceDate()->format('Y-m-d H:i:s'), + $txt + ); + $msg = '%s
' + . '' + . '%s
%s'; + if (!self::$post && $logbrow) { + if ($curlog >= $level) { + printf( + $msg, + "\n", + $txt, + "\n" + ); + } + } + $typePath = 'events'; + if ($obj instanceof Hook) { + $typePath = 'hooks'; + } + if ($logfile) { + $log = sprintf( + '%s%slib%s%s%s%s.log', + BASEPATH, + DS, + DS, + $typePath, + DS, + // Short name: this becomes a log filename. + self::shortName($obj) + ); + $logtxt = sprintf( + "[%s] %s\r\n", + self::niceDate()->format('d-m-Y H:i:s'), + $txt + ); + file_put_contents( + $log, + $logtxt, + FILE_APPEND | LOCK_EX + ); + } + } +} + +/* + * Compatibility alias, for the same reason every other name in this tree has + * one: a plugin writing `use Listener;` unqualified keeps working. + * Supported for all of 1.6; see docs/adr/0013. + */ +class_alias(__NAMESPACE__ . '\\Listener', 'Listener'); diff --git a/tests/hook-event-contract.test.php b/tests/hook-event-contract.test.php index 20a0bc36d4..122e8d361a 100644 --- a/tests/hook-event-contract.test.php +++ b/tests/hook-event-contract.test.php @@ -279,6 +279,32 @@ class CharManager extends \FOG\HookManager . trim($hookRefusal); } +// F-18, third part (#1203). The inheritance itself is gone: Hook extends +// FOGBase and takes the shared listener boilerplate from the Listener trait. +// `instanceof Event` now answers what it always meant to. +if ($hook instanceof \FOG\Event) { + $fails[] = 'Hook is still an Event, so `instanceof Event` still cannot' + . ' tell a hook from an event listener'; +} +// The boilerplate that came with the parent has to still be there, or every +// hook in every plugin loses its activation flag. +foreach (['active', 'logLevel', 'logToFile', 'logToBrowser'] as $prop) { + if (!property_exists('FOG\Hook', $prop)) { + $fails[] = "a hook lost \$$prop when it stopped extending Event"; + } +} +// And the log() a hook resolves must be the listener one, not FOGBase's. +// The two have IDENTICAL signatures and completely different jobs -- FOGBase's +// writes a history row -- so a hook that lost the trait would not fail, it +// would quietly call the wrong one. hookdebugger and template both call +// self::log(). A trait method's declaring class is the class that used it. +$hookLog = (new \ReflectionMethod('FOG\Hook', 'log')) + ->getDeclaringClass()->getName(); +if ('FOG\Hook' !== $hookLog) { + $fails[] = 'Hook::log() resolves to ' . $hookLog . ', not the Listener' + . ' trait -- a hook calling self::log() now writes a history row'; +} + // F-18, second half. Event::onEvent()'s default used to print the event name // into the response, so an event class that had not overridden it wrote text // into whatever output was being produced -- including a client protocol reply @@ -287,8 +313,14 @@ class CharManager extends \FOG\HookManager if (!(new \ReflectionMethod('FOG\Event', 'onEvent'))->isPublic()) { $fails[] = 'Event::onEvent() is no longer the public default dispatch target'; } +// Invoked on an EVENT, not a hook. It used to be handed $hook, which was +// only possible while Hook extended Event; #1203 separated them, and a +// ReflectionMethod refuses an object that is not an instance of the class +// declaring the method. The assertion is unchanged -- Event::onEvent()'s own +// body must write nothing -- and reflection on the declaring class still runs +// that body rather than CharEvent's override. ob_start(); -(new \ReflectionMethod('FOG\Event', 'onEvent'))->invoke($hook, 'CHAR_PRINTED', []); +(new \ReflectionMethod('FOG\Event', 'onEvent'))->invoke($event, 'CHAR_PRINTED', []); $printed = ob_get_clean(); if ('' !== $printed) { $fails[] = 'Event::onEvent() writes to the response by default: '