Fix UploadedFile::move() $this->hasMoved value

[diegoscoponi]

The move method always assigns a true value to the $this->hasMoved attribute regardless of whether move_uploaded_file has failed, this means that calling $file->hasMoved() always returns true.
I'm right?
Thanks!!!

[kenjis]

It seems even if move_uploaded_file() only returns false, $this->hasMoved may be set to true.

https://www.php.net/manual/en/function.move-uploaded-file.php#refsect1-function.move-uploaded-file-returnvalues

Return Values ¶
Returns true on success.
If from is not a valid upload file, then no action will occur, and move_uploaded_file() will return false.
If from is a valid upload file, but cannot be moved for some reason, no action will occur, and move_uploaded_file() will return false. Additionally, a warning will be issued.

[kenjis]

@diegoscoponi Thank you for PR!

We ask that contributions have code commits signed.
Could you sign?

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

[kenjis]

Coding style checking failed.
https://github.com/codeigniter4/CodeIgniter4/runs/4128729743?check_suite_focus=true

Could you fix the style?
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#php-style

[kenjis]

PHPUnit testing failed.

https://github.com/codeigniter4/CodeIgniter4/runs/4128729664?check_suite_focus=true

There was 1 error:

1) CodeIgniter\HTTP\Files\FileMovingTest::testMoved
TypeError: Return value of CodeIgniter\HTTP\Files\UploadedFile::hasMoved() must be of the type bool, null returned

/home/runner/work/CodeIgniter4/CodeIgniter4/system/HTTP/Files/UploadedFile.php:188
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/HTTP/Files/FileMovingTest.php:176

Could you fix the broken test?

[kenjis]

Fix the failed checks in GitHub Actions, please.

If you have any questions, feel free to ask.

[sfadschm]

I might be mistaken, but if move_uploaded_file fails, the current code throws the exception so this->hasMoved willl never be set to true in that case
I am not sure if this is really a problem with the current code.

[MGatner]

@sfadschm it looks like that is true some of the time, but move_uploaded_file() appears to be able to fail quietly as well. I think this PR is indeed needed as long as the issues can be addressed.

[sfadschm]

@sfadschm it looks like that is true some of the time, but move_uploaded_file() appears to be able to fail quietly as well. I think this PR is indeed needed as long as the issues can be addressed.

Thanks, I did not grasp that. Does 'fail silently' mean that the method simy returns false? If so, we would maybe like to throw the same exception for that case as in the catch block to have consistent output in case of failure?

[kenjis]

we would maybe like to throw the same exception for that case as in the catch block to have consistent output in case of failure?

Yes.

[kenjis]

@diegoscoponi
Thank you for signing.

You don't use git merge, use git rebase instead, please.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#pushing-your-branch

Please fix two checking failed. See
#5290 (comment)
#5290 (comment)

And see #5290 (comment)

[kenjis]

Please fix the above.

[kenjis]

When move_uploaded_file() returns false, the current code returns false after all.
But we need to throw HTTPException::forMoveFailed() as the same as move_uploaded_file() raise warning
to have consistent output in case of failure.

Please change the code like that.

[kenjis]

@diegoscoponi I added a comment.

And you have a merge commit in this PR.
Please rebase your branch like the following:

$ git fetch upstream
$ git checkout UploadedFileHasMoved
$ git rebase upstream/develop
$ git push --force-with-lease origin UploadedFileHasMoved

Thank you!