Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (seehttps://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
• Features and deprecations must be submitted against the 7.1 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

I fear this contradict the implementation of themove() method

Ifthis->isValid() returnsfalse, with your change the method would throw an exception with no message.

/**     * Moves the file to a new location.     *     * @throws FileException if, for any reason, the file could not have been moved     */publicfunction move(string$directory, ?string$name =null):File    {if ($this->isValid()) {

Not sure what you mean@smnandre, can you elaborate or share a test case?

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 ofmove method to handle\UPLOAD_ERR_OK error, in order to have an empty string fromUploadedFile::getErrorMessage(), but a legit exception message thrown bymove method on the use case provided by@smnandre. WDYT?

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.

@nicolas-grekas ?

Thanks@smnandre!
@nicolas-grekas, what do you think about these changes?