-
Notifications
You must be signed in to change notification settings - Fork 36
fix: Ported master’s route injection behavior to 0.33.x #209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 0.33.x
Are you sure you want to change the base?
Conversation
… route resource after wildcard resolution
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Http.php (1)
907-936:⚠️ Potential issue | 🟠 MajorDead code: This
elseifbranch for OPTIONS handling is unreachable.The OPTIONS handling at lines 868-903 already returns at line 902 when
$method == OPTIONS. This means theelseif (self::REQUEST_METHOD_OPTIONS == $method)branch at line 907 can never execute.This appears to be duplicate code that should be removed.
🔧 Proposed fix: Remove dead code
if (null !== $route) { return $this->execute($route, $request, $response); - } elseif (self::REQUEST_METHOD_OPTIONS == $method) { - try { - foreach ($groups as $group) { - foreach (self::$options as $option) { // Group options hooks - if (in_array($group, $option->getGroups())) { - \call_user_func_array($option->getAction(), $this->getArguments($option, [], $request->getParams())); - } - } - } - - foreach (self::$options as $option) { // Global options hooks - if (in_array('*', $option->getGroups())) { - \call_user_func_array($option->getAction(), $this->getArguments($option, [], $request->getParams())); - } - } - } catch (\Throwable $e) { - foreach (self::$errors as $error) { // Global error hooks - if (in_array('*', $error->getGroups())) { - self::setResource('error', function () use ($e) { - return $e; - }); - try { - $arguments = $this->getArguments($error, [], $request->getParams()); - \call_user_func_array($error->getAction(), $arguments); - } catch (\Throwable $e) { - throw new Exception('Error handler had an error: ' . $e->getMessage() . "\nStack trace: " . $e->getTraceAsString(), 500, $e); - } - } - } - } } else { foreach (self::$errors as $error) { // Global error hooks
🧹 Nitpick comments (1)
src/Http.php (1)
859-861: Minor: Consider using parsed path instead of raw URI for fallback route.When no route matches and no wildcard exists, the fallback creates a Route using
$request->getURI(), which may include query strings (e.g.,/path?query=1). For consistency with how the wildcard route handling works (line 855 usesparse_url), consider using the parsed path.💡 Suggested improvement
self::setResource('route', function () use ($route, $request) { - return $route ?? new Route($request->getMethod(), $request->getURI()); + $path = \parse_url($request->getURI(), PHP_URL_PATH) ?? '/'; + return $route ?? new Route($request->getMethod(), $path); });
|
|
||
| Http::init() | ||
| ->inject('route') | ||
| ->action(function (Route $route) use (&$initRoutePath, &$initRouteMethod) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use response
| $actionRoutePath = null; | ||
| $actionRouteMethod = null; | ||
|
|
||
| Http::init() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have separate test function for init
| $_SERVER['REQUEST_METHOD'] = 'GET'; | ||
| $_SERVER['REQUEST_URI'] = '/route-inject-shutdown'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a hacky pattern? Can't we do the same as other tests and build a reeal request?
… route resource after wildcard resolution
Summary by CodeRabbit
Refactor
Tests