Skip to content

[8.x] Use duplicate instead of createFromBase to clone request when routes are cached#42420

Merged
taylorotwell merged 1 commit into
laravel:8.xfrom
rodrigopedra:8.x
May 18, 2022
Merged

[8.x] Use duplicate instead of createFromBase to clone request when routes are cached#42420
taylorotwell merged 1 commit into
laravel:8.xfrom
rodrigopedra:8.x

Conversation

@rodrigopedra

@rodrigopedra rodrigopedra commented May 18, 2022

Copy link
Copy Markdown
Contributor

Closes #42403

When routes are compiled, the CompiledRouteCollection@requestWithoutTrailingSlash method uses the static method Request@createFromBase to clone a Illuminate\Http\Request.

When the request is a JSON request, this call causes the request's JSON payload to be parsed twice:

Actually, issue #42403 describes a memory exhaust scenario with default PHP configuration.

The doc block on CompiledRouteCollection@requestWithoutTrailingSlash states this method Get a cloned instance of the given request without any trailing slash on the URI, so as the original $request is already a Illuminate\Http\Request, e.g. it is already bootstrapped, we can safely change this call to use the Request@duplicate method, which internally actually clones the request object, avoiding the double json_encode parsing.

This PR:

  • Changes the method CompiledRouteCollection@requestWithoutTrailingSlash to call Request@duplicate instead of Request@captureFromBase, to clone the current request.

Notes:

This behavior only happens when routes are cached.

I didn't add any tests, as I would need to mock the Illuminate\Http\Request and assert its static method createFromBase was not called twice.

I don't know how to properly mock static methods. If someone can guide me on how to do it, I will appreciate.

@rodrigopedra rodrigopedra changed the title [8.x] Prefer cloning a Illuminte Request with duplicate instead of createFromBase [8.x] Use duplicate instead of createFromBase to clone request when routes are cached May 18, 2022
@taylorotwell taylorotwell merged commit ebe067c into laravel:8.x May 18, 2022
@driesvints

Copy link
Copy Markdown
Member

Thanks @rodrigopedra

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