Closed garygreen closed 5 years ago
This is because Illuminate\Http\UploadedFile
extends Symfony\Component\HttpFoundation\File\UploadedFile
which inherits its move
method. The Storage
facade uses the underlying Filesystem
to move files and thus works differently. You say it uses "put" under the hood but I believe that to be incorrect. It seems to me it uses a plain PHP rename
under the hood.
As these ways differ from each other I don't think that these two methods should match each others behavior exactly.
As these ways differ from each other I don't think that these two methods should match each others behavior exactly.
I disagree completely. If your production implementation needs to change because the testing environment doesn't quite match then there's a discrepancy there that needs to be addressed. Simply suggesting to use the Storage
method doesn't really help if you have code that relies on ->move()
which is perfectly valid in production code.
It really frustrates me that issues like this are closed without much thought or consideration 😒
Using the Storage
facade (moving files) and using the move
method on the UploadedFile
class (moving the current file) are two very different things imo. If you disagree then it's best that you open up an issue in laravel/ideas to see if there's more people who agree with you. If there's enough support we could change the behavior perhaps.
Using the Storage facade (moving files) and using the move method on the UploadedFile class (moving the current file) are two very different things imo
The methods on the storage class will copy the file your quite right, but Symphony's move
method should always be a file that can be moved (not a tmpfile()
)
At the moment, ->move()
is completely broken for everyone when unit testing, which means any production code you have which uses it won't work with a fake UploadedFile
. Remember - this is only for when your creating a fake uploaded file solely for the purpose of testing.
I would say it's perfectly acceptable to move the temporary file to the same location as used in Storage
- by doing this it would align both implementations.
Ah I think I see where you're getting at. I think this falls a little out of scope why the fake Test
file exists. Please note that the fake method doesn't returns an instance of UploadedFile
but an instance of Illuminate\Http\Testing\File
which extends UploadedFile
. This instance is purely for testing and assertions and not meant for manipulations.
So you can't do integration testing with fake file uploads that get moved?
Here's an example of what our production code looks like (simplified example):
namespace Tests\Feature;
use Tests\TestCase;
use Illuminate\Http\UploadedFile;
use Illuminate\Foundation\Testing\DatabaseTransactions;
class UserRegisterTest extends TestCase
{
use DatabaseTransactions;
public function test_can_register_user()
{
$response = $this
->post(route('users.store'), [
'email' => 'jane@blahh.com',
'picture' => UploadedFile::fake()->image('jane.jpg', 400, 400),
]);
$response->assertSessionHasNoErrors();
$user = User::first();
$this->assertNotNull($user);
$this->assertEquals('jane@blah.com', $user->email);
$this->assertTrue(file_exists(storage_path('user-pictures/' . $user->picture)));
}
}
class UserController extends Controller {
public function store()
{
$user = User::create([
'email' => request('email'),
'picture' => Str::random(10) . '.jpg',
]);
// This works perfectly fine in production code.
// But unit testing with a fake uploaded file won't work because `->move()`
// is trying to move a tmpfile() which won't work.
request()->file('picture')->move(storage_path('user-pictures/' . $user->picture));
return redirect()->route('users.show', $user);
}
}
This instance is purely for testing and assertions and not meant for manipulations.
This clearly isn't true because it does work when you use Storage
. Just because you don't use storage and use ->move
instead doesn't mean it shouldn't work, IMO. I personally don't use Storage because I find it restrictive as you need to use a certain root path, also I use legacy code which uses ->move()
before storage even existed.
The problem is that files created with tmpfile
are resources and not actual files. In order to do what you want to do we will need to modify the test files to be actual files. As this is a behavioral change rather than a bug you'll need to make a proposition for this in laravel/ideas first to see if this would be a valid addition. After that a PR can be send in if it's deemed valid.
Sorry to resurrect such an old thread, but how did you "resolve" this for your own use-case @garygreen ?
I have the exact same scenario here and am looking for a workable solution.
Interestingly enough, I don't believe this was ever a problem when testing on GNU/Linux; it only became a problem when testing on Windows.
@cbj4074 honestly I didn't really look that much further into it so I basically gave up unit testing in regards to file uploads. 😆
From what I gather, if you want to use Laravel's UploadedFile::fake()
then you must also use Storage
for all your underlying storage needs rather than calling Symphony methods directly e.g. $file->move()
otherwise when you go to unit test it will break as there is a discrepency between what Symphony expects (a real file) and what Laravel gives it (a fake file tmpfile
).
If you do manage to solve it then let me know as I'm still looking for a solution that doesn't require using Storage
@garygreen Thank you for the quick reply! I appreciate it tremendously.
Like you, I don't have the time or patience to wrestle with it, so I'm just using a real file from the filesystem in the test:
$testFile = base_path('tests/assets/test.jpg');
$file = new UploadedFile($testFile, 'test.jpg', 'image/png', filesize($testFile), null, true);
The test passes and I can move on with life. Of course, this "solution" is in no way ideal because it crosses I/O boundaries and requires the test file to be under version control (it's a tiny JPEG file).
In the end, I'm not sure which is worse: using Storage
everywhere, or changing the test(s) in this way.
Description:
When you use
$file->move($path)
on aIlluminate\Http\Testing\File
it will fail because the file is locked. This can cause implementation difficulty if you use->move
in your application instead ofStorage::
Steps To Reproduce:
This is because under the hood Laravel uses
tmpfile
to generate a temporary file which is locked and cannot be moved. However if you use theStorage
method, it will work:This seems a bit of a discrepancy to me. If the testing uploaded file is extending Symphony, all methods on it should still work, really?