Call to move_uploaded_file() always returns true

[dandjo]

When uploading a file with php forms using $_FILES and calling move_uploaded_file() with a stream wrapper as destination, the return value of writeStream is ignored.

[dandjo]

It seems to be a bug in PHP ignoring the return value of the method stream_flush in stream wrappers.
https://bugs.php.net/bug.php?id=78077

[Lewiscowles1986]

Would you be able to reproduce a test case or script to demonstrate?

[dandjo]

Of course, like written in the PHP Bug above:

class MyStreamWrapper
{

    public function stream_write($data)
    {
        return 1;
    }

    public function stream_flush()
    {
        return false;
    }

}

stream_wrapper_register('my-wrapper', \MyStreamWrapper::class, 0);
$res = move_uploaded_file($_FILES['uploaded_file']['tmp_name'], 'my-wrapper://' . basename($_FILES['uploaded_file']['name']));
stream_wrapper_unregister('my-wrapper');
echo $res ? 'true' : 'false';

Expected result: 'false'
Actual result: 'true'

[Lewiscowles1986]

Right, but that's how PHP the language works and seems devoid of anything related to this wrapper.

[Lewiscowles1986]

To be even more clear https://github.com/php/php-src/blob/16f194c75e05381628ae2b9468fb8004dec9e176/ext/standard/basic_functions.c#L3282-L3333 shows no reference to flush being called at all. If the failure is encapsulated elsewhere, unless that function returns false (it seems there are only rename and copy then rm semantics), then this needs to be attributed to those.

[Lewiscowles1986]

looking at https://github.com/php/php-src/blob/9a76a2a0329aae203e6f27be40f2977a8d7e223f/ext/standard/file.c#L1644-L1733 it seems this is where stat is checked for copy-case it likewise does not flush

[dandjo]

Is this really a matter of "how PHP works"? In my opinion it's a severe bug or concept glitch. In combination with streamwrappers you will never know whether the upload succeeded or not. You won't even get a useful feedback. We ended up patching Drupal to use our own move_uploaded_file().

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21File%21FileSystem.php/function/FileSystem%3A%3AmoveUploadedFile/8.2.x

https://www.drupal.org/files/issues/2019-06-03/3057565-6-move-uploaded-file-streamwrapper.patch

[danhunsaker]

Ultimately, what you've encountered is a bug in move_uploaded_file itself, so yes, it's a PHP bug, rather than a stream wrapper bug.

[Lewiscowles1986]

@dandjo I guess congrats on getting something that works for you.

@danhunsaker should this be closed or marked with descriptive labels in-case someone else encounters? Technically since it's not part of this library, I suppose it's not a problem to close? IDK

Also thanks for writing this library. Very cool concept to make filesystem easier

[danhunsaker]

Oh, I didn't write it. I just follow issues for it since I wrote a lib for Laravel that automatically pulls this one in and sets it up for configured drives.

[Lewiscowles1986]

Link please 😉

[danhunsaker]

https://github.com/danhunsaker/laravel-flysystem-others

[dandjo]

Ultimately, what you've encountered is a bug in move_uploaded_file itself, so yes, it's a PHP bug, rather than a stream wrapper bug.

Yes, fully d'accord. I'm thinking about writing either a polyfill to overload the behaviour or sending a pull request to patch the Flysystem to handle this issue. I've no hope the PHP guys will patch this over the next few years. :-)