Fix the hook and event dispatch contract by mastacontrola · Pull Request #1194 · FOGProject/fogproject

mastacontrola · 2026-08-19T02:01:38Z

The hook system is the plugin ABI. A scan of eventmanager, hookmanager,
event and hook found thirteen defects; this fixes them, pins the behaviour
first so every change is deliberate, and records the five that were decisions
in docs/adr/0017-hook-dispatch-contract.md.

The one that matters

register()'s error handler was itself fatal. It interpolated $listener[0]
inside the catch meant to swallow a bad listener, so a Closure — or any
non-array object — raised Error: Cannot use object of type X as array, which
catch (\Exception) does not catch. Registration runs in a hook constructor
during LoadGlobals, so it escaped base.inc.php: HTTP 500, zero-byte body,
every entry point, until the file was deleted from disk.

And docs/plugin-development.md documented that exact shape three times, in
the §7 examples for the Phase 2 authentication seams. Nothing in fog-plugins
follows the guide — the real OIDC plugin uses registerInstalled() — which is
why it survived. Anyone writing a third-party identity provider from the docs
took their server down.

Also found

No core hook or event has ever loaded on any FOG server. All eleven files
in lib/hooks/lib/events are $active = false and none matched the
activation regex. Every live listener on every install comes from a plugin.
Activation was a regex over source text. \s? is zero-or-one, the match
was case-sensitive, and it could not tell a comment from code, so
public $active = true; with two spaces was inactive, TRUE was inactive,
and $active = false; with = true; in the comment above it was active.
$active was decorative for every plugin hook. Dispatch force-set
active = true for any listener whose file path contained the substring
plugins, so a plugin could not turn one of its own hooks off. Verified by
experiment, not inferred.
HookManager::notify() returned true having invoked nothing — it
iterates listeners as objects while HookManager stores arrays.
Hook extends Event, so instanceof Event accepted a hook as an event
listener, and Event::onEvent()'s default printed the event name into the
response.
notify() treated "nobody listening" as an exception and logged it —
which writes a history row once an admin is signed in — on every host
checkin, for an event nothing has ever listened to. It also ran an uncached
exists() SELECT per call: the The Scheduled Tasks feature is not functioning in version 1.6.0-beta.2167 #707 shape processEvent() was fixed for.
load() told the two managers apart by an ordering accident, HookManager
satisfying both instanceof checks.
register() switched on self::shortName($this) with a throwing default
— the shape whose comment records it taking every hook in the system down
once already, during the namespace migration.

What changes for plugin authors

register() now accepts a Closure as well as [Hook, 'method']. Both have
an owner — for a closure, whatever $this it was written inside, recovered
with ReflectionFunction::getClosureThis() — and the owner carries $active,
so admitting closures needed no new activation rule. The documented seam
examples work as written.

$active decides, wherever the file lives.

Blast radius: one thing can break — a third-party plugin hook that declares
$active = false and relied on the force-activation to run anyway. All 87
bundled hook and event files set it true, and Event::$active defaults to
true, so a hook that omits the property is unaffected. Only an explicit
false is, and writing false while expecting the hook to run is not a
coherent intent.

Verification

Twelve commits, tree green after each (sh tests/run-all.sh, 66 passed).

tests/hook-event-contract.test.php pins the whole contract — 684 lines, no
database — and was written first, asserting the broken behaviour, so every
fix shows up as a case somebody had to rewrite. Each fix is mutation-verified:
widening the activation regex, reordering the instanceof blocks, restoring
the force-activation, making the catch safe, emptying Event::onEvent(),
dropping the Closure branch, ignoring a closure's owner, inlining the payload
merge, restoring the notify() throw, and dropping the name cache are each
caught.

End to end against the live 1.6 lab install with six plugins enabled (ldap location ntfy oidc ou windowskey), nine authenticated pages: a closure owned
by an active plugin hook, a static closure and a [$this, 'method'] pair all
fired on every page; neither the pair nor the closure belonging to a hook
declaring $active = false fired at all.

