Repository navigation
[v4.x] fix: group middleware not applied on dynamic routes (#327 backport) - #335
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.
Backport of the #327 fix from
v5.x(c076298, "fix: patch up middleware not running on dynamic routes within groups") tov4.x.Problem
Fixes #327 for Leaf v4 users (also reported as leafsphp/auth#32 and #316).
Group / in-route middleware on a dynamic route (e.g.
auth.requiredon/clients/{id}insideapp()->group()) is skipped whenever any catch-all middleware is registered (app()->use(), e.g. CORS):Router::handle()filtered the matched middleware withpattern === $uri || pattern === '/.*' || …. A dynamic pattern never string-equals the URI, and the global/.*entry keeps the filtered list non-empty, so the "fall back to everything" branch never kicks in. Without a global middleware the fallback hides the bug, which is why it looked intermittent.Fix
Same change as
v5.x: drop the filter and run every middlewarefindRoute()matched (it already does regex matching against the URI).Tests
Ported the three regression tests from
v5.xtests/middleware.test.php(#316 + #327):group middleware runs for dynamic routes inside the groupin-route middleware runs for dynamic routes inside a groupdynamic group middleware still runs when a catch-all middleware existsRouter::reset()only exists onv5.x, so the tests use a smallresetRouterState()helper (reflection, restores the router's static defaults) instead. No change tosrc/beyond the fix.Results (PHP 8.3, deps pinned to the v4-era majors locally):
dynamic group middleware still runs when a catch-all middleware existsfails, i.e. the test catches the bugHeads-up on CI
composer.jsonon this branch requiresleafs/http,leafs/anchorandleafs/exceptionas*, so a fresh install now resolves the v5 packages (leafs/exception5.x has noLeaf\Exception\Run) and the whole suite errors before reaching these tests. Locally I tested withcomposer update --with 'leafs/exception:<5' --with 'leafs/http:<5' --with 'leafs/anchor:<5' …. If CI is red here, that is why. Pinning those constraints is a separate change; happy to open it if you want it.