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: '