fix(modules): read a Boot file's namespace instead of guessing it from the path - #8
Conversation
…m the path
ModuleScanner derived a class name from the directory a Boot.php sat in — /Core
meant Core\, /Mod meant Mod\. That is a convention, not a fact. It held for an
application laid out the way the scaffolding lays one out, and broke everywhere
else, starting with this package: src/Mod/Trees/Boot.php declares Core\Mod\Trees
and the rule produced Mod\Trees\Boot.
The failure was not a miss. In an application that owns a module of the same
name — host.uk.com has an app/Mod/Trees — the wrong name RESOLVES, so the
scanner wired the consumer's unrelated class for a Boot file it had read out of
vendor. Verified against that application before the change: Core\Mod\Trees\Boot
exists=yes wired=NO, Mod\Trees\Boot exists=yes wired=yes. Nothing failed. The
wrong code ran, and this package's own Trees module had never run at all.
So the name is now read from the file's own namespace declaration, which is the
only thing about a file that is not a guess. The path convention survives only
for a Boot.php that declares no namespace at all.
Fixing that immediately proved the point twice over. Core\Mod\Trees\Boot began
wiring for the first time, its onWebRoutes registers a Livewire component, and
three route tests started failing with BindingResolutionException on
livewire.finder — because processLivewire guarded on class_exists(Livewire::class),
which is true the moment the package is in vendor/ and says nothing about
whether its provider has booted. An application shipping Livewire without
booting it got an exception where it should have got a no-op, the way
processViews already skips a view path that is not there. That guard now asks
the container.
ModuleRegistry::registerClass() is the second half. Scanning cannot find vendor
packages, and cannot be made to: php-uptelligence puts Core\Mod\Uptelligence at
its package root, php-commerce keeps Core\Service\Commerce under Service/,
php-admin has Core\Mod\Hub under src/Mod/Hub. No directory convention describes
all of those. But every one of them is already a ServiceProvider Laravel has
constructed, so the name is simply available — static::class, no derivation:
public function register(): void
{
$this->app->make(ModuleRegistry::class)->registerClass(static::class);
}
Idempotent, so a package that is both scanned and self-registering does not run
its handlers twice. Documented in CLAUDE.md as the vendor registration path,
naming the trap it closes: a $listens array on a class nothing scans is dead
code that reads as live.
Deliberately not done: path-based vendor scanning. There is no convention to
fit, and a scanner that guesses wrong fails silently, which is the defect above
one layer down.
./vendor/bin/pest --testsuite=Feature,Unit 261 -> 267 passed, 0 failed
./vendor/bin/pest --testsuite=Module 448 failed / 308 passed, unchanged
vendor/bin/pint --test pass
vendor/bin/phpstan analyse no errors
vendor/bin/psalm no errors
The six new tests are in tests/Feature deliberately, not src/**/Tests: the
Module suite is allow_failure in CI by design, and a regression pin belongs in
the job that can stop a merge.
Co-Authored-By: Virgil <virgil@lethean.io>
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesThe module system now reads declared namespaces during scanning and supports explicit Boot class registration. Registration is idempotent, classes without listeners are ignored, and Livewire processing requires an available container binding. Documentation describes these behaviours. ChangesModule discovery and registration
Sequence Diagram(s)sequenceDiagram
participant ServiceProvider
participant ModuleRegistry
participant BootClass
participant LaravelEventDispatcher
ServiceProvider->>ModuleRegistry: registerClass(Boot class)
ModuleRegistry->>BootClass: read declared listeners
ModuleRegistry->>LaravelEventDispatcher: register LazyModuleListener
LaravelEventDispatcher->>BootClass: dispatch registered event
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Core/ModuleScanner.php (1)
227-229: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSupport bracketed namespace declarations.
namespaceFromSource()accepts only statement namespaces likenamespace Core\Mod\Foo;, sonamespace Core\Mod\Foo { ... }falls back to path-derived names and can resolve the wrong class.Use a parser that recognises the opening brace as the namespace terminator, and add a fixture for bracketed syntax.
🤖 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/Core/ModuleScanner.php` around lines 227 - 229, Update namespaceFromSource() to recognize bracketed namespace declarations by treating either a semicolon or the opening brace as the namespace terminator, while preserving support for statement-style namespaces. Add a fixture covering bracketed syntax and verify it resolves the declared namespace instead of falling back to the path-derived name.CLAUDE.md (1)
112-115: 🩺 Stability & Availability | 🔵 TrivialEnsure external modules do not resolve
ModuleRegistrybeforeLifecycleEventProviderruns.
LifecycleEventProviderbindsModuleRegistryfrom itsregister()method, while this example resolves it from another provider’sregister()method. This is safe while only the framework’s configured providers exist becauseLifecycleEventProvideris first there, but vendor packages using this example still depend on the consuming application’s provider order. Defer self-registration to a later hook or document this ordering requirement.🤖 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 `@CLAUDE.md` around lines 112 - 115, Update the example provider’s register() method so it does not resolve ModuleRegistry before LifecycleEventProvider has run; defer registerClass(static::class) to a later lifecycle hook, or explicitly document the required provider ordering for external modules.Source: MCP tools
🤖 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 `@CLAUDE.md`:
- Around line 99-101: Update the ModuleScanner documentation to state that
vendor paths are not scanned by default but are scanned when included in
configured core.module_paths. Replace the inaccurate claim that class names are
always derived from paths with the behavior that Boot.php namespaces are read
first and path inference is only a fallback.
In `@src/Core/LifecycleEventProvider.php`:
- Around line 278-293: Update the documentation immediately above
processLivewire to state that app()->bound('livewire') only confirms the
registration path is available, not that Livewire has completed booting. Keep
the explanation generic across Livewire 3 and 4, avoiding claims tied
specifically to livewire.finder or ComponentRegistry, and preserve the
documented no-op behavior when Livewire is unavailable.
In `@src/Core/ModuleRegistry.php`:
- Around line 265-274: Update src/Core/ModuleRegistry.php lines 265-274 in
registerClass() so scanned mappings are merged with existing mappings rather
than replacing direct registrations, skipping any class already registered for
the event. Add the regression test in tests/Feature/ModuleRegistryTest.php lines
180-195 covering registerClass() followed by register() with the displaced
fixture, and assert the lifecycle handler runs only once.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 112-115: Update the example provider’s register() method so it
does not resolve ModuleRegistry before LifecycleEventProvider has run; defer
registerClass(static::class) to a later lifecycle hook, or explicitly document
the required provider ordering for external modules.
In `@src/Core/ModuleScanner.php`:
- Around line 227-229: Update namespaceFromSource() to recognize bracketed
namespace declarations by treating either a semicolon or the opening brace as
the namespace terminator, while preserving support for statement-style
namespaces. Add a fixture covering bracketed syntax and verify it resolves the
declared namespace instead of falling back to the path-derived name.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af154b7f-73f0-4204-8ef1-57d3e08fc22d
📒 Files selected for processing (7)
CLAUDE.mdsrc/Core/LifecycleEventProvider.phpsrc/Core/ModuleRegistry.phpsrc/Core/ModuleScanner.phptests/Feature/ModuleRegistryTest.phptests/Feature/ModuleScannerTest.phptests/Fixtures/Mod/Displaced/Boot.php
Both are accuracy, and the first is a mistake worth naming: I wrote the case for registerClass() using the OLD scanner's limitation, in the same change that removed it. The text said scanning cannot work for vendor packages because it derives class names from paths — but it no longer does. A configured vendor path now resolves correctly, whatever the package's layout. So the honest statement is the one about who carries the burden, not about what is possible. A vendor path CAN be scanned by adding it to core.module_paths; registerClass() is preferred because it asks nothing of the consumer. A package that is installed and not configured looks installed and does nothing, which is the same silent-absence failure one configuration step removed. Second: the processLivewire docblock named livewire.finder, which is a binding internal to one Livewire version. The check is on the facade's own binding precisely so it survives Livewire moving its internals around, and the comment now says that — and says what the check does and does not prove: the registration path is reachable, not that Livewire has finished booting. Doc-only. No behaviour change. vendor/bin/pint --test pass ./vendor/bin/pest --testsuite=Feature,Unit 267 passed, 0 failed Co-Authored-By: Virgil <virgil@lethean.io>
Correction: my Psalm claim was wrongThe commit message and PR body above say I read Psalm's summary tail, which ends with a coverage percentage, and took it for a pass. Psalm exits 2. What is actually true is the delta, which I have now measured properly on both sides:
Identical. The single error is Same correction applies to the PHP 8.4 matrix leg: red on The honest gate summary for this PR: That leaves this repo's |
Reshaping one sentence in the description aboveThe body says the 36 dead
36 handlers, 14 packages, none of them in this repo. The fix per package is one line in Measured in host.uk.com, which is the only consumer: none of these is currently depended on, because the application re-implements the same ground in its own |
|
@coderabbitai review |
|
…ater scan
CodeRabbit found a real bug in the registerClass() I added, and it was mine.
register() did `$this->mappings = $this->scanner->scan($paths)` — assignment, not
merge. So a package that called registerClass() from its own provider had that
record thrown away the moment the framework's scan ran, which meant the
idempotency guard inside registerClass() had nothing left to see. If the scan
also found the class, it wired a second listener and the handler ran twice on
every event.
The order is not hypothetical. A package provider's register() runs whenever
Laravel gets to it, which may be before LifecycleEventProvider's — so the
sequence that breaks is the ordinary one, not a corner.
Proven before fixing, with the fixture registering both ways:
assertCount(1, $displaced)
Failed asserting that actual size 2 matches expected size 1
register() now merges into what is already there and skips a class already
registered for that event — the same guard addPaths() has always had, which is
where I should have looked before writing a second registration path.
./vendor/bin/pest --testsuite=Feature,Unit 267 -> 268 passed, 0 failed
./vendor/bin/pest --testsuite=Module 448 failed / 308 passed, unchanged
vendor/bin/pint --test pass
vendor/bin/phpstan analyse no errors
Co-Authored-By: Virgil <virgil@lethean.io>
|
Fixed in ce4d8bb — and this one was a real bug, not a doc nit. Thank you.
The order that breaks is the ordinary one: a package provider's Proven before fixing, exactly as you suggested testing it:
Both earlier findings also addressed in db5a6e2 (the CLAUDE.md vendor-scanning description, and the version-specific |
|
@coderabbitai review |
|
1 participant
ModuleScannerderived a class name from the directory aBoot.phpsat in —/CoremeantCore\,/ModmeantMod\. That is a convention, not a fact. It held for an application laid out the way the scaffolding lays one out, and broke everywhere else — starting with this package.src/Mod/Trees/Boot.phpdeclaresCore\Mod\Trees. The rule producedMod\Trees\Boot.The failure was not a miss
In an application that owns a module of the same name — host.uk.com has an
app/Mod/Trees— the wrong name resolves. The scanner then wired the consumer's unrelated class for a Boot file it had read out ofvendor/. Measured against that application before the change:Nothing failed. The wrong code ran, and this package's own Trees module had never run at all.
The class name is now read from the file's own
namespacedeclaration — the only thing about a file that is not a guess. The path convention survives only for aBoot.phpthat declares no namespace.Fixing it proved the point twice
Core\Mod\Trees\Bootbegan wiring for the first time. ItsonWebRoutesregisters a Livewire component, and three route tests immediately failed withBindingResolutionException: Target class [livewire.finder] does not exist.processLivewireguarded onclass_exists(Livewire::class)— true the moment the package is invendor/, and silent on whether its provider has booted. An application shipping Livewire without booting it got an exception where it should have got a no-op, the wayprocessViewsalready skips a view path that is not there. That guard now asks the container.ModuleRegistry::registerClass()Scanning cannot find vendor packages, and cannot be made to:
Core\Mod\UptelligenceCore\Service\CommerceService/Core\Mod\Hubsrc/Mod/HubCore\Mod\Agentic\Mod\Apiphp/Mod/ApiNo directory convention describes all of those. But every one is already a ServiceProvider Laravel has constructed, so the name is simply available:
static::class, no derivation. Idempotent, so a package that is both scanned and self-registering does not run its handlers twice. Documented inCLAUDE.mdas the vendor registration path, naming the trap it closes: a$listensarray on a class nothing scans is dead code that reads as live.Deliberately not done
Path-based vendor scanning. There is no convention to fit, and a scanner that guesses wrong fails silently — the defect above, one layer down.
The 36 dead
$listensdeclarations across 14 vendor packages are left in place; they convert toregisterClass()per package as consumers need them.Receipts
The six new tests are in
tests/Featuredeliberately, notsrc/**/Tests: the Module suite isallow_failurein CI by design, and a regression pin belongs in the job that can stop a merge.Note for maintainers
origin(GitHub) andgitlab(git.lthn.sh) have diverged — GitHub main is 5 commits ahead, GitLab has nothing GitHub lacks. This branch is cut from GitHub main. The mirror needs a catch-up push.🤖 Generated with Claude Code
Co-Authored-By: Virgil virgil@lethean.io
Summary by CodeRabbit
New Features
Bug Fixes
Documentation