Repository navigation
Keep a failing module from taking the application down - #326
Merged
Merged
Conversation
ModuleManager::loadEnabled() registered each enabled module's provider inside a try/catch, and the comment promised "a bad module never prevents the application from booting". It only half held. Laravel calls a provider's boot() later, while booting every provider, so an exception thrown there escapes that catch: the module that threw takes every route with it. Seen with a module built against a newer core — it called a menu helper with an argument this version does not accept. The result was a 502 on the whole installation, including the admin screen needed to disable the module again; recovery meant deleting its directory by hand. Providers are now wrapped in ModuleProviderGuard, which runs register() and boot() itself and swallows what either throws, logging it and reporting it. A module that fails simply contributes nothing — no routes, menus or listeners — and the rest of the application is untouched. The guard is registered with force: true because the container deduplicates providers by class name. Every module's guard is the same class, so without it the second module's guard would be mistaken for the first one's and that module would never load at all — a quieter bug than the one being fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QRWsLoNeVWdsSHve6vUmxs
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: Mes-Open/OpenMes/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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.
ModuleManager::loadEnabled()registers each enabled module's provider inside a try/catch, and the comment above it promises "a bad module never prevents the application from booting". It only half held.Laravel calls a provider's
boot()later, while booting every provider — long afterloadEnabled()returned. An exception thrown there escapes that catch, and the module that threw takes every route with it.How it showed up
A module built against a newer core called a menu helper with an argument this version does not accept (
Unknown named parameter $badge). The result was a 502 on the whole installation, including the admin screen needed to disable the module again. Recovery meant deleting the module's directory by hand, as root, from inside the container.The fix
Providers are wrapped in
ModuleProviderGuard, which runsregister()andboot()itself and swallows what either throws, logging it and passing it toreport(). A module that fails contributes nothing — no routes, menus or listeners — and the rest of the application is untouched.boot()is resolved through the container, the way Laravel calls it, so a provider that type-hints dependencies there still works.One trap worth naming
The guard is registered with
force: true, because the container deduplicates providers by class name. Every module's guard is the same class, so without it the second module's guard would be mistaken for the first one's and that module would never load at all.That would have been a worse bug than the one being fixed: silent, and only visible once someone installed a second module. There is a test for it.
Tests
Five new cases in
ModuleProviderGuardTest:register()does not escapeboot()does not escapeboot()at all is fineFull suite: 2952 passed, 2 failed. Both failures are in
InstallPresetTestand were verified against this branch's base withgit stash— they exist without this change.Note
While investigating this I also expected enabling a module to need a manual restart for its routes to appear. That turned out to be wrong:
ModulesController::enable()already callsclearCache(), which runsroute:clear,config:clearand hooks an Octane worker reload toRequestTerminated. No change was needed there.🤖 Generated with Claude Code
https://claude.ai/code/session_01QRWsLoNeVWdsSHve6vUmxs