Performance was measured before proposing anything, and the answer was to stop:
the whole subsystem is 1–2 ms of an 11–543 ms page render, and the
per-listener ReflectionClass everyone assumes is expensive totalled 14–168 µs
per request. Nothing here was done for speed. The reflection is gone anyway —
it existed only to feed the path substring — and the activation scan dropped
from 400–490 µs to 77–167 µs per request as a side effect of asking the class
instead of grepping the file.

Not in this PR

HOST_IMAGE_FAIL and HOST_IMAGEUP_COMPLETE have listeners in slack,
ntfy and pushbullet and are notified by nothing in core, so those
notifications have never fired. That is making a feature work for the first
time rather than refactoring a subsystem; it needs its own issue and test.

Making Hook extend FOGBase directly, so hooks and events are genuine peers,
is the honest modelling change and changes $obj instanceof Event for every
hook — its own issue too.

Downstream

None. No route class changes, so FogApi's hardcoded class list is unaffected.

Findings are recorded as F-11 … F-26 in docs/refactor-facts.md; the reasoning,
the alternatives and what each decision would have cost are in
docs/hook-event-plan.md.

🤖 Generated with Claude Code

https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd

The hook system is the plugin ABI: every bundled plugin, and every plugin neither of us has read, registers through EventManager::register() and is dispatched through HookManager::processEvent(). So the bar for changing it is not "the tests pass", it is "no plugin needs editing" -- and the only way to hold that bar is to write down what happens now, wrong parts included, so a behaviour change shows up as a case somebody had to edit on purpose rather than as a silent difference. Nothing changes here. The cases that pin defects say so, and say that the fix is to rewrite the case rather than to leave it green. Pinned, with the fact-ledger entries they correspond to: - registering [Hook, method] works; a pair whose first element is not a Hook, and a pair naming a method that does not exist, are both swallowed and logged, and the caller cannot tell - a Closure listener raises \Error *out of* register(), because the catch that exists to swallow the failure interpolates $listener[0] (F-13). In production that is HTTP 500 with an empty body on every entry point, and docs/plugin-development.md documents that exact shape three times (F-14) - the one diagnostic a swallowed failure produces names neither the class nor the event, thanks to a literal $s where a specifier was meant and seven arguments for six specifiers - Hook extends Event, so EventManager::register() accepts a hook as an event listener, and Event::onEvent()'s default prints into the response (F-18) - HookManager inherits notify(), which cannot read its own listener shape (F-17); pinned structurally, since notify() queries before a test can reach it - activation is a regex over source text, so it turns on whitespace and case and cannot tell a comment from code -- six variants, two of which disagree with the property in opposite directions (F-15). The pattern is read out of the shipped file rather than copied, so replacing it fails this case - load() picks its file extension by an ordering accident, HookManager satisfying both instanceof checks (F-22) - processEvent() force-activates any listener whose path contains "plugins", so a plugin hook declaring $active = false runs anyway, while the same hook off a plugin path does not (F-16) - hasListeners() is deliberately blind to active No database: the managers and fixtures are built with newInstanceWithoutConstructor() -- Event::__construct() dereferences self::$FOGUser, which no test has -- and $knownEvents is seeded by reflection so processEvent() never asks the hookevent table anything. Mutation-verified. Widening the regex's \s?, reordering the instanceof blocks, deleting the force-activation, making the catch safe, and emptying Event::onEvent() are each caught. Findings: docs/refactor-facts.md F-11..F-26. Proposal: docs/hook-event-plan.md. Co-Authored-By: Claude <noreply@anthropic.com>

