Skip to content

Commit

Permalink
Merge pull request #98 from k2rn/fixed_UploadedFile_moveTo
Browse files Browse the repository at this point in the history 
Fixed `UploadedFile::moveTo()` so it actually removes the original file when used in CLI context, and doesn't leave orphaned files
  • Loading branch information
@Ocramius
Ocramius authored Jul 6, 2022
2 parents 78846cb + 091487a commit 1f97b0c
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 87 deletions.
46 changes: 1 addition & 45 deletions psalm-baseline.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1164,54 +1164,10 @@
</MissingReturnType>
</file>
<file src="test/UploadedFileTest.php">
<MissingParamType occurrences="6">
<code>$path</code>
<code>$status</code>
<code>$status</code>
<code>$status</code>
<code>$status</code>
<code>$streamOrFile</code>
</MissingParamType>
<MissingPropertyType occurrences="1">
<code>$tmpFile</code>
</MissingPropertyType>
<MissingReturnType occurrences="24">
<code>errorConstantsAndMessages</code>
<code>invalidErrorStatuses</code>
<code>invalidMovePaths</code>
<code>invalidStreams</code>
<code>nonOkErrorStatus</code>
<code>testCannotRetrieveStreamAfterMove</code>
<code>testConstructorDoesNotRaiseExceptionForInvalidStreamWhenErrorStatusPresent</code>
<code>testGetStreamRaisesExceptionWhenErrorStatusPresent</code>
<code>testGetStreamRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected</code>
<code>testGetStreamReturnsOriginalStreamObject</code>
<code>testGetStreamReturnsStreamForFile</code>
<code>testGetStreamReturnsWrappedPhpStream</code>
<code>testMoveCannotBeCalledMoreThanOnce</code>
<code>testMoveRaisesExceptionForInvalidPath</code>
<code>testMoveToCreatesStreamIfOnlyAFilenameWasProvided</code>
<code>testMoveToRaisesExceptionWhenErrorStatusPresent</code>
<code>testMoveToRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected</code>
<code>testMovesFileToDesignatedPath</code>
<code>testRaisesExceptionOnInvalidErrorStatus</code>
<code>testRaisesExceptionOnInvalidStreamOrFile</code>
<code>testValidClientFilename</code>
<code>testValidClientMediaType</code>
<code>testValidNullClientFilename</code>
<code>testValidSize</code>
</MissingReturnType>
<MixedArgument occurrences="6">
<MixedArgument occurrences="2">
<code>$path</code>
<code>$status</code>
<code>$status</code>
<code>$status</code>
<code>$status</code>
<code>$streamOrFile</code>
</MixedArgument>
<UndefinedThisPropertyAssignment occurrences="1">
<code>$this-&gt;tmpfile</code>
</UndefinedThisPropertyAssignment>
</file>
<file src="test/UriTest.php">
<InvalidScalarArgument occurrences="2">
Expand Down
7 changes: 7 additions & 0 deletions src/UploadedFile.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,13 @@ public function moveTo($targetPath) : void
case (empty($sapi) || 0 === strpos($sapi, 'cli') || 0 === strpos($sapi, 'phpdbg') || ! $this->file):
// Non-SAPI environment, or no filename present
$this->writeFile($targetPath);

if ($this->stream instanceof StreamInterface) {
$this->stream->close();
}
if (is_string($this->file) && file_exists($this->file)) {
unlink($this->file);
}
break;
default:
// SAPI environment, with file present
Expand Down
115 changes: 73 additions & 42 deletions test/UploadedFileTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,23 +30,33 @@
use const UPLOAD_ERR_OK;
use const UPLOAD_ERR_PARTIAL;

