Skip to content

Commit 1f97b0c

Browse files
OcramiusOcramius
authored
Merge pull request #98 from k2rn/fixed_UploadedFile_moveTo
Fixed `UploadedFile::moveTo()` so it actually removes the original file when used in CLI context, and doesn't leave orphaned files
2 parents 78846cb + 091487a commit 1f97b0c

File tree

3 files changed

+81
-87
lines changed

3 files changed

+81
-87
lines changed

‎psalm-baseline.xml‎

Lines changed: 1 addition & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1164,54 +1164,10 @@
11641164
</MissingReturnType>
11651165
</file>
11661166
<file src="test/UploadedFileTest.php">
1167-
<MissingParamType occurrences="6">
1168-
<code>$path</code>
1169-
<code>$status</code>
1170-
<code>$status</code>
1171-
<code>$status</code>
1172-
<code>$status</code>
1173-
<code>$streamOrFile</code>
1174-
</MissingParamType>
1175-
<MissingPropertyType occurrences="1">
1176-
<code>$tmpFile</code>
1177-
</MissingPropertyType>
1178-
<MissingReturnType occurrences="24">
1179-
<code>errorConstantsAndMessages</code>
1180-
<code>invalidErrorStatuses</code>
1181-
<code>invalidMovePaths</code>
1182-
<code>invalidStreams</code>
1183-
<code>nonOkErrorStatus</code>
1184-
<code>testCannotRetrieveStreamAfterMove</code>
1185-
<code>testConstructorDoesNotRaiseExceptionForInvalidStreamWhenErrorStatusPresent</code>
1186-
<code>testGetStreamRaisesExceptionWhenErrorStatusPresent</code>
1187-
<code>testGetStreamRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected</code>
1188-
<code>testGetStreamReturnsOriginalStreamObject</code>
1189-
<code>testGetStreamReturnsStreamForFile</code>
1190-
<code>testGetStreamReturnsWrappedPhpStream</code>
1191-
<code>testMoveCannotBeCalledMoreThanOnce</code>
1192-
<code>testMoveRaisesExceptionForInvalidPath</code>
1193-
<code>testMoveToCreatesStreamIfOnlyAFilenameWasProvided</code>
1194-
<code>testMoveToRaisesExceptionWhenErrorStatusPresent</code>
1195-
<code>testMoveToRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected</code>
1196-
<code>testMovesFileToDesignatedPath</code>
1197-
<code>testRaisesExceptionOnInvalidErrorStatus</code>
1198-
<code>testRaisesExceptionOnInvalidStreamOrFile</code>
1199-
<code>testValidClientFilename</code>
1200-
<code>testValidClientMediaType</code>
1201-
<code>testValidNullClientFilename</code>
1202-
<code>testValidSize</code>
1203-
</MissingReturnType>
1204-
<MixedArgument occurrences="6">
1167+
<MixedArgument occurrences="2">
12051168
<code>$path</code>
1206-
<code>$status</code>
1207-
<code>$status</code>
1208-
<code>$status</code>
1209-
<code>$status</code>
12101169
<code>$streamOrFile</code>
12111170
</MixedArgument>
1212-
<UndefinedThisPropertyAssignment occurrences="1">
1213-
<code>$this-&gt;tmpfile</code>
1214-
</UndefinedThisPropertyAssignment>
12151171
</file>
12161172
<file src="test/UriTest.php">
12171173
<InvalidScalarArgument occurrences="2">

‎src/UploadedFile.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,13 @@ public function moveTo($targetPath) : void
187187
case (empty($sapi) || 0 === strpos($sapi, 'cli') || 0 === strpos($sapi, 'phpdbg') || ! $this->file):
188188
// Non-SAPI environment, or no filename present
189189
$this->writeFile($targetPath);
190+
191+
if ($this->stream instanceof StreamInterface) {
192+
$this->stream->close();
193+
}
194+
if (is_string($this->file) && file_exists($this->file)) {
195+
unlink($this->file);
196+
}
190197
break;
191198
default:
192199
// SAPI environment, with file present

