Moving testing UploadedFile with Symphony breaks

[garygreen]

• Laravel Version: 5.7
• PHP Version: 7.2

Description:

When you use $file->move($path) on a Illuminate\Http\Testing\File it will fail because the file is locked. This can cause implementation difficulty if you use ->move in your application instead of Storage::

Steps To Reproduce:

// This will error with:
// Could not move the file "C:\Users\garreh\AppData\Local\Temp\phpD967.tmp" to "E:\Sites\cc-new\jane.jpg\phpD967.tmp"
// The process cannot access the file because it is being used by another process. (code: 32))
$file = Illuminate\Http\UploadedFile::fake()->image('jane.jpg', 400, 400);
$file->move(base_path('jane.jpg'));

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 the Storage method, it will work:

// When you use the storage which uses "put" under the hood rather than a move (like Symphony suggests), it then works:
Storage::fake('local');
Storage::disk('local')->move('test', $file);

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?

[driesvints]

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.

[garygreen]

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.

[garygreen]

It really frustrates me that issues like this are closed without much thought or consideration 😒

[driesvints]

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.

[garygreen]

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.

[driesvints]

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.

framework/src/Illuminate/Http/Testing/FileFactory.php

Lines 9 to 21 in 8da5b7d

	/**
	* Create a new fake file.
	*
	* @param string $name
	* @param int $kilobytes
	* @return \Illuminate\Http\Testing\File
	*/
	public function create($name, $kilobytes = 0)
	{
	return tap(new File($name, tmpfile()), function ($file) use ($kilobytes) {
	$file->sizeToReport = $kilobytes * 1024;
	});
	}

[garygreen]

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.

[driesvints]

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.

[cbj4074]

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.

[garygreen]

@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

[cbj4074]

@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.