Fix moving uploaded file crashed when source was stream

[raigu]

Description

When UploadedFile is created from stream then calling movingTo will throw an exception.

Script to reproduce the bug

require_once __DIR__ . '/vendor/autoload.php';

$file = new \Slim\Psr7\UploadedFile(
    (new \Slim\Psr7\Factory\StreamFactory())->createStream('Hello, World!')
);

$targetPath = tempnam(sys_get_temp_dir(), 'sample');
$file->moveTo($targetPath);

echo file_get_contents($targetPath);

Expected output:

Hello, World!

Actual output:

PHP Warning:  rename(): PHP wrapper does not support renaming in /home/rait/dev/github/Slim-Psr7/src/UploadedFile.php on line 172
PHP Stack trace:
PHP   1. {main}() /home/rait/dev/github/Slim-Psr7/demo.php:0
PHP   2. Slim\Psr7\UploadedFile->moveTo() /home/rait/dev/github/Slim-Psr7/demo.php:10
PHP   3. rename() /home/rait/dev/github/Slim-Psr7/src/UploadedFile.php:172
PHP Fatal error:  Uncaught RuntimeException: Error moving uploaded file  to /tmp/samplezHN38z in /home/rait/dev/github/Slim-Psr7/src/UploadedFile.php:173
Stack trace:
#0 /home/rait/dev/github/Slim-Psr7/demo.php(10): Slim\Psr7\UploadedFile->moveTo('/tmp/samplezHN3...')
#1 {main}
  thrown in /home/rait/dev/github/Slim-Psr7/src/UploadedFile.php on line 173

Motivation

I discovered this bug when I was creating a stub for a test.

Notes on implementation

I used stream_copy_to_stream for copying. I speculate that built-in stream function should be more efficient because it is implemented in C.

My fix also handles php://temp case where huge files might be the case.

[coveralls]

Coverage decreased (-0.1%) to 99.873% when pulling c5fb95a on raigu:fix-uploaded-file-move-to into dea7736 on slimphp:master.

[raigu]

I have problem of creating meaningful test in order to improve the code coverage. I kind of recognize too much duplications.

I was thinking that maybe I should restructure the moveTo method?

I recognize three possible scenarios how to apply the movement behavior:

SAPI	source	target	method
yes	-	-	move_uploaded_file
no	file	file	rename
no	file	resource	stream_copy_to_stream
no	resource	file	stream_copy_to_stream
no	resource	resource	stream_copy_to_stream
no	file	uri	stream_copy_to_stream
no	resource	uri	stream_copy_to_stream

The SAPI would be first because PSR-7 documentation states:

When used in an SAPI environment where $_FILES is populated, when writing
files via moveTo(), is_uploaded_file() and move_uploaded_file() SHOULD be
used to ensure permissions and upload status are verified correctly.

Then if source and destination are files, then moving file would be most efficient.

All others will be moved as streams. If parameter is file (or uri), it is opened using fopen.

Also noticed that there are test cases that cover cases when $targetPath is uri.
Will make sure that it will not be broken and backward compatibility is guaranteed.
They should also work with fopen.

Before making this effort would like to have blessing from author.

[l0gicgate]

I think that we should do something similar to:
https://github.com/Nyholm/psr7/blob/master/src/UploadedFile.php#L120-L150

[raigu]

I think that we should do something similar to:
https://github.com/Nyholm/psr7/blob/master/src/UploadedFile.php#L120-L150

Probably yes, but I decided to fix only the original bug.

Current code works, refactoring has always risks. Better to leave it in case there is major improvement planned.

Currently managed to make tests that covered checks that where required by PHPStan.

[raigu]

Ready for review and merge.

Only one PHPStan warning remained which, turns out, exists in master also.

$ ./vendor/bin/phpstan analyse src
Note: Using configuration file /home/rait/dev/gs/Slim-Psr7/phpstan.neon.dist.
 18/18 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   Factory/StreamFactory.php                                                                                                                               
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------- 
  74     Parameter #1 $callback of function set_error_handler expects (callable(int, string, string, int, array): bool)|null, Closure(int, string): null given.  
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------- 

[l0gicgate]

I'm closing this as stale. Feel free to reopen.