php-pm / php-pm-httpkernel

HttpKernel adapter for use of Symfony and Laravel frameworks with PHP-PM
MIT License
246 stars 72 forks source link

Use illuminate's Request::createFromBase to create laravel requests #137

Closed tobiasdierich closed 6 years ago

tobiasdierich commented 6 years ago

This PR fixes #136.

There are now two different bootstrapper for laravel, one for v4 and one for v5. This was the easiest way to fix the issue mentioned in #136 without changing too much of the existing code. Please let me know what you think about this structure. I have a few more ideas in mind how you could implement this, but they require a lot more refactoring.

I'll add some tests once you're okay with the implementation and I've figured out how to test the mapper sensibly.

andig commented 6 years ago

I understand Laravel4 is the backwards compatible provider, Laravel the new one? Both seem to share a bit of common code but don't inherit anything. Imho that will make maintenance and bugfixing a bit harder. On the other side the differences don't seem so bug. Do you think it would be possible to combine this in one file? Is the createFromBase available in earlier Laravel versions? If needed I'd be happy to get rid of the RequestClassProviderInterface and move this PR to 2.0 then.

andig commented 6 years ago

Do you think it would be possible to combine this in one file? Is the createFromBase available in earlier Laravel versions?

Actually, that seems to be the case: https://laravel.com/api/4.2/Illuminate/Http/Request.html#method_createFromBase. Do we really need separate versions then?

tobiasdierich commented 6 years ago

I understand Laravel4 is the backwards compatible provider, Laravel the new one?

Yes, exactly.

Both seem to share a bit of common code but don't inherit anything.

Yep, initialize, preHandle, postHandle and resetProvider could be implemented in a parent class. However, I was not sure about whether there might be some v4 only or v5 only stuff in the future, e.g. you might want to use a custom preHandle for v4 and a different one for v5. That's why I simply duplicated the code for now.

Actually, that seems to be the case: https://laravel.com/api/4.2/Illuminate/Http/Request.html#method_createFromBase. Do we really need separate versions then?

createFromBase was introduced in laravel 4.1, v 4.0 does not have it, see: https://github.com/illuminate/http/blob/4.0/Request.php. If it's okay to drop support for laravel < 4.1, we could get rid of the Laravel4 bootstrapper completely.

Do you think it would be possible to combine this in one file?

I actually tried to come up with a solution which only needs a single bootstrapper for laravel 4 and 5. However, the solutions I came up with were either kinda ugly (i.e. more or less hardcoded and not customisable) or would require a large(r) rewrite of the existing code. Imho the cleanest solution would be to move the request creation from the HttpKernel to the bridges. The bridges would then just receive the psr request and can do whatever they want to create a request. This would also make it easier to add support for frameworks which are not symfony based.

andig commented 6 years ago

I must admit that I don't like the amount of code this PR changes for an apparently simple problem. Going back to the problem:

Access the request data using $request->get('data') returns nothing, however using $request->data works.

First, I don't see where and why request->data is even supposed to work (though it does). Then, I couldn't quickly find out why request->get(data) should work.

Once we understand both questions it might be a simple matter of setting the data parameter in the bridge as expected? Before doing so I'd still like to understand if there are other cases to consider.

tobiasdierich commented 6 years ago

I agree, the amount of code is high for a seemingly simple problem.

I dug a little into the Illuminate request class source code and found that this line is the reason why createFromBase fixes the issue. If you remove that line, $request->get('data') will return null again, i.e. it will no longer fix the problem.

$request->data works without createFromBase because it is implemented by the Illuminate request class and directly searches the entire request input using $request->input().

$request->get() on the other hand is a symfony method which should retrieve data from $request->request. $request->request should in theory be initialised with the parsed body from the psr request retrieved here since it's passed to the symfony request constructor as the second parameter. However, $psrRequest->getParsedBody() always returns null when using JSON content type (works with form data). Shouldn't this method return the parsed JSON ?

andig commented 6 years ago

If you remove that line, $request->get('data') will return null again, i.e. it will no longer fix the problem.

get is Symfony, see https://github.com/symfony/http-foundation/blob/master/Request.php#L685. It will take attributes, query or post.

$request->data works without createFromBase because it is implemented by the Illuminate request class and directly searches the entire request input using $request->input().

That's because input() uses getInputSource(). getInputSource is funny since it actually treats JSON as structured data by converting to array.

I believe this is where the actual difference comes from. Since createFromBase again uses getInputSource it will populate that JSON as array. It seems a bit peculiar but there it is. I don't think we can apply the same behaviour to the Symfony mapping but we could for any Laravel version.

So... where to take it from there?

tobiasdierich commented 6 years ago

We could check if were currently creating a Illuminate request, if so apply the important line from createFromBase manually, like:

if ($this->bootstrap instanceof RequestClassProviderInterface) {
    $class = $this->bootstrap->requestClass();
} else {
     $class = SymfonyRequest::class;
}

/** @var SymfonyRequest $syRequest */
$syRequest = new $class($query, $post, $attributes = [], $_COOKIE, $uploadedFiles, $_SERVER, (string)$psrRequest->getBody());
$syRequest->setMethod($method);

if ($syRequest instanceof \Illuminate\Request\Http) {
    $syRequest->request = $request->getInputSource();
}

That way we can remove the separate bootstrapper for laravel 4.

andig commented 6 years ago

Or check if ‘getInputSource()‘ exists if subclassed. None of this is very clean but it looks less convoluted?

Update I guess type checking should work better if that method always exists in all Laravel versions (?). Or checking both.

Do you want to update this PR?

tobiasdierich commented 6 years ago

Sorry for the late response. I'll update my PR as soon as possible, probably the upcoming weekend.

tobiasdierich commented 6 years ago

I've just updated my PR. Turns out getInputSource() is protected, so I could not use it. However, the only part we need is:

if ($request->isJson()) {
    $request->request = $request->json;
}

So I just duplicated that part into the http bridge.

andig commented 6 years ago

Great, thank you!