Skip to content

Application::getMethod() and Application::getPathInfo() #6

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

Closed
wants to merge 1 commit into from
Closed

Application::getMethod() and Application::getPathInfo() #6

wants to merge 1 commit into from

Conversation

iwankgb
Copy link
Contributor

@iwankgb iwankgb commented Apr 14, 2015

There is no reason to rely on getMethod() and getPathInfo() methods from Laravel\Lumen\Application as respective methods of Symfony\Component\HttpFoundation\Request provides very same functionality.

@mlukaszewski
Copy link

👍

@GrahamCampbell
Copy link
Member

This is intentional. Luman doesn't create a symfony request object if it doesn't need to in order to save time.

@iwankgb
Copy link
Contributor Author

iwankgb commented Apr 14, 2015

@GrahamCampbell - what about getMethod() allowing to use non-existent method names? I don't think it's a very good idea to be honest.
Regarding not creating Symfony object for performance reasons - as far as I understand Application::dispatch() is intended to be called from Application::handle() (and will never be null in this case) or Application::run() (and might be null in this case). Wouldn't it make sense to drop implementation of HttpKernelInterface in this case? It does not seem to be used anywhere in the application (app/index.php in lumen repository calls Application::run() only).

@iwankgb
Copy link
Contributor Author

iwankgb commented Apr 14, 2015

An update: run() method seems to be used when handling HTTP request but handle() method is being used in tests - so code used to handle HTTP requests is not really being tested.

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.

3 participants