register() catches a bad listener, logs it and returns -- except that the catch block interpolated $listener[0]. Hand it an object that is not an array and that line raised "Cannot use object of type X as array", an \Error, which catch (\Exception) does not catch. Registration runs inside a hook constructor during LoadGlobals, so the \Error escaped base.inc.php and the whole application answered HTTP 500 with a zero-byte body -- every entry point, every request, until the file was deleted from disk. Indistinguishable in a browser from the autoload collision at commons/init.php:242-250. An error handler must not be able to fail harder than the error it reports, so name the listener through a describer that copes with every shape: an array (name its first element), an object (name it), anything else (its type). The message itself was no use either. The format carried a literal $s where a specifier was meant, and supplied seven arguments for six specifiers, so $listener[0] -- the only field saying which class failed -- was dropped before it was ever printed. What you got was: Could not register: Error: Class must extend hook, $s: Event, X: Class and what you get now is: Could not register: Error: Class must extend hook, Event: X, Class: stdClass Names go through shortName(): this is log text, not a class reference (ADR 0013, tests/class-name-derivation.test.php). This changes behaviour only for a listener shape that takes the server down today, so the affected population is installs that are currently offline. What changes for them is that the server stays up with one hook unregistered. The characterization cases that pinned the fatal are rewritten to assert that nothing escapes, for five listener shapes, and that both the class and the event are named. Verified by reverting the fix: six cases fail. Refs F-13, F-14 in docs/refactor-facts.md. Co-Authored-By: Claude <noreply@anthropic.com>

docs/plugin-development.md has documented the closure form for the three Phase 2 authentication seams since ADR 0014: self::$HookManager->register('API_PLUGIN_ROUTES', function ($args) { ... }); register() has never accepted one. Before the previous commit it took the server down; after it, it logs and returns. Either way the documented way to contribute a route, a session-less page node or a login button did not work, and nothing in fog-plugins follows the guide -- the OIDC plugin uses registerInstalled() -- which is why nobody had noticed. Make the documentation true rather than deleting it. A hook listener is now either [Hook, method] or a Closure, and that needed no new activation rule, because both shapes have an owner and the owner is what carries $active: [$hook, 'method'] owner is $hook a closure owner is whatever $this it was written inside, which for one declared in a hook constructor is that hook -- ReflectionFunction::getClosureThis() a static closure no owner, so always active; registering it is the opt-in Anything else is still refused: a bare function name and [Class::class, 'staticMethod'] have no owner, and an owner is what a listener needs in order to have an $active at all. The array form's instanceof Hook guard is unchanged. Storage is unchanged for existing entries -- an array listener is stored as the array it is today and a closure as itself -- so nothing that already works moves. Verified that nothing outside the two manager classes reads $data, in packages/web, packages/service or fog-plugins (F-26), so the mixed array is not observable. The dispatch loop now resolves owner and callable once and calls the callable, which also fixes the argument by reference: the merged payload stays in a variable, because only a variable can bind to a listener declaring its parameter by reference. No shipped callback does, but a plugin is free to. Mutation-verified: dropping the Closure branch, ignoring the closure's owner, and inlining the merge into the call are each caught. Refs F-14, F-25, F-26 in docs/refactor-facts.md. Co-Authored-By: Claude <noreply@anthropic.com>

load() decided whether a non-plugin hook or event ran by scanning the file line by line for the literal text `$active = true;`. \s? is zero-or-one, the match was case-sensitive, and the scan could not tell a comment from code, so what actually ran turned on a file's whitespace: public $active = true; ran public $active = true; (two spaces) did NOT run public $active = true; (two after the =) did NOT run public $active=true; ran public $active = TRUE; did NOT run public $active = false; with `= true;` in a comment above it -- the obvious way to document the toggle ran Ask the class instead. _declaresActive() reads the declared default of $active through ReflectionClass::getDefaultProperties(), which is what the regex was approximating: a value assigned in a constructor is still not consulted, exactly as before. No shipped file changes verdict. All eleven core hooks and events declare `public $active = false;`, and none of the 87 bundled plugin hook and event files disagrees with the old regex either (F-12) -- though plugin files never reached this path anyway, being merged in unconditionally further down. Two things do change, both on purpose: - spacing and case stop deciding anything, which is the point; - a file declaring no $active at all now inherits Event's default of true and runs, where the regex found no literal and skipped it. Asking the class is the whole idea, and the class genuinely is active. Truthiness rather than identity, so this agrees with the check processEvent() makes at dispatch. One notion of active, read the same way at both ends. An unresolvable class name is skipped rather than reflected on: load() runs inside LoadGlobals, so a ReflectionException there is a 500 rather than one hook not starting. Mutation-verified: restoring the regex in load(), and dropping the class_exists guard, are each caught. Refs F-11, F-12, F-15 in docs/refactor-facts.md. Co-Authored-By: Claude <noreply@anthropic.com>

