A hook is not a kind of event, so stop saying it is - #1232
Merged
Conversation
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 <noreply@anthropic.com>
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
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.
Closes #1203.
Hook extends Eventmade$listener instanceof Event— the only type checkseparating the two — true for every hook in existence. #1194 closed the defect
that fell out of it with a refusal in
acceptListener(); it did not fix themodelling that made the wrong registration look reasonable.
The change
Hook extends FOGBase. The boilerplate the two genuinely share —$name,$description,$active, the three log settings,log()and the constructor —moves to a
Listenertrait that both use, so nothing a hook relied on went awaywith the parent. What did go away is
run()andonEvent(): both are theevent dispatch surface, both were empty bodies, and no hook in core or in
fog-pluginscalls either.log()had to move rather than stay onEvent.FOGBasedeclares alog()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 wouldhave quietly called that one.
acceptListener()now tests forHookbeforeEvent. Both arms refuse a hookonce 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
extendsthis removes.Blast radius, measured before the change
instanceof Eventinpackages/eventmanager.class.phpinstanceof Eventinpackages/serviceinstanceof Eventinfog-plugins(72 hooks, 15 events)$activeparent::__construct()$logLevel/$logToFile/$logToBrowser/log()$active = falserun()/onEvent()run(); none call eitherThe residue no sweep can see is a plugin that type-hints
Eventon its ownfunction and passes its own hook to it. Nothing in core or
fog-pluginsdoesthis. Core never puts a hook in front of plugin code where that type is tested —
hook dispatch passes
[$obj, $method]callables, andacceptListener()hasrefused hooks outright since #1194 — so a plugin asking the question is either
mirroring core's pre-#1194 behaviour (already broken by #1194, not by this) or
running its own dispatch and calling
onEvent()on a hook, which #1194 emptied.Verification
Booted the app from a shadow tree against the live 1.6 database and constructed
every listener the managers load: 52 hooks, 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. Driver:scripts/background_scripts/boot_hooks_1203.php.tests/hook-event-contract.test.phpgains three guards, each mutation-verified —reverting the
extendsfails the first, droppinguse Listenerfails the othertwo. 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
itself is unchanged. Full suite: 76 passed, 0 failed.
Not in scope
HookManager extends EventManageris the same shape one level up and isdeliberately untouched —
docs/hook-event-plan.md§6 records why.🤖 Generated with Claude Code
https://claude.ai/code/session_01GN7hADN5QLzoxXWDA6xeUk