‎test/UploadedFileTest.php‎

Lines changed: 73 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -30,23 +30,33 @@
3030
use const UPLOAD_ERR_OK;
3131
use const UPLOAD_ERR_PARTIAL;
3232

33-
class UploadedFileTest extends TestCase
33+
final class UploadedFileTest extends TestCase
3434
{
35-
protected $tmpFile;
35+
/** @var false|null|string */
36+
private $orgFile;
37+
38+
/** @var mixed */
39+
private $tmpFile;
3640

3741
protected function setUp() : void
3842
{
39-
$this->tmpfile = null;
43+
$this->tmpFile = null;
44+
$this->orgFile = null;
4045
}
4146

4247
protected function tearDown() : void
4348
{
4449
if (is_string($this->tmpFile) && file_exists($this->tmpFile)) {
4550
unlink($this->tmpFile);
4651
}
52+
53+
if (is_string($this->orgFile) && file_exists($this->orgFile)) {
54+
unlink($this->orgFile);
55+
}
4756
}
4857

49-
public function invalidStreams()
58+
/** @return non-empty-array<non-empty-string, array{mixed}> */
59+
public function invalidStreams(): array
5060
{
5161
return [
5262
'null' => [null],
@@ -65,22 +75,24 @@ public function invalidStreams()
6575

6676
/**
6777
* @dataProvider invalidStreams
78+
* @param mixed $streamOrFile
6879
*/
69-
public function testRaisesExceptionOnInvalidStreamOrFile($streamOrFile)
80+
public function testRaisesExceptionOnInvalidStreamOrFile($streamOrFile): void
7081
{
7182
$this->expectException(InvalidArgumentException::class);
7283

7384
new UploadedFile($streamOrFile, 0, UPLOAD_ERR_OK);
7485
}
7586

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

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

83-
public function invalidErrorStatuses()
94+
/** @return non-empty-array<non-empty-string, array{int}> */
95+
public function invalidErrorStatuses(): array
8496
{
8597
return [
8698
'negative' => [-1],
@@ -91,48 +103,48 @@ public function invalidErrorStatuses()
91103
/**
92104
* @dataProvider invalidErrorStatuses
93105
*/
94-
public function testRaisesExceptionOnInvalidErrorStatus($status)
106+
public function testRaisesExceptionOnInvalidErrorStatus(int $status): void
95107
{
96108
$this->expectException(InvalidArgumentException::class);
97109
$this->expectExceptionMessage('status');
98110

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

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

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

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

120-
public function testGetStreamReturnsOriginalStreamObject()
132+
public function testGetStreamReturnsOriginalStreamObject(): void
121133
{
122134
$stream = new Stream('php://temp');
123135
$upload = new UploadedFile($stream, 0, UPLOAD_ERR_OK);
124136
$this->assertSame($stream, $upload->getStream());
125137
}
126138

127-
public function testGetStreamReturnsWrappedPhpStream()
139+
public function testGetStreamReturnsWrappedPhpStream(): void
128140
{
129141
$stream = fopen('php://temp', 'wb+');
130142
$upload = new UploadedFile($stream, 0, UPLOAD_ERR_OK);
131143
$uploadStream = $upload->getStream()->detach();
132144
$this->assertSame($stream, $uploadStream);
133145
}
134146

135-
public function testGetStreamReturnsStreamForFile()
147+
public function testGetStreamReturnsStreamForFile(): void
136148
{
137149
$this->tmpFile = $stream = tempnam(sys_get_temp_dir(), 'diac');
138150
$upload = new UploadedFile($stream, 0, UPLOAD_ERR_OK);
@@ -142,20 +154,22 @@ public function testGetStreamReturnsStreamForFile()
142154
$this->assertSame($stream, $r->getValue($uploadStream));
143155
}
144156

145-
public function testMovesFileToDesignatedPath()
157+
public function testMovesFileToDesignatedPath(): void
146158
{
159+
$originalContents = 'Foo bar!';
147160
$stream = new Stream('php://temp', 'wb+');
148-
$stream->write('Foo bar!');
161+
$stream->write($originalContents);
149162
$upload = new UploadedFile($stream, 0, UPLOAD_ERR_OK);
150163

151164
$this->tmpFile = $to = tempnam(sys_get_temp_dir(), 'diac');
152165
$upload->moveTo($to);
153166
$this->assertTrue(file_exists($to));
154167
$contents = file_get_contents($to);
155-
$this->assertSame($stream->__toString(), $contents);
168+
$this->assertSame($originalContents, $contents);
156169
}
157170

158-
public function invalidMovePaths()
171+
/** @return non-empty-array<non-empty-string, array{mixed}> */
172+
public function invalidMovePaths(): array
159173
{
160174
return [
161175
'null' => [null],
@@ -171,8 +185,10 @@ public function invalidMovePaths()
171185

172186
/**
173187
* @dataProvider invalidMovePaths
188+
*
189+
* @param mixed $path
174190
*/
175-
public function testMoveRaisesExceptionForInvalidPath($path)
191+
public function testMoveRaisesExceptionForInvalidPath($path): void
176192
{
177193
$stream = new Stream('php://temp', 'wb+');
178194
$stream->write('Foo bar!');
@@ -186,7 +202,7 @@ public function testMoveRaisesExceptionForInvalidPath($path)
186202
$upload->moveTo($path);
187203
}
188204

189-
public function testMoveCannotBeCalledMoreThanOnce()
205+
public function testMoveCannotBeCalledMoreThanOnce(): void
190206
{
191207
$stream = new Stream('php://temp', 'wb+');
192208
$stream->write('Foo bar!');
@@ -202,7 +218,7 @@ public function testMoveCannotBeCalledMoreThanOnce()
202218
$upload->moveTo($to);
203219
}
204220

205-
public function testCannotRetrieveStreamAfterMove()
221+
public function testCannotRetrieveStreamAfterMove(): void
206222
{
207223
$stream = new Stream('php://temp', 'wb+');
208224
$stream->write('Foo bar!');
@@ -218,7 +234,8 @@ public function testCannotRetrieveStreamAfterMove()
218234
$upload->getStream();
219235
}
220236

221-
public function nonOkErrorStatus()
237+
/** @return non-empty-array<non-empty-string, array{positive-int}> */
238+
public function nonOkErrorStatus(): array
222239
{
223240
return [
224241
'UPLOAD_ERR_INI_SIZE' => [ UPLOAD_ERR_INI_SIZE ],
@@ -235,7 +252,7 @@ public function nonOkErrorStatus()
235252
* @dataProvider nonOkErrorStatus
236253
* @group 60
237254
*/
238-
public function testConstructorDoesNotRaiseExceptionForInvalidStreamWhenErrorStatusPresent($status)
255+
public function testConstructorDoesNotRaiseExceptionForInvalidStreamWhenErrorStatusPresent(int $status): void
239256
{
240257
$uploadedFile = new UploadedFile('not ok', 0, $status);
241258
$this->assertSame($status, $uploadedFile->getError());
@@ -245,7 +262,7 @@ public function testConstructorDoesNotRaiseExceptionForInvalidStreamWhenErrorSta
245262
* @dataProvider nonOkErrorStatus
246263
* @group 60
247264
*/
248-
public function testMoveToRaisesExceptionWhenErrorStatusPresent($status)
265+
public function testMoveToRaisesExceptionWhenErrorStatusPresent(int $status): void
249266
{
250267
$uploadedFile = new UploadedFile('not ok', 0, $status);
251268

@@ -259,7 +276,7 @@ public function testMoveToRaisesExceptionWhenErrorStatusPresent($status)
259276
* @dataProvider nonOkErrorStatus
260277
* @group 60
261278
*/
262-
public function testGetStreamRaisesExceptionWhenErrorStatusPresent($status)
279+
public function testGetStreamRaisesExceptionWhenErrorStatusPresent(int $status): void
263280
{
264281
$uploadedFile = new UploadedFile('not ok', 0, $status);
265282

@@ -272,20 +289,24 @@ public function testGetStreamRaisesExceptionWhenErrorStatusPresent($status)
272289
/**
273290
* @group 82
274291
*/
275-
public function testMoveToCreatesStreamIfOnlyAFilenameWasProvided()
292+
public function testMoveToCreatesStreamIfOnlyAFilenameWasProvided(): void
276293
{
294+
$this->orgFile = tempnam(sys_get_temp_dir(), 'ORG');
277295
$this->tmpFile = tempnam(sys_get_temp_dir(), 'DIA');
296+
file_put_contents($this->orgFile, 'Hello');
297+
298+
$original = file_get_contents($this->orgFile);
278299

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

282-
$original = file_get_contents(__FILE__);
283-
$test = file_get_contents($this->tmpFile);
303+
$contents = file_get_contents($this->tmpFile);
284304

285-
$this->assertSame($original, $test);
305+
$this->assertSame($original, $contents);
286306
}
287307

288-
public function errorConstantsAndMessages()
308+
/** @return iterable<int, array{int, non-empty-string}> */
309+
public function errorConstantsAndMessages(): iterable
289310
{
290311
foreach (UploadedFile::ERROR_MESSAGES as $constant => $message) {
291312
if ($constant === UPLOAD_ERR_OK) {
@@ -295,13 +316,11 @@ public function errorConstantsAndMessages()
295316
}
296317
}
297318

298-
/**
299-
* @dataProvider errorConstantsAndMessages
300-
* @param int $constant Upload error constant
301-
* @param string $message Associated error message
302-
*/
303-
public function testGetStreamRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected($constant, $message)
304-
{
319+
/** @dataProvider errorConstantsAndMessages */
320+
public function testGetStreamRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected(
321+
int $constant,
322+
string $message
323+
): void {
305324
$uploadedFile = new UploadedFile(__FILE__, 100, $constant);
306325
$this->expectException(RuntimeException::class);
307326
$this->expectExceptionMessage($message);
@@ -310,14 +329,26 @@ public function testGetStreamRaisesExceptionWithAppropriateMessageWhenUploadErro
310329

311330
/**
312331
* @dataProvider errorConstantsAndMessages
313-
* @param int $constant Upload error constant
314-
* @param string $message Associated error message
315332
*/
316-
public function testMoveToRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected($constant, $message)
317-
{
333+
public function testMoveToRaisesExceptionWithAppropriateMessageWhenUploadErrorDetected(
334+
int $constant,
335+
string $message
336+
): void {
318337
$uploadedFile = new UploadedFile(__FILE__, 100, $constant);
319338
$this->expectException(RuntimeException::class);
320339
$this->expectExceptionMessage($message);
321340
$uploadedFile->moveTo('/tmp/foo');
322341
}
342+
343+
public function testMoveToInCLIShouldRemoveOriginalFile(): void
344+
{
345+
$this->orgFile = tempnam(sys_get_temp_dir(), 'ORG');
346+
file_put_contents($this->orgFile, 'Hello');
347+
$upload = new UploadedFile($this->orgFile, 0, UPLOAD_ERR_OK);
348+
349+
$this->tmpFile = $to = tempnam(sys_get_temp_dir(), 'diac');
350+
$upload->moveTo($to);
351+
$this->assertFalse(file_exists($this->orgFile));
352+
$this->assertTrue(file_exists($to));
353+
}
323354
}

0 commit comments

Comments
 (0)