load() chose its file extension and directory with two sequential instanceof checks, EventManager first and HookManager second. HookManager extends EventManager, so it satisfied both, and reached .hook.php only because the second assignment overwrote the first. Reordering those two blocks would have made every HookManager load .event.php files. Nothing in PHP and nothing in the suite would have said so; the symptom would have been every hook in the system quietly not registering, which is the failure the comment above register()'s switch records happening once already during the namespace migration. A parent that has to identify its own children by instanceof, in an order that matters, is the thing to remove -- not the ordering. $fileExtension and $fileDirectory are declared on EventManager and overridden by HookManager, so each class answers for itself and load() just reads them. No behaviour change: the pair each class declares is exactly what the check order produced. Refs F-22 in docs/refactor-facts.md. Co-Authored-By: Claude <noreply@anthropic.com>

register() switched on self::shortName($this), with a case per subclass and a default arm that threw. The comment above it recorded what that shape costs: during the namespace migration every call landed on the default, so no hook and no event registered anywhere -- and because the throw is caught and logged, the application went on serving pages with every hook silently absent. A parent that enumerates its children by name has the same failure available to it every time either name changes, and it also means a subclass of either manager -- something a plugin is free to write -- registers nothing at all and is told so only in a log line. acceptListener() is the override point instead. EventManager refuses anything that is not an Event; HookManager refuses anything that is neither [Hook, method] nor a Closure. Neither knows the other exists, and a subclass inherits whichever it does not override rather than falling off the end. Also: HookManager::$data defaults to [] rather than null, so appending to it no longer relies on auto-vivification. Nothing outside the two managers reads it (F-26). No behaviour change for any valid caller. A subclass of either manager stops silently registering nothing, which is an improvement nobody can be relying on the absence of. Co-Authored-By: Claude <noreply@anthropic.com>

…wice Two problems in one method, both the same shape as bugs already fixed next door in HookManager. Nobody listening was an exception. notify() threw "Event and data are not set", caught it, logged it and returned false -- while processEvent() handles the identical condition with a bare return. It is not an error condition: of the five names core notifies, only three have a listener in any bundled plugin, and HOST_CHECKIN has never had one anywhere. So a stock server threw and logged on every host checkin, and FOGBase::log() calls logHistory(), which writes a history row once an admin is signed in. And the name bookkeeping was uncached. processEvent() keeps the hookevent name list in a static because asking per fire cost 2000 round trips on a 1000-host tasking (GH-707); notify() still ran an exists() SELECT on every call -- measured at 0.155ms of the 0.178ms a listener-less notify() costs, 87% of the work being bookkeeping for a discovery aid. Same fix, same staleness argument: nothing removes names, so at worst one redundant upsert. It also ran that query *before* the guards, so a caller passing something that was not an event name got it written to the database and was then told it was invalid. Also fixes the failure message, which carried a literal $s where a specifier was meant and so dropped the event name it was trying to report -- the same typo the register() half had. No caller can notice: the return value is unchanged (false still means nothing was notified) and no core call site reads it. Refs F-19, F-21 in docs/refactor-facts.md. Co-Authored-By: Claude <noreply@anthropic.com>

