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

Added App::process() to expose Request -> Response functionality #1688

Merged
merged 2 commits into from
Dec 22, 2015

Conversation

AndrewCarterUK
Copy link
Contributor

The Problem

It's currently very difficult to give the application a request and retrieve a response. To have a request that isn't created from the PHP super globals you need to patch the container.

A project of mine (PHPFastCGI) has struggled to provide a clean adapter to the Slim framework for this reason.

The Solution

Split App::run() into two methods by creating App::process(). The latter acts as a simple map from request to response.

Downsides

The are two that I can think of:

  1. It adds a new method to the public API that needs to be maintained and supported.
  2. Confusion over the difference between App:process() and App::__invoke(). Both have the same signature and do similar things, but ultimately at a different level.

Questions

Assuming that people think this is a good idea:

  • How would you like me to test this? I haven't really added any features. Any test of App::run() now covers App:process(). I'm not adverse to adding unit tests, I just thought it was worth asking here what people think they should look like rather than just adding something pointless for the sake of it.
  • Would you like me to add documentation (and where)?

Interested to hear thoughts :)

@@ -297,13 +297,39 @@ public function run($silent = false)
$request = $this->container->get('request');
$response = $this->container->get('response');

$response = $this->process($request, $response);

$response = $this->finalize($response);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This:

$response = $this->finalize($response);

should be on process(), shouldn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot!

@geggleto geggleto mentioned this pull request Dec 18, 2015
@geggleto
Copy link
Member

I don't see the problem you are solving with this PR. Is there something that __invoke doesn't do that you need?

To have a request that isn't created from the PHP super globals you need to patch the container. totally agree with you. https://github.com/geggleto/reactive-slim/blob/master/bin/app.php such a hack :)

@mathmarques
Copy link
Contributor

When using directly __invoke, you are ignoring the app level middlewares. I think this is the only problem.

@geggleto
Copy link
Member

I think we should fix that...

@AndrewCarterUK
Copy link
Contributor Author

@geggleto

__invoke() also doesn't have the exception handling logic. If __invoke() worked in such a way that doing something like:

$request = Request::createFromGlobals();
$response = $app($request, $response);
$app->send($response);

Was the same as doing:

$app->run();

Then that would solve the problem.

(Edit) - But that sort of change couldn't be made until 4.x because it would break B/C

@geggleto
Copy link
Member

@AndrewCarterUK there is 1 other way to do this.

$res = $app->run(true);

https://github.com/slimphp/Slim/blob/3.x/tests/AppTest.php#L1558-L1592

@AndrewCarterUK
Copy link
Contributor Author

@geggleto That only works if you've patched the container or want the request to be created from global variables.

In my project the request is received from a FastCGI connection.

(edit to clarify: That's not PHP-FPM, the application runs as its own FastCGI daemon)

@akrabat akrabat merged commit 8bc17f1 into slimphp:3.x Dec 22, 2015
akrabat added a commit that referenced this pull request Dec 22, 2015
@akrabat
Copy link
Member

akrabat commented Dec 22, 2015

Looks good to me and maybe is that start of removing request and response from the container.

@akrabat akrabat added this to the 3.1.0 milestone Dec 22, 2015
@adambro
Copy link
Contributor

adambro commented Jan 8, 2016

The same problem is for creating WebTest class that mocks Slim, creates a request and checks response object. For now I'm using __invoke which works, because I don't have any middleware yet.

@stivni
Copy link

stivni commented Jul 14, 2016

Looks good to me and maybe is that start of removing request and response from the container.

@akrabat is there an issue for that?

@geggleto
Copy link
Member

@stivni its a 4.0 thing ;)

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

Successfully merging this pull request may close these issues.

6 participants