-
Notifications
You must be signed in to change notification settings - Fork 15
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
Exclude method OPTIONS in the list of supported request methods #11
Conversation
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.
OPTIONS
should definitely be ignored 👌
I'm curious to understand when Route doesnt have a getName method before I merge.
return false; | ||
} | ||
|
||
if (isset($regex['uri']) && $regex['uri'] && preg_match($regex['uri'], $route->uri)) { | ||
return false; | ||
} | ||
|
||
// Lets exclude OPTIONS in the list of methods supported | ||
if (Str::is($event->request->getMethod(), 'OPTIONS')) { |
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.
👍
@@ -33,15 +34,21 @@ protected function shouldLogUsage($event) | |||
|
|||
$route = $event->request->route(); | |||
$regex = config('route-usage.excluding-regex'); | |||
$name = method_exists($route, 'getName') ? $route->getName() : null; |
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.
Can you explain when this method would not be defined?
Thanks for your contributions! 👍 |
Could you remove the dependency on <?php $event->request->isMethod('options'); ?> |
Good idea. It's also more readable. Any reason why you want to remove the dependency to Str (since this package only works with Laravel anyway)? |
@julienbourdeau None in particular, other than it could read a little better as you've said, and as it's on the request object Laravel will already be handling bizarre edge cases (if there are any?) |
@ingalless @harlekoy In the end, I went for #12. Thanks for the suggestions and ideas 💯 |
No description provided.