notify() is EventManager's, and it iterates listeners as objects: $element->active, then $element->onEvent(). HookManager stores listeners as [object, method] arrays and Closures. Under PHP 8 reading a property off an array is a warning that yields null, so every listener was skipped -- and the method returned TRUE, having invoked nothing. Nothing in core, packages/service or fog-plugins calls it, so the only code this reaches is third-party code whose listeners have never fired. It now says so and names processEvent(), instead of quietly reporting success. The friendlier fix -- delegate to processEvent() so the caller gets what they meant -- is deliberately not taken. The two are not two spellings of one thing: processEvent() merges an `event` key into the payload and calls a named method that can mutate its arguments through references, while notify() passes a copy to a fixed method and discards the result. Making one quietly behave as the other would blur exactly the boundary the next commit sharpens, where EventManager::register() stops accepting a Hook as an event listener. Also corrects the two existing self::log() calls in EventManager, which passed $this as $logbrow and 0 as $obj. FOGBase::log() reads neither, so this is inert today -- but it is wrong, and both sites are in a file already open. Refs F-17 in docs/refactor-facts.md. Co-Authored-By: Claude <noreply@anthropic.com>

EventManager::register() guards with `$listener instanceof Event`. Hook extends Event, so that guard -- the only type check separating the two -- accepts a hook. What follows is not theoretical. notify() then calls Event::onEvent() on it, hooks do not implement onEvent(), and the inherited default printed the event name into the response. On a page that is stray text; on a client protocol endpoint it is arbitrary output in front of a reply the fog-client parses positionally. So: refuse a Hook where an event listener is expected, and make Event::onEvent()'s default do nothing. Every bundled plugin event overrides it. lib/events/hostlist.event.php, the one core event, does not -- being inactive is the only reason the default was never reached in production. Not fixed here: making Hook extend FOGBase directly, so hooks and events are genuine peers sharing a base class rather than one being a kind of the other. That is the honest modelling change and it changes the answer to `$obj instanceof Event` for every hook in existence, which needs its own issue and its own blast-radius argument rather than riding along with a bug fix. The one-line guard closes the defect either way. Refs F-18 in docs/refactor-facts.md. Co-Authored-By: Claude <noreply@anthropic.com>

processEvent() reflected on every listener's class, took its filename, and force-set active = true whenever the path contained the substring "plugins" -- so a plugin hook declaring $active = false ran anyway. Verified by experiment, not inferred: fileitems() does filter plugin files to installed and enabled plugins before this point, but that is a different question from whether the hook's own flag is read. The reason for the net has expired. When capone was the only plugin, plugins had no hooks of their own; hooks were adopted into the plugin system by copying core's example hooks, and every core example declares $active = false. Force- truthing anything on a plugin path made a copied example work without its author noticing the flag. Plugins have shipped their own hooks for years and all 87 bundled hook and event files set $active = true, so there is nothing left to catch. It was never free either. It made $active decorative for every plugin hook, so a plugin could not turn one of its own hooks off; and being a bare stripos over the whole path, an install whose base directory contained the string would have force-activated core's hooks too. $active is an intended flag. Set it false and the hook does not run, wherever the file lives. Blast radius, stated plainly: a third-party plugin hook that declares $active = false and relied on this to run anyway stops running. Event::$active defaults to true, so a hook that simply omits the property is unaffected -- only an explicit false is, and writing false while expecting the hook to run is not a coherent intent. The per-listener ReflectionClass and getFileName() go with it; they existed only to feed the substring test. That is the whole measured reflection cost of the subsystem removed by deletion rather than by optimization. The loop also stops iterating by reference, since nothing writes through it now. docs/adr/0017 records the five decisions this work settled, four of which leave no trace in the code: the two listener shapes and how a closure gets an owner, where activation is read from, this deletion and why the net existed, that hooks and events are peers, and that notify() on a HookManager is an error. Refs F-16, F-20, F-24 in docs/refactor-facts.md. Co-Authored-By: Claude <noreply@anthropic.com>

