Repository navigation
fix: resolve named middleware lazily so registration order does not drop it - #334
Merged
Merged
Conversation
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.
Problem
Named middleware is looked up when the route is declared (
Router::mapHandler()doesstatic::$namedMiddleware[$name] ?? null). IfregisterMiddleware()/auth()->middleware()runs after the routes that reference it, the lookup returnsnull, the route is stored with no middleware at all, and there is no warning. The handler then runs unprotected.The array form (
'middleware' => ['auth.required']) is worse: it stores anullhandler andinvoke()dies withTypeError: stripos(): Argument #1 ($haystack) must be of type string, null given.Reproduced on v5.0 and v3.12. Surfaced while triaging #327 — not the same bug as #327 (that one is already fixed on v5.x with tests), but it is the kind of ordering mistake that produces "auth works on some routes and not others" reports.
Fix
mapHandler()now goes through a newresolveMiddleware():RuntimeException("Middleware named 'x' not found…")instead of letting the handler run. Fail-closed on purpose: a missing auth middleware should be a 500, not an open route. Happy to downgrade totrigger_errorif you'd rather matchinvoke()'s existing behaviour.Behaviour for middleware registered before routes is unchanged (same callable, same params forwarded).
Tests
Two new tests in
tests/middleware.test.php:named middleware registered after the routes still runs— string form, array form and group form, all registered after the routes. Fails onv5.x(middleware never runs / TypeError), passes here.a route using an unregistered named middleware fails closed— asserts the exception and that the handler did not run.alchemy test: 63 passed (89 assertions).alchemy lint: clean.