Add lifecycle hooks#37
Conversation
|
Warning Review limit reached
More reviews will be available in 36 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis pull request implements a complete lifecycle hooks system enabling plugins to observe and react to build and render phases. The system comprises core dispatcher infrastructure, immutable event objects carrying build context and entry data, integration through the build command and entry renderer, and comprehensive tests demonstrating both the dispatcher's basic mechanics and end-to-end hook dispatch during actual builds. ChangesLifecycle Hooks System
Sequence DiagramsequenceDiagram
participant BuildCommand
participant HookDispatcher
participant Listeners
participant ParallelEntryWriter
participant EntryRenderer
BuildCommand->>BuildCommand: createBuildContext()
BuildCommand->>HookDispatcher: dispatch(BuildStartedEvent)
HookDispatcher->>Listeners: invoke listener callbacks
Listeners-->>HookDispatcher: listeners observe build context
BuildCommand->>ParallelEntryWriter: construct with HookDispatcher
ParallelEntryWriter->>EntryRenderer: construct with HookDispatcher
loop for each entry
EntryRenderer->>HookDispatcher: dispatch(RenderStartedEvent)
HookDispatcher->>Listeners: invoke listener callbacks
EntryRenderer->>EntryRenderer: render entry
EntryRenderer->>HookDispatcher: dispatch(RenderFinishedEvent)
HookDispatcher->>Listeners: invoke listener callbacks
Listeners->>HookDispatcher: setHtml() modifies final output
end
BuildCommand->>HookDispatcher: dispatch(BuildFinishedEvent)
HookDispatcher->>Listeners: invoke listener callbacks
Listeners-->>HookDispatcher: listeners observe final state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request introduces a lightweight lifecycle hooks API (HookDispatcher + event objects) to enable plugin integrations to observe build and render phases, and wires those hooks into the build command and entry rendering pipeline. It also documents the new extension point and adds unit/integration coverage plus a micro-benchmark for dispatch overhead.
Changes:
- Add hook core types (
HookDispatcher,HookEventInterface,HookInterface) and lifecycle event classes for build and render phases. - Dispatch lifecycle events from
BuildCommandandEntryRenderer(including support for render HTML mutation viaRenderFinishedEvent::setHtml()). - Add tests/benchmarks and update documentation/roadmap to reflect the new hook surface.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Hook/HookDispatcherTest.php | Adds unit coverage for listener ordering, interface listeners, and no-listener behavior. |
| tests/Unit/Console/BuildCommandTest.php | Adds an integration-style test asserting build lifecycle events are dispatched. |
| tests/Unit/Build/EntryRendererTest.php | Adds coverage for render hooks observing and mutating final HTML. |
| src/Hook/HookEventInterface.php | Defines the base hook event contract. |
| src/Hook/HookInterface.php | Defines a reusable listener interface for DI-friendly hooks. |
| src/Hook/HookDispatcher.php | Implements listener registration and dispatch for closures and HookInterface listeners. |
| src/Hook/BuildContext.php | Introduces a context object exposing build paths and build-mode flags. |
| src/Hook/BuildStartedEvent.php | Adds the build-start event payload (context + parsed metadata). |
| src/Hook/BuildFinishedEvent.php | Adds the build-finished event payload (context + site config). |
| src/Hook/RenderStartedEvent.php | Adds a render-start event payload (site config + entry + permalink). |
| src/Hook/RenderFinishedEvent.php | Adds a render-finished event payload with mutable HTML. |
| src/Console/BuildCommand.php | Wires build started/finished dispatch and passes dispatcher into entry writing. |
| src/Build/ParallelEntryWriter.php | Threads the dispatcher into EntryRenderer for both single and forked workers. |
| src/Build/EntryRenderer.php | Dispatches render started/finished hooks and allows final HTML mutation. |
| config/common/di/content-pipeline.php | Injects a shared HookDispatcher into BuildCommand via DI. |
| docs/plugins.md | Documents lifecycle hooks, events, and DI configuration examples. |
| docs/engine.md | Explains how hooks differ from processors and where they fit in the engine. |
| docs/architecture.md | Adds hooks as an architectural extension point reference. |
| roadmap.md | Marks the hooks/events roadmap item as complete. |
| benchmarks/HookDispatcherBench.php | Adds benchmarks for empty and single-listener dispatch paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $this->hookDispatcher->dispatch(new BuildStartedEvent($buildContext, $siteConfig, $navigation, $collections, $authors)); | ||
|
|
| $this->hookDispatcher->dispatch(new BuildFinishedEvent($buildContext, $siteConfig)); | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Build/ParallelEntryWriter.php (1)
100-139:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftParallel build dispatches render hooks only in forked workers; in-memory listener state won’t reach the parent.
In
src/Build/ParallelEntryWriter.php::writeParallel(), eachpcntl_fork()child builds its ownEntryRendererand dispatchesRenderStartedEvent/RenderFinishedEvent; the child thenexit(0)s while the parent onlypcntl_waitpid()s. Any listener that aggregates cross-entry data by mutating in-memory state (counters, collected permalinks, indexes, object mutations, etc.) will do so in the child process and that state is discarded.RenderFinishedEvent::setHtml()-based HTML mutation still works becauseEntryRenderer::dispatchRenderFinished()returns$event->html()and the child writes the rendered file.
writeEntries()(workers==1) dispatches hooks in-process, so plugin behavior silently diverges between single-worker and parallel builds.Document this constraint for hook authors and/or enforce a “HTML-only mutation” contract (or move aggregation to a parent-side mechanism).
🧹 Nitpick comments (1)
src/Console/BuildCommand.php (1)
351-798: ⚖️ Poor tradeoff
BuildFinishedEventis not guaranteed to fire if the build throws.
BuildStartedEventis dispatched at Line 351, butBuildFinishedEventat Line 798 is only reached on the success path. Any exception in between (e.g. theRuntimeExceptiondirectory failures, writer failures) leaves listeners with an unpaired start event and no chance to release/finalize resources. Consider atry { ... } finally { dispatch(BuildFinishedEvent) }around the build body, or document that finish is best-effort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Console/BuildCommand.php` around lines 351 - 798, The build dispatch pattern currently sends BuildStartedEvent but only dispatches BuildFinishedEvent on the success path; wrap the entire build body (the code between the BuildStartedEvent dispatch and the BuildFinishedEvent dispatch) in a try/finally so BuildFinishedEvent is always dispatched even if exceptions occur; locate the dispatch calls for BuildStartedEvent and BuildFinishedEvent in BuildCommand (around the parsing/writing logic) and move the bulk of the work into the try block and put $this->hookDispatcher->dispatch(new BuildFinishedEvent(...)) in the finally block, ensuring any needed variables (e.g. $buildContext, $siteConfig) are available for the finally scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Unit/Build/EntryRendererTest.php`:
- Around line 406-436: The test only exercises a cold render; add a second
render call to exercise the cached path and verify hooks still run and mutate
cached HTML. After the existing $html = $renderer->render(...), call $cachedHtml
= $renderer->render($this->createSiteConfig(), $entry, '/blog/hooked-post/');
then assertStringContainsString('<span id="hooked"></span></body>', $cachedHtml)
(and optionally assert events still contains 'render.started:Hooked Post' or
other expected hook side-effects). This ensures EntryRenderer::render and its
dispatchRenderFinished behavior (and the RenderFinishedEvent mutation) are
covered for cache hits.
In `@tests/Unit/Hook/HookDispatcherTest.php`:
- Around line 36-54: Add a unit test to HookDispatcherTest that verifies
non-Closure callables passed to HookDispatcher::listen are accepted and invoked:
create and register (a) an array-callable (e.g., [new SomeListener, 'handle'])
and (b) an invokable object, call HookDispatcher::dispatch with a TestHookEvent
for their event name, and assert they were invoked (check listener call records)
and that HookDispatcher::hasListeners returns true for that event; place the
test alongside testDispatchesHookInterfaceListeners and
testIgnoresEventsWithoutListeners and reference HookDispatcher::listen,
HookDispatcher::dispatch and HookDispatcher::hasListeners when locating code to
exercise.
---
Nitpick comments:
In `@src/Console/BuildCommand.php`:
- Around line 351-798: The build dispatch pattern currently sends
BuildStartedEvent but only dispatches BuildFinishedEvent on the success path;
wrap the entire build body (the code between the BuildStartedEvent dispatch and
the BuildFinishedEvent dispatch) in a try/finally so BuildFinishedEvent is
always dispatched even if exceptions occur; locate the dispatch calls for
BuildStartedEvent and BuildFinishedEvent in BuildCommand (around the
parsing/writing logic) and move the bulk of the work into the try block and put
$this->hookDispatcher->dispatch(new BuildFinishedEvent(...)) in the finally
block, ensuring any needed variables (e.g. $buildContext, $siteConfig) are
available for the finally scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e568e6a5-7622-4fd2-bcd7-187da210246b
📒 Files selected for processing (20)
benchmarks/HookDispatcherBench.phpconfig/common/di/content-pipeline.phpdocs/architecture.mddocs/engine.mddocs/plugins.mdroadmap.mdsrc/Build/EntryRenderer.phpsrc/Build/ParallelEntryWriter.phpsrc/Console/BuildCommand.phpsrc/Hook/BuildContext.phpsrc/Hook/BuildFinishedEvent.phpsrc/Hook/BuildStartedEvent.phpsrc/Hook/HookDispatcher.phpsrc/Hook/HookEventInterface.phpsrc/Hook/HookInterface.phpsrc/Hook/RenderFinishedEvent.phpsrc/Hook/RenderStartedEvent.phptests/Unit/Build/EntryRendererTest.phptests/Unit/Console/BuildCommandTest.phptests/Unit/Hook/HookDispatcherTest.php
| public function testDispatchesHookInterfaceListeners(): void | ||
| { | ||
| $event = new TestHookEvent('test.event'); | ||
| $listener = new TestHookListener(); | ||
| $dispatcher = new HookDispatcher(); | ||
| $dispatcher->listen('test.event', $listener); | ||
|
|
||
| $dispatcher->dispatch($event); | ||
|
|
||
| assertSame(['test.event'], $listener->calls); | ||
| } | ||
|
|
||
| public function testIgnoresEventsWithoutListeners(): void | ||
| { | ||
| $dispatcher = new HookDispatcher(); | ||
|
|
||
| assertSame(false, $dispatcher->hasListeners('missing.event')); | ||
| $dispatcher->dispatch(new TestHookEvent('missing.event')); | ||
| } |
There was a problem hiding this comment.
Add a test for non-Closure callables passed to listen().
src/Hook/HookDispatcher.php:36-43 has a separate Closure::fromCallable() branch, but this suite only exercises raw closures and HookInterface instances. A regression in array-callable or object-callable registration would currently slip through.
Suggested test shape
+ public function testDispatchesCallableListeners(): void
+ {
+ $event = new TestHookEvent('test.event');
+ $listener = new class {
+ /** `@var` list<string> */
+ public array $calls = [];
+
+ public function onEvent(HookEventInterface $event): void
+ {
+ $this->calls[] = $event->name();
+ }
+ };
+
+ $dispatcher = new HookDispatcher();
+ $dispatcher->listen('test.event', [$listener, 'onEvent']);
+ $dispatcher->dispatch($event);
+
+ assertSame(['test.event'], $listener->calls);
+ }As per coding guidelines, **/*Test.php: For each piece of code add a test using phpunit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testDispatchesHookInterfaceListeners(): void | |
| { | |
| $event = new TestHookEvent('test.event'); | |
| $listener = new TestHookListener(); | |
| $dispatcher = new HookDispatcher(); | |
| $dispatcher->listen('test.event', $listener); | |
| $dispatcher->dispatch($event); | |
| assertSame(['test.event'], $listener->calls); | |
| } | |
| public function testIgnoresEventsWithoutListeners(): void | |
| { | |
| $dispatcher = new HookDispatcher(); | |
| assertSame(false, $dispatcher->hasListeners('missing.event')); | |
| $dispatcher->dispatch(new TestHookEvent('missing.event')); | |
| } | |
| public function testDispatchesHookInterfaceListeners(): void | |
| { | |
| $event = new TestHookEvent('test.event'); | |
| $listener = new TestHookListener(); | |
| $dispatcher = new HookDispatcher(); | |
| $dispatcher->listen('test.event', $listener); | |
| $dispatcher->dispatch($event); | |
| assertSame(['test.event'], $listener->calls); | |
| } | |
| public function testIgnoresEventsWithoutListeners(): void | |
| { | |
| $dispatcher = new HookDispatcher(); | |
| assertSame(false, $dispatcher->hasListeners('missing.event')); | |
| $dispatcher->dispatch(new TestHookEvent('missing.event')); | |
| } | |
| public function testDispatchesCallableListeners(): void | |
| { | |
| $event = new TestHookEvent('test.event'); | |
| $listener = new class { | |
| /** `@var` list<string> */ | |
| public array $calls = []; | |
| public function onEvent(HookEventInterface $event): void | |
| { | |
| $this->calls[] = $event->name(); | |
| } | |
| }; | |
| $dispatcher = new HookDispatcher(); | |
| $dispatcher->listen('test.event', [$listener, 'onEvent']); | |
| $dispatcher->dispatch($event); | |
| assertSame(['test.event'], $listener->calls); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Unit/Hook/HookDispatcherTest.php` around lines 36 - 54, Add a unit test
to HookDispatcherTest that verifies non-Closure callables passed to
HookDispatcher::listen are accepted and invoked: create and register (a) an
array-callable (e.g., [new SomeListener, 'handle']) and (b) an invokable object,
call HookDispatcher::dispatch with a TestHookEvent for their event name, and
assert they were invoked (check listener call records) and that
HookDispatcher::hasListeners returns true for that event; place the test
alongside testDispatchesHookInterfaceListeners and
testIgnoresEventsWithoutListeners and reference HookDispatcher::listen,
HookDispatcher::dispatch and HookDispatcher::hasListeners when locating code to
exercise.
Summary
Events
Verification
Performance
EventDispatcherBench:
The previous custom dispatcher measured about ~0.058us for empty dispatch and ~0.112us for one listener, so Yii's dispatcher is slower at the micro level. Whole-build impact was not significant: SmallSiteBuildBench comparison against the pre-hook baseline showed all six measured scenarios between -0.95% and -3.76%.
Notes