class UploadedFileTest extends TestCase
final class UploadedFileTest extends TestCase
{
protected $tmpFile;
/** @var false|null|string */
private $orgFile;

/** @var mixed */
private $tmpFile;

protected function setUp() : void
{
$this->tmpfile = null;
$this->tmpFile = null;
$this->orgFile = null;
}

protected function tearDown() : void
{
if (is_string($this->tmpFile) && file_exists($this->tmpFile)) {
unlink($this->tmpFile);
}

if (is_string($this->orgFile) && file_exists($this->orgFile)) {
unlink($this->orgFile);
}
}

public function invalidStreams()
/** @return non-empty-array<non-empty-string, array{mixed}> */
public function invalidStreams(): array
{
return [
'null' => [null],
Expand All @@ -65,22 +75,24 @@ public function invalidStreams()

/**
* @dataProvider invalidStreams
* @param mixed $streamOrFile
*/
public function testRaisesExceptionOnInvalidStreamOrFile($streamOrFile)
public function testRaisesExceptionOnInvalidStreamOrFile($streamOrFile): void
{
$this->expectException(InvalidArgumentException::class);

new UploadedFile($streamOrFile, 0, UPLOAD_ERR_OK);
}

public function testValidSize()
public function testValidSize(): void
{
$uploaded = new UploadedFile(fopen('php://temp', 'wb+'), 123, UPLOAD_ERR_OK);

$this->assertSame(123, $uploaded->getSize());
}

public function invalidErrorStatuses()
/** @return non-empty-array<non-empty-string, array{int}> */
public function invalidErrorStatuses(): array
{
return [
'negative' => [-1],
Expand All @@ -91,48 +103,48 @@ public function invalidErrorStatuses()
/**
* @dataProvider invalidErrorStatuses
*/
public function testRaisesExceptionOnInvalidErrorStatus($status)
public function testRaisesExceptionOnInvalidErrorStatus(int $status): void
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('status');

new UploadedFile(fopen('php://temp', 'wb+'), 0, $status);
}

public function testValidClientFilename()
public function testValidClientFilename(): void
{
$file = new UploadedFile(fopen('php://temp', 'wb+'), 0, UPLOAD_ERR_OK, 'boo.txt');
$this->assertSame('boo.txt', $file->getClientFilename());
}

public function testValidNullClientFilename()
public function testValidNullClientFilename(): void
{
$file = new UploadedFile(fopen('php://temp', 'wb+'), 0, UPLOAD_ERR_OK, null);
$this->assertSame(null, $file->getClientFilename());
}

public function testValidClientMediaType()
public function testValidClientMediaType(): void
{
$file = new UploadedFile(fopen('php://temp', 'wb+'), 0, UPLOAD_ERR_OK, 'foobar.baz', 'mediatype');
$this->assertSame('mediatype', $file->getClientMediaType());
}

public function testGetStreamReturnsOriginalStreamObject()
public function testGetStreamReturnsOriginalStreamObject(): void
{
$stream = new Stream('php://temp');
$upload = new UploadedFile($stream, 0, UPLOAD_ERR_OK);
$this->assertSame($stream, $upload->getStream());
}

public function testGetStreamReturnsWrappedPhpStream()
public function testGetStreamReturnsWrappedPhpStream(): void
{
$stream = fopen('php://temp', 'wb+');
$upload = new UploadedFile($stream, 0, UPLOAD_ERR_OK);
$uploadStream = $upload->getStream()->detach();
$this->assertSame($stream, $uploadStream);
}

public function testGetStreamReturnsStreamForFile()
public function testGetStreamReturnsStreamForFile(): void
{
$this->tmpFile = $stream = tempnam(sys_get_temp_dir(), 'diac');
$upload = new UploadedFile($stream, 0, UPLOAD_ERR_OK);
Expand All @@ -142,20 +154,22 @@ public function testGetStreamReturnsStreamForFile()
$this->assertSame($stream, $r->getValue($uploadStream));
}

public function testMovesFileToDesignatedPath()
public function testMovesFileToDesignatedPath(): void
{
$originalContents = 'Foo bar!';
$stream = new Stream('php://temp', 'wb+');
$stream->write('Foo bar!');
$stream->write($originalContents);
$upload = new UploadedFile($stream, 0, UPLOAD_ERR_OK);

$this->tmpFile = $to = tempnam(sys_get_temp_dir(), 'diac');
$upload->moveTo($to);
$this->assertTrue(file_exists($to));
$contents = file_get_contents($to);
$this->assertSame($stream->__toString(), $contents);
$this->assertSame($originalContents, $contents);
}

public function invalidMovePaths()
/** @return non-empty-array<non-empty-string, array{mixed}> */
public function invalidMovePaths(): array
{
return [
'null' => [null],
Expand All @@ -171,8 +185,10 @@ public function invalidMovePaths()

/**
* @dataProvider invalidMovePaths
*
* @param mixed $path
*/
public function testMoveRaisesExceptionForInvalidPath($path)
public function testMoveRaisesExceptionForInvalidPath($path): void
{
$stream = new Stream('php://temp', 'wb+');
$stream->write('Foo bar!');
Expand All @@ -186,7 +202,7 @@ public function testMoveRaisesExceptionForInvalidPath($path)
$upload->moveTo($path);
}

public function testMoveCannotBeCalledMoreThanOnce()
public function testMoveCannotBeCalledMoreThanOnce(): void
{
$stream = new Stream('php://temp', 'wb+');
$stream->write('Foo bar!');
Expand All @@ -202,7 +218,7 @@ public function testMoveCannotBeCalledMoreThanOnce()
$upload->moveTo($to);
}

public function testCannotRetrieveStreamAfterMove()
public function testCannotRetrieveStreamAfterMove(): void
{
$stream = new Stream('php://temp', 'wb+');
$stream->write('Foo bar!');
Expand All @@ -218,7 +234,8 @@ public function testCannotRetrieveStreamAfterMove()
$upload->getStream();
}

public function nonOkErrorStatus()
/** @return non-empty-array<non-empty-string, array{positive-int}> */
public function nonOkErrorStatus(): array
{
return [
'UPLOAD_ERR_INI_SIZE' => [ UPLOAD_ERR_INI_SIZE ],
Expand All @@ -235,7 +252,7 @@ public function nonOkErrorStatus()
* @dataProvider nonOkErrorStatus
* @group 60
*/
public function testConstructorDoesNotRaiseExceptionForInvalidStreamWhenErrorStatusPresent($status)
public function testConstructorDoesNotRaiseExceptionForInvalidStreamWhenErrorStatusPresent(int $status): void
{
$uploadedFile = new UploadedFile('not ok', 0, $status);
$this->assertSame($status, $uploadedFile->getError());
Expand All @@ -245,7 +262,7 @@ public function testConstructorDoesNotRaiseExceptionForInvalidStreamWhenErrorSta
* @dataProvider nonOkErrorStatus
* @group 60
*/
public function testMoveToRaisesExceptionWhenErrorStatusPresent($status)
public function testMoveToRaisesExceptionWhenErrorStatusPresent(int $status): void
{
$uploadedFile = new UploadedFile('not ok', 0, $status);

Expand All @@ -259,7 +276,7 @@ public function testMoveToRaisesExceptionWhenErrorStatusPresent($status)
* @dataProvider nonOkErrorStatus
* @group 60
*/
public function testGetStreamRaisesExceptionWhenErrorStatusPresent($status)
public function testGetStreamRaisesExceptionWhenErrorStatusPresent(int $status): void
{
$uploadedFile = new UploadedFile('not ok', 0, $status);

Expand All @@ -272,20 +289,24 @@ public function testGetStreamRaisesExceptionWhenErrorStatusPresent($status)
/**
* @group 82
*/
public function testMoveToCreatesStreamIfOnlyAFilenameWasProvided()
public function testMoveToCreatesStreamIfOnlyAFilenameWasProvided(): void
{
$this->orgFile = tempnam(sys_get_temp_dir(), 'ORG');
$this->tmpFile = tempnam(sys_get_temp_dir(), 'DIA');
file_put_contents($this->orgFile, 'Hello');

$original = file_get_contents($this->orgFile);

$uploadedFile = new UploadedFile(__FILE__, 100, UPLOAD_ERR_OK, basename(__FILE__), 'text/plain');
$uploadedFile = new UploadedFile($this->orgFile, 100, UPLOAD_ERR_OK, basename($this->orgFile), 'text/plain');
$uploadedFile->moveTo($this->tmpFile);

$original = file_get_contents(__FILE__);
$test = file_get_contents($this->tmpFile);
$contents = file_get_contents($this->tmpFile);

$this->assertSame($original, $test);
$this->assertSame($original, $contents);
}

public function errorConstantsAndMessages()
/** @return iterable<int, array{int, non-empty-string}> */
public function errorConstantsAndMessages(): iterable
{
foreach (UploadedFile::ERROR_MESSAGES as $constant => $message) {
if ($constant === UPLOAD_ERR_OK) {
Expand All @@ -295,13 +316,11 @@ public function errorConstantsAndMessages()
}
}

/**
* @dataProvider errorConstantsAndMessages
* @param int $constant Upload error constant
* @param string $message Associated error message
*/
public function testGetStreamRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected($constant, $message)
{
/** @dataProvider errorConstantsAndMessages */
public function testGetStreamRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected(
int $constant,
string $message
): void {
$uploadedFile = new UploadedFile(__FILE__, 100, $constant);
$this->expectException(RuntimeException::class);
$this->expectExceptionMessage($message);
Expand All @@ -310,14 +329,26 @@ public function testGetStreamRaisesExceptionWithAppropriateMessageWhenUploadErro

/**
* @dataProvider errorConstantsAndMessages
* @param int $constant Upload error constant
* @param string $message Associated error message
*/
public function testMoveToRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected($constant, $message)
{
public function testMoveToRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected(
int $constant,
string $message
): void {
$uploadedFile = new UploadedFile(__FILE__, 100, $constant);
$this->expectException(RuntimeException::class);
$this->expectExceptionMessage($message);
$uploadedFile->moveTo('/tmp/foo');
}

public function testMoveToInCLIShouldRemoveOriginalFile(): void
{
$this->orgFile = tempnam(sys_get_temp_dir(), 'ORG');
file_put_contents($this->orgFile, 'Hello');
$upload = new UploadedFile($this->orgFile, 0, UPLOAD_ERR_OK);

$this->tmpFile = $to = tempnam(sys_get_temp_dir(), 'diac');
$upload->moveTo($to);
$this->assertFalse(file_exists($this->orgFile));
$this->assertTrue(file_exists($to));
}
}

0 comments on commit 1f97b0c

Please sign in to comment.