The guide's §7 examples register closures, and until this branch register() did not accept one -- so the documented way to contribute a route, a session-less page node or a login button never worked. Now that it does, say so in §4.5 where hooks are introduced, rather than leaving the closure form to appear only in the authentication section. Also states the activation rule, which was documented nowhere: $active is inherited as true from Event, read from the declaration at load and from the property at dispatch, and a closure obeys the $active of the hook it was written inside. With a note that a hook on a plugin path used to be force-activated regardless, since anyone reading old plugin code will see hooks that set the flag and hooks that do not and wonder which mattered. And a line on the failure mode nobody expects: a registration that cannot be satisfied is logged and swallowed, so a typo in a method name costs a hook that silently never fires. Check the log before you check the event name. Refs docs/adr/0017-hook-dispatch-contract.md. Co-Authored-By: Claude <noreply@anthropic.com>

Both were written outside the tree while the work was being scoped, and both are now cited from things that are in it: docs/adr/0017 refers to F-11..F-26 by number, and nine of this branch's commit messages do the same. A reference to a file nobody else has is not a reference. docs/refactor-facts.md is the append-only ledger the modernization work keeps: one claim per entry, each with the command that proved it, and a rule that a claim you cannot write a command for belongs in a plan's INFERRED section instead. F-01..F-10 predate this branch and describe the Composer and vendor work already in the tree; F-11..F-26 are this scan. docs/hook-event-plan.md is the proposal the ADR condenses -- the defect list as found, the measurement, the alternatives, and the five decisions with what each option would have cost. Kept because the ADR states conclusions and this states the reasoning, including the parts that turned out to be wrong: the first draft leaned four times on "ADR 0013 froze the plugin ABI for all of 1.6", which is not what that ADR says and could not apply anyway to a 1.6.0 that has not been released. The two brief files that drove the work are deliberately not tracked. They are instructions to an agent, not project documentation. Co-Authored-By: Claude <noreply@anthropic.com>

`EventManager::register()` catches a bad listener, logs it and returns — except that the `catch` block interpolated `$listener[0]`. Hand it an object that is not an array and that line raises `Cannot use object of type X as array`, an `Error`, which `catch (Exception)` does not catch. Registration runs inside a hook constructor during `LoadGlobals`, so it escapes `base.inc.php`: **HTTP 500 with a zero-byte body, every entry point, every request, until the file is deleted from disk.** In a browser it is indistinguishable from an autoloader collision. An error handler must not be able to fail harder than the error it is reporting. The message it produced was no use either — the format carried a literal `$s` where a specifier was meant, and supplied seven arguments for six specifiers, so `$listener[0]`, the only field saying *which* class failed, was dropped before it was ever printed: ``` before: Could not register: Error: Class must extend hook, $s: Event, X: Class after: Could not register: Error: Class must extend hook, Event: X, Class: stdClass ``` `tests/register-failure-not-fatal.test.php` covers it — no database — and is mutation-verified: restoring `$listener[0]` fails seven cases. Suite is 11 passed, 0 failed. ## Scope Found by a scan of the whole subsystem on `working-1.6` (#1194, `docs/adr/0017-hook-dispatch-contract.md`). **This is the safety half only.** That branch also replaces the source-text activation regex with reading the property, deletes the force-activation of any listener whose file path contains the substring `plugins`, and refuses a `Hook` where an event listener is expected — all of which change *which* listeners run. Every one of those defects is present here too, but on a released line with a plugin population that is neither small nor enumerable they are a separate decision, and this PR is deliberately not it. Nothing that works today changes. The only behaviour that changes belongs to installs that are currently returning 500. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd

