Stop the error handlers going fatal on a non-string event name - #1200
Merged
Conversation
F-13 fixed one half of this: register()'s catch interpolated $listener[0], so a listener that was an object and not an array raised "Cannot use object of type X as array" from inside the handler. That is an \Error, catch (\Exception) does not catch it, and registration runs in a hook constructor during LoadGlobals -- HTTP 500 with an empty body on every entry point. The other half was left live. Both register() and notify() throw when $event is not a string, and both then render that same $event with %s. On an object with no __toString that is the same \Error, from the same handler, for the one input the guard exists to reject. An array name only warns; an object is fatal. _describeEvent() renders it the way _describeListener() renders a listener. Gated in tests/hook-event-contract.test.php; reverting either call site fails the suite. Recorded as F-27. Co-Authored-By: Claude <noreply@anthropic.com>
mastacontrola
added a commit
that referenced
this pull request
Aug 19, 2026
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1194, which fixed one half of this and left the other half live.
The defect
EventManager::register()andEventManager::notify()both start by rejecting a$eventthat is not a string:and both catches then render that same
$eventwith%s:%son an object with no__toStringis an\Error, andcatch (\Exception)does not catch an\Error:So the handler goes fatal on precisely the input the guard exists to reject.
register()runs from hook constructors duringLoadGlobals, 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$eventfails the suite: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-branchand is fixed there in a companion PR.🤖 Generated with Claude Code
https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd