From 5773e7646ee29051b1d3640fba787357bd0c454d Mon Sep 17 00:00:00 2001 From: katsuren Date: Mon, 4 Jul 2022 16:56:17 +0900 Subject: [PATCH 1/3] fixed UploadedFile::moveTo doesn't work by cli. Original file leave it as is. Signed-off-by: katsuren --- src/UploadedFile.php | 7 ++++++ test/UploadedFileTest.php | 45 +++++++++++++++++++++++++++++++++------ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/UploadedFile.php b/src/UploadedFile.php index 93980f30..93e94195 100644 --- a/src/UploadedFile.php +++ b/src/UploadedFile.php @@ -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 diff --git a/test/UploadedFileTest.php b/test/UploadedFileTest.php index f7518862..c741b838 100644 --- a/test/UploadedFileTest.php +++ b/test/UploadedFileTest.php @@ -32,11 +32,15 @@ class UploadedFileTest extends TestCase { + /** @var false|null|string */ + protected $orgFile; + protected $tmpFile; protected function setUp() : void { - $this->tmpfile = null; + $this->tmpFile = null; + $this->orgFile = null; } protected function tearDown() : void @@ -44,6 +48,10 @@ 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() @@ -142,17 +150,21 @@ public function testGetStreamReturnsStreamForFile() $this->assertSame($stream, $r->getValue($uploadStream)); } + /** + * @return void + */ public function testMovesFileToDesignatedPath() { + $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() @@ -271,18 +283,22 @@ public function testGetStreamRaisesExceptionWhenErrorStatusPresent($status) /** * @group 82 + * @return void */ public function testMoveToCreatesStreamIfOnlyAFilenameWasProvided() { + $this->orgFile = tempnam(sys_get_temp_dir(), 'ORG'); $this->tmpFile = tempnam(sys_get_temp_dir(), 'DIA'); + file_put_contents($this->orgFile, 'Hello'); - $uploadedFile = new UploadedFile(__FILE__, 100, UPLOAD_ERR_OK, basename(__FILE__), 'text/plain'); + $original = file_get_contents($this->orgFile); + + $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() @@ -320,4 +336,19 @@ public function testMoveToRaisesExceptionWithAppropriateMessageWhenUploadErrorDe $this->expectExceptionMessage($message); $uploadedFile->moveTo('/tmp/foo'); } + + /** + * @return void + */ + public function testMoveToInCLIShouldRemoveOriginalFile() + { + $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)); + } } From 197f195ef75dd8db528a459154ba917c0fd644b9 Mon Sep 17 00:00:00 2001 From: katsuren Date: Mon, 4 Jul 2022 22:49:43 +0900 Subject: [PATCH 2/3] cleanup file regardless of the stream Signed-off-by: katsuren --- src/UploadedFile.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/UploadedFile.php b/src/UploadedFile.php index 93e94195..e918697f 100644 --- a/src/UploadedFile.php +++ b/src/UploadedFile.php @@ -190,9 +190,9 @@ public function moveTo($targetPath) : void if ($this->stream instanceof StreamInterface) { $this->stream->close(); - if (is_string($this->file) && file_exists($this->file)) { - unlink($this->file); - } + } + if (is_string($this->file) && file_exists($this->file)) { + unlink($this->file); } break; default: From 091487a175a4748aa7edb3f24f69f695f8bdb925 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 6 Jul 2022 11:21:17 +0200 Subject: [PATCH 3/3] Cleaned up `UploadedFileTest` by providing return types and parameter types everywhere --- psalm-baseline.xml | 46 +------------------- test/UploadedFileTest.php | 88 +++++++++++++++++++-------------------- 2 files changed, 45 insertions(+), 89 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 1456fb12..47be887f 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1164,54 +1164,10 @@ - - $path - $status - $status - $status - $status - $streamOrFile - - - $tmpFile - - - errorConstantsAndMessages - invalidErrorStatuses - invalidMovePaths - invalidStreams - nonOkErrorStatus - testCannotRetrieveStreamAfterMove - testConstructorDoesNotRaiseExceptionForInvalidStreamWhenErrorStatusPresent - testGetStreamRaisesExceptionWhenErrorStatusPresent - testGetStreamRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected - testGetStreamReturnsOriginalStreamObject - testGetStreamReturnsStreamForFile - testGetStreamReturnsWrappedPhpStream - testMoveCannotBeCalledMoreThanOnce - testMoveRaisesExceptionForInvalidPath - testMoveToCreatesStreamIfOnlyAFilenameWasProvided - testMoveToRaisesExceptionWhenErrorStatusPresent - testMoveToRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected - testMovesFileToDesignatedPath - testRaisesExceptionOnInvalidErrorStatus - testRaisesExceptionOnInvalidStreamOrFile - testValidClientFilename - testValidClientMediaType - testValidNullClientFilename - testValidSize - - + $path - $status - $status - $status - $status $streamOrFile - - $this->tmpfile - diff --git a/test/UploadedFileTest.php b/test/UploadedFileTest.php index c741b838..0b2459ed 100644 --- a/test/UploadedFileTest.php +++ b/test/UploadedFileTest.php @@ -30,12 +30,13 @@ use const UPLOAD_ERR_OK; use const UPLOAD_ERR_PARTIAL; -class UploadedFileTest extends TestCase +final class UploadedFileTest extends TestCase { /** @var false|null|string */ - protected $orgFile; + private $orgFile; - protected $tmpFile; + /** @var mixed */ + private $tmpFile; protected function setUp() : void { @@ -54,7 +55,8 @@ protected function tearDown() : void } } - public function invalidStreams() + /** @return non-empty-array */ + public function invalidStreams(): array { return [ 'null' => [null], @@ -73,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 */ + public function invalidErrorStatuses(): array { return [ 'negative' => [-1], @@ -99,7 +103,7 @@ public function invalidErrorStatuses() /** * @dataProvider invalidErrorStatuses */ - public function testRaisesExceptionOnInvalidErrorStatus($status) + public function testRaisesExceptionOnInvalidErrorStatus(int $status): void { $this->expectException(InvalidArgumentException::class); $this->expectExceptionMessage('status'); @@ -107,32 +111,32 @@ public function testRaisesExceptionOnInvalidErrorStatus($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); @@ -140,7 +144,7 @@ public function testGetStreamReturnsWrappedPhpStream() $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); @@ -150,10 +154,7 @@ public function testGetStreamReturnsStreamForFile() $this->assertSame($stream, $r->getValue($uploadStream)); } - /** - * @return void - */ - public function testMovesFileToDesignatedPath() + public function testMovesFileToDesignatedPath(): void { $originalContents = 'Foo bar!'; $stream = new Stream('php://temp', 'wb+'); @@ -167,7 +168,8 @@ public function testMovesFileToDesignatedPath() $this->assertSame($originalContents, $contents); } - public function invalidMovePaths() + /** @return non-empty-array */ + public function invalidMovePaths(): array { return [ 'null' => [null], @@ -183,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!'); @@ -198,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!'); @@ -214,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!'); @@ -230,7 +234,8 @@ public function testCannotRetrieveStreamAfterMove() $upload->getStream(); } - public function nonOkErrorStatus() + /** @return non-empty-array */ + public function nonOkErrorStatus(): array { return [ 'UPLOAD_ERR_INI_SIZE' => [ UPLOAD_ERR_INI_SIZE ], @@ -247,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()); @@ -257,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); @@ -271,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); @@ -283,9 +288,8 @@ public function testGetStreamRaisesExceptionWhenErrorStatusPresent($status) /** * @group 82 - * @return void */ - public function testMoveToCreatesStreamIfOnlyAFilenameWasProvided() + public function testMoveToCreatesStreamIfOnlyAFilenameWasProvided(): void { $this->orgFile = tempnam(sys_get_temp_dir(), 'ORG'); $this->tmpFile = tempnam(sys_get_temp_dir(), 'DIA'); @@ -301,7 +305,8 @@ public function testMoveToCreatesStreamIfOnlyAFilenameWasProvided() $this->assertSame($original, $contents); } - public function errorConstantsAndMessages() + /** @return iterable */ + public function errorConstantsAndMessages(): iterable { foreach (UploadedFile::ERROR_MESSAGES as $constant => $message) { if ($constant === UPLOAD_ERR_OK) { @@ -311,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); @@ -326,21 +329,18 @@ 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'); } - /** - * @return void - */ - public function testMoveToInCLIShouldRemoveOriginalFile() + public function testMoveToInCLIShouldRemoveOriginalFile(): void { $this->orgFile = tempnam(sys_get_temp_dir(), 'ORG'); file_put_contents($this->orgFile, 'Hello');