Follow-up to #1194, which fixed one half of this and left the other half live. ## The defect `EventManager::register()` and `EventManager::notify()` both start by rejecting a `$event` that is not a string: ```php if (!is_string($event)) { throw new Exception(_('Event must be a string')); } ``` and both catches then render that same `$event` with `%s`: ```php } catch (\Exception $e) { $string = sprintf( '%s: %s: %s, %s: %s, %s: %s', _('Could not register'), _('Error'), $e->getMessage(), _('Event'), $event, // <- here _('Class'), self::_describeListener($listener) ); ``` `%s` on an object with no `__toString` is an `\Error`, and `catch (\Exception)` does not catch an `\Error`: ``` $ php -r 'try { echo sprintf("%s", new stdClass); } catch (\Exception $e) { echo "caught\n"; } catch (\Error $e) { echo "ESCAPED: ".$e->getMessage()."\n"; }' ESCAPED: Object of class stdClass could not be converted to string ``` So the handler goes fatal on precisely the input the guard exists to reject. `register()` runs from hook constructors during `LoadGlobals`, so it escapes to the top: HTTP 500 with a zero-byte body, on every entry point, until the offending file is removed from disk. This is exactly the shape #1194 fixed for the *listener* argument (F-13) — the same handler, one argument along. Fixing the listener half left this half untouched on both branches. An **array** event name renders as `"Array"` with a warning and is survivable; an **object** is the fatal case. The existing contract test covered the array only, which is why this got through. ## The fix `_describeEvent()` renders the event name the way `_describeListener()` already renders a listener — `is_string($event) ? $event : gettype($event)` — so the log line still identifies the bad call without the handler being able to fail harder than the error it is reporting. ## Verification Two cases added to `tests/hook-event-contract.test.php`, one per catch. Mutation-verified — reverting either call site to the raw `$event` fails the suite: ``` --- register catch reverted: FAIL: 2 problem(s): - register() goes fatal reporting a non-string event name - the register failure message does not say what the event name was... --- notify catch reverted: FAIL: 2 problem(s): - notify() goes fatal reporting a non-string event name: Error - the notify failure message does not say what the event name was ``` Full suite: `66 passed, 0 failed`. Recorded as F-27 in `docs/refactor-facts.md`. ## Blast radius None for plugin authors. Nothing changes about which listeners register or run, or what they receive — only the text of a diagnostic, and whether producing it can take the request down. No route change, so no OpenAPI change. The same defect is present on `dev-branch` and is fixed there in a companion PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd

…1201) Follow-up to #1196, which was the fatal-catch fix alone. **Scoped deliberately.** Every change here is wrong today, and none of them changes which listeners register, which ones fire, or what they receive. The activation regex, the path-substring force-activation and the acceptance of a `Hook` where an event listener is expected are all still here and still untouched — those decide *which* listeners run, and on a released line with a plugin population that is neither small nor enumerable that is a separate decision, not this PR. ## 1. Both error handlers went fatal on a non-string event name #1196 fixed the listener half: the catch interpolated `$listener[0]`, so an object that was not an array raised an `Error`, which `catch (Exception)` does not catch, and registration runs in a hook constructor during `LoadGlobals` — HTTP 500 with an empty body on every entry point. The event half was left live. `register()` and `notify()` both start with: ```php if (!is_string($event)) { throw new Exception(_('Event must be a string')); } ``` and both catches then render that same `$event` with `%s`: ``` $ php -r 'try { echo sprintf("%s", new stdClass); } catch (Exception $e) { echo "caught\n"; } catch (Error $e) { echo "ESCAPED: ".$e->getMessage()."\n"; }' ESCAPED: Object of class stdClass could not be converted to string ``` Same uncaught `Error`, same handler, for the one input the guard exists to reject. An **array** name renders as `"Array"` with a warning and survives; an **object** is the fatal case — which is why the array-name test in #1196 did not catch it. ## 2. `notify()` recorded the event name before validating it The `NotifyEventManager` lookup and the `NotifyEvent` save sat *above* the `try`: ```php $exists = self::getClass('NotifyEventManager')->exists($event, '', 'name'); if (!$exists) { self::getClass('NotifyEvent')->set('name', $event)->save(); } try { if (!is_string($event)) { throw new Exception(_('Event must be a string')); } ``` So a caller passing an array or an object had that value written into the discovery table, and then rejected by the guard one line later. It was also a database round trip on **every** call — `notify()` is reached from the snapin client protocol (`snapinclient.class.php`, four call sites), from `taskqueue`, and from `user.class.php` on every failed login. That is the same mistake `processEvent()` had; this is the same fix and the same cache rationale, and the docblock points at `HookManager::$knownEvents` for it. ## 3. `load()` chose its paths with two sequential `if`s ```php if ($this instanceof self) { /* event regex, dir, offset */ } if ($this instanceof HookManager) { /* hook regex, dir, offset */ } ``` `HookManager extends EventManager`, so a HookManager satisfies **both**. The hook branch was reached only because it ran second and overwrote what the event branch had just assigned — swap the two blocks and every hook silently loads as an event and finds nothing. Now one decision, most-specific first. The generalized `sprintf`s produce byte-identical output to the strings they replace: ``` event regex=#^.+/events/.*\.event\.php$# dir=/events/ strlen=-10 hook regex=#^.+/hooks/.*\.hook\.php$# dir=/hooks/ strlen=-9 orig regex=#^.+/events/.*\.event\.php$# dir=/events/ strlen=-10 orig regex=#^.+/hooks/.*\.hook\.php$# dir=/hooks/ strlen=-9 ``` ## 4. `load()` built a lookup of every declared class and threw it away ```php $decClasses = get_declared_classes(); foreach ((array)$decClasses as $key => &$classExist) { $exists[$classExist] = 1; unset($classExist); } $exists = class_exists($className, false); // <- overwrites it, unread ``` Once per hook or event file. Removed. Also fixes the malformed `notify()` format string, which carried a literal `$s` where a specifier was meant and so dropped the failing event's name from the one diagnostic it produces. ## Verification All four gated in `tests/register-failure-not-fatal.test.php` and mutation-verified — reverting any one fails the suite: ``` --- M1 register catch reverted: register() goes fatal reporting a non-string event name --- M2 notify catch reverted: notify() goes fatal reporting a non-string event name: Error --- M3 record moved back above the guard: notify() still reaches the database for a name it has already seen --- M4 load() ordering restored: load() decides between events and hooks by statement order again --- M5 dead loop restored: load() builds a lookup of every declared class again ``` Full suite: `11 passed, 0 failed`. ## Blast radius None for plugin authors. No listener shape is newly accepted or newly refused, no listener's activation changes, and no payload changes. The only externally visible differences are that a diagnostic can no longer take the request down with it, and that the `notifyEvents` table stops collecting rows for values that were never event names. No route change, so no OpenAPI change. The companion `working-1.6` PR for defect 1 is #1200; defects 2–4 were already fixed there by #1194. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd

darksidemilk and others added 12 commits August 18, 2026 20:39

mastacontrola mentioned this pull request Aug 19, 2026

Stop register()'s error handler being fatal #1196

Merged

mastacontrola merged commit 9e83cba into working-1.6 Aug 19, 2026
3 checks passed

mastacontrola deleted the fix-hook-event-contract branch August 19, 2026 02:09

This was referenced Aug 19, 2026

Stop the error handlers going fatal on a non-string event name #1200

Merged

Fix four hook/event defects that cannot change which listener runs #1201

Merged

This was referenced Aug 19, 2026

Two notification events have never fired: HOST_IMAGE_FAIL and HOST_IMAGEUP_COMPLETE #1202

Closed

Hook extends Event, but a hook is not a kind of event #1203

Closed

A hook is not a kind of event, so stop saying it is #1232

Merged

Navigation Menu

Uh oh!

Fix the hook and event dispatch contract - #1194

Fix the hook and event dispatch contract#1194
mastacontrola merged 12 commits into
working-1.6from
fix-hook-event-contract

mastacontrola commented Aug 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

mastacontrola commented Aug 19, 2026

The one that matters

Also found

What changes for plugin authors

Verification

Not in this PR

Downstream

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants