Use Laravel UploadedFile class for requests #3417

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@torkiljohnsen
Contributor

torkiljohnsen commented Aug 8, 2016

Using Symfony’s UploadedFile class has the effect that when Laravel’s Request class does convertUploadedFiles(), all files are converted to Laravel’s UploadedFile class, but with the $test set to false.

This, in turn, makes it difficult to use the class in a test, because the class then must pass the is_uploaded_file() test in UploadedFile::isValid().

This is just a proposal, I have no tests to go with the code, as this repo is missing some basic requirements for this class to even be instantiable, like Laravel's Eloquent and Symfony's HttpFoundation component. Please have a look @janhenkgerritsen.

Use Laravel UploadedFile class for requests
Using Symfony’s UploadedFile has the effect that when Laravel’s Request class does convertUploadedFiles, all files are converted to Laravel’s UploadedFile class, but with the test flag set to false.

This, in turn, makes it difficult to use the class in a test, because the class then must pass the is_uploaded_file test in UploadedFile::isValid().
@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Aug 10, 2016

Contributor

Can you show me some pseudo code of a test that fails due to this problem? That way I can play around with it a bit. I think this PR will not cause any problems, but I'd like to look into it myself a bit better before I merge it.

Tests for this sort of problem should be added to the Laravel 5 sample application, but I can do that myself.

Contributor

janhenkgerritsen commented Aug 10, 2016

Can you show me some pseudo code of a test that fails due to this problem? That way I can play around with it a bit. I think this PR will not cause any problems, but I'd like to look into it myself a bit better before I merge it.

Tests for this sort of problem should be added to the Laravel 5 sample application, but I can do that myself.

@torkiljohnsen

This comment has been minimized.

Show comment
Hide comment
@torkiljohnsen

torkiljohnsen Aug 10, 2016

Contributor

Sure. Here is a CEST method and the corresponding controller endpoint. This uses the REST module too obviously.

// CEST test
public function createMedia(ApiTester $i)
{
    $i->wantTo('create media');
    $i->sendPOST('media', [], ['file' => codecept_data_dir('cat1.jpg')]);
    $i->seeRecord('media', [
        'path' => public_path('images/cat1.jpg')
    ]);
}

// Controller method
public function store(Request $request)
{
    $uploadedFile = $request->file('file');
    $file = $uploadedFile->move(public_path('images'), $uploadedFile->getClientOriginalName());
    Media::create(['path' => $file->getRealPath()]);

    return response(null, 201);
}

Without my patch, this will fail, because $uploadedFile->move() will test for isValid(), which will return false, since $uploadedFile->test is set to false when Symfony UploadedFile is converted to Laravel UploadedFile. So $uploadedFile->move() will not work at all in the test.

Contributor

torkiljohnsen commented Aug 10, 2016

Sure. Here is a CEST method and the corresponding controller endpoint. This uses the REST module too obviously.

// CEST test
public function createMedia(ApiTester $i)
{
    $i->wantTo('create media');
    $i->sendPOST('media', [], ['file' => codecept_data_dir('cat1.jpg')]);
    $i->seeRecord('media', [
        'path' => public_path('images/cat1.jpg')
    ]);
}

// Controller method
public function store(Request $request)
{
    $uploadedFile = $request->file('file');
    $file = $uploadedFile->move(public_path('images'), $uploadedFile->getClientOriginalName());
    Media::create(['path' => $file->getRealPath()]);

    return response(null, 201);
}

Without my patch, this will fail, because $uploadedFile->move() will test for isValid(), which will return false, since $uploadedFile->test is set to false when Symfony UploadedFile is converted to Laravel UploadedFile. So $uploadedFile->move() will not work at all in the test.

@torkiljohnsen

This comment has been minimized.

Show comment
Hide comment
@torkiljohnsen

torkiljohnsen Aug 10, 2016

Contributor

To elaborate: When the POST request is made, Laravel converts files to its native UploadedFile class in Illuminate\Http\Request->convertUploadedFiles(): It calls UploadedFile::createFromBase($file);, which in turn has $test hardcoded to false, probably because there is no public getter for getting the test parameter from Symfony's UploadedFile.

Contributor

torkiljohnsen commented Aug 10, 2016

To elaborate: When the POST request is made, Laravel converts files to its native UploadedFile class in Illuminate\Http\Request->convertUploadedFiles(): It calls UploadedFile::createFromBase($file);, which in turn has $test hardcoded to false, probably because there is no public getter for getting the test parameter from Symfony's UploadedFile.

@torkiljohnsen

This comment has been minimized.

Show comment
Hide comment
@torkiljohnsen

torkiljohnsen Aug 10, 2016

Contributor

When an instance of Laravel's UploadedFile is passed to UploadedFile::createFromBase(), it just returns the instance directly, so the test flag is kept at true.

Contributor

torkiljohnsen commented Aug 10, 2016

When an instance of Laravel's UploadedFile is passed to UploadedFile::createFromBase(), it just returns the instance directly, so the test flag is kept at true.

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Aug 10, 2016

Contributor

I created a test in the Laravel 5 sample application that reproduces your issue. The fix you propose is also correct, only you forgot to add the import for the UploadedFile class. That's why I created a new pull request #3429.

After the PR is merged I will push the test to the Laravel 5 sample application, because it will break the build if I do that right away.

Thanks for the help!

Contributor

janhenkgerritsen commented Aug 10, 2016

I created a test in the Laravel 5 sample application that reproduces your issue. The fix you propose is also correct, only you forgot to add the import for the UploadedFile class. That's why I created a new pull request #3429.

After the PR is merged I will push the test to the Laravel 5 sample application, because it will break the build if I do that right away.

Thanks for the help!

@torkiljohnsen

This comment has been minimized.

Show comment
Hide comment
@torkiljohnsen

torkiljohnsen Aug 10, 2016

Contributor

Ouch. Good call on the missing import and thanks for helping out :)

Contributor

torkiljohnsen commented Aug 10, 2016

Ouch. Good call on the missing import and thanks for helping out :)

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