[HttpFoundation] When calling UploadedFile::getErrorMessage() to a file which has no error and is uploaded successfully, it should not return an error #54304
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
carsonbot
changed the title
|
I fear this contradict the implementation of the move() method If /**
* Moves the file to a new location.
*
* @throws FileException if, for any reason, the file could not have been moved
*/
public function move(string $directory, ?string $name = null): File
{
if ($this->isValid()) { |
|
Not sure what you mean @smnandre, can you elaborate or share a test case? |
public function testInvalidFile()
{
$this->expectException(FileException::class);
$this->expectExceptionMessage('The file "original.gif" was not uploaded due to an unknown error.');
$file = new UploadedFile(
__DIR__.'/Fixtures/test.gif',
'original.gif',
'image/gif',
);
$file->move(__DIR__.'/Fixtures/directory');
}Testing Symfony\Component\HttpFoundation\Tests\File\UploadedFileTest
...................F.............. 34 / 34 (100%)
Time: 00:00.024, Memory: 12.00 MB
There was 1 failure:
1) Symfony\Component\HttpFoundation\Tests\File\UploadedFileTest::testInvalidFile
- Failed asserting that exception message '' contains 'The file "original.gif" was not uploaded due to an unknown error.'. |
|
The error message is used as exception message, so it seems to me this code was a failsafe if "something went wrong". |
|
I guess we should add a new case at the end of |
|
I'm wondering if we can handle this very-specific use case in another way, because your changes make sense and are easier to "read" than this special "no error but an error" case. If no, your solution seems very sensible yes! |
|
Thanks @smnandre for the review! I'm considering renaming the original method "getErrorMessage()" to private "getExceptionMessage()" and creating a new "getErrorMessage()" method, which will check if "$this->error === \UPLOAD_ERR_OK", and if it does, it will return null (or an empty string to ensure backward compatibility); otherwise, it will internally call "getExceptionMessage()". Then, in the "move()" method, we can utilize the "getExceptionMessage()" method. What do you think? P.S. I'll also add your test case to the UploadedFileTest.php. |
|
Seems perfect to me. |
…rror and is uploaded successfully, it should not return an error
|
Thanks @smnandre! |
When calling
Symfony\Component\HttpFoundation\File\UploadedFile::getErrorMessage()for an uploaded file which has no error and is uploaded successfully, should not give any error message.Before this changes it returned The file "%s" was not uploaded due to an unknown error..
After this changes, it will return an empty string.