Skip to content
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

Add CLI check to RouteCollection #2022

Closed
wants to merge 1 commit into from

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented May 22, 2019

Description
CodeIgniter.php sets the request method on request or clirequest service, so RouteCollection needs to check the proper service to get the correct HTTPVerb to account for CLI, spoofing, and standard scenarios.
See #2021

FWIW I believe the reason this wasn't caught is that all the request & routing clauses in CodeIgniter.php have conditional ! (ENVIRONMENT === 'testing') to be able to simulate non-CLI requests from CLI. There should probably be a test that spoofs its own environment (if possible) just to check for spark integrity.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

CodeIgniter.php sets the request method on `request` or `clirequest` service, so RouteCollection needs to check the proper service to get the correct HTTPVerb to account for CLI, spoofing, and standard scenarios.
See codeigniter4#2021
@lonnieezell
Copy link
Member

I think a slightly better fix here is to either pass in the request instance either through the constructor or through a setter method. That removes the dependency creation back to CodeIgniter.php, where it already knows which type of request it is.

@MGatner
Copy link
Member Author

MGatner commented May 22, 2019

That's a good idea! I'm out for a few days but I'll take a look when I get a chance. Glad for anyone else to hop in before then.

@MGatner
Copy link
Member Author

MGatner commented May 23, 2019

Looking at this, get/set HTTPVerb and the $HTTPVerb property only appear to be used internally to RouteCollection and really just for getRoutes(). It is also always just a duplicate of $_SERVER['REQUEST_METHOD'] (old version) or $request->getMethod() (current revision).

Do we get rid of HTTPVerb altogether, overload the constructor to __construct(FileLocator $locator, $moduleConfig, $request = null) and then just use $request->getMethod() for getRoutes() when $verb isn't supplied explicitly?

@MGatner
Copy link
Member Author

MGatner commented May 23, 2019

One more place for HTTPVerb, called from Router but extraneous as it is simply injecting what is loaded by default:

./Router/Router.php:390: $routes = $this->collection->getRoutes($this->collection->getHTTPVerb());

@lonnieezell
Copy link
Member

@MGatner I've got this about wrapped up, actually. Just have one more test I have to fix up. It was a bigger change than expected. Due to the way routeCollection is used I didn't want to force a new call within route files to that, so I added it as a dependency to the Router class, which then sets the HTTPVerb on the RouteCollection itself.

@MGatner
Copy link
Member Author

MGatner commented May 23, 2019

Oh great! Thanks for addressing it - I look forward to seeing your solution. Any thoughts on testing (re: my original comment "FWIW...")?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants