Use illuminate's Request::createFromBase to create laravel requests#137
Use illuminate's Request::createFromBase to create laravel requests#137andig merged 1 commit intophp-pm:masterfrom tobiasdierich:master
Conversation
|
I understand |
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? |
Yes, exactly.
Yep,
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 |
|
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:
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. |
|
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
|
That's because I believe this is where the actual difference comes from. Since So... where to take it from there? |
|
We could check if were currently creating a Illuminate request, if so apply the important line from 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. |
|
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? |
|
Sorry for the late response. I'll update my PR as soon as possible, probably the upcoming weekend. |
|
I've just updated my PR. Turns out So I just duplicated that part into the http bridge. |
|
Great, thank you! |
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.