twistor / flysystem-stream-wrapper

Provides the ability to register Flysystem filesystems as stream wrappers.
MIT License
58 stars 19 forks source link

Calls to `uri_stat()` triggers a notice #12

Open sjadema opened 5 years ago

sjadema commented 5 years ago

When checking for directories with is_dir() and the directory exists, the following notice is thrown:

PHP Notice: Undefined offset: 0 in <SOME_PATH>/vendor/twistor/flysystem-stream-wrapper/src/Flysystem/Plugin/Stat.php on line 157

The following code should trigger the error:

<?php

use League\Flysystem\Filesystem;
use League\Flysystem\Memory\MemoryAdapter;
use Twistor\FlysystemStreamWrapper;

require_once 'vendor/autoload.php';

$filesystem = new Filesystem(new MemoryAdapter());
FlysystemStreamWrapper::register('test', $filesystem);

$directory = 'test:///test';
var_dump(is_dir($directory)); // Works as expected, no notice

mkdir($directory);
var_dump(is_dir($directory)); // Works as expected, but a notice is thrown
sjadema commented 5 years ago

This happens because $metadata['visibility'] = false. It tries to use that false as an index for $this->permissions and the false is casted to 0. The indexes in $this->permissions['dir'] are private & public hence the 0 isn't found.

sjadema commented 5 years ago

After doing some research, the error originates in het MemoryAdapter. When creating a new directory, the visibility isn't stored properly. I still think you should validate the metadata properties though.

kor3k commented 5 years ago

replace https://github.com/twistor/flysystem-stream-wrapper/blob/master/src/Flysystem/Plugin/Stat.php#L157 with

$ret['mode'] += empty($this->permissions[$metadata['type']][$metadata['visibility']]) ? $metadata['visibility'] : $this->permissions[$metadata['type']][$metadata['visibility']];
ronaldvdlp commented 5 years ago

A recent change in league/flysystem (1.0.55) is causing more problems with this: https://github.com/thephpleague/flysystem/commit/b0f7bee046dec58814f7f0fbe025c4ff4eb3fdc5 Error "Undefined index: 0666" is triggered in the mergeMeta function when calling $metadata = $this->filesystem->getMetadata($path); from Stat:getWithMetdata when working with the Local Adapter.

kor3k commented 5 years ago

the $metadata['visibility'] is supposed to be either a "named permission" string like public, or an octal pemission mode.

this should fix it for the moment: https://github.com/twistor/flysystem-stream-wrapper/pull/15/files

but it looks it will be changed again: https://github.com/thephpleague/flysystem/issues/1051

oofz commented 4 years ago

the $metadata['visibility'] is supposed to be either a "named permission" string like public, or an octal pemission mode.

this should fix it for the moment: https://github.com/twistor/flysystem-stream-wrapper/pull/15/files

but it looks it will be changed again: thephpleague/flysystem#1051

tested and works 👍

hexus commented 4 years ago

This is affecting usage on PHP 7.4.

This repo hasn't been updated in quite a while, is it no longer being maintained?

hexus commented 4 years ago

@twistor, any chance we could get #15, #18 and #19 reviewed or merged? It appears that #15 and #19 address this particular error in different ways, and #18 is just about PHP 7.4 support in Travis.

kor3k commented 4 years ago

@hexus mine #15 was just a quick fix, but #19 looks more sophisticated, so i'd prefer this one. well, @twistor ain't responding for over a year 🤷‍♂️ so perhaps i'll make a fork and merge #18 and #19 in there.

kor3k commented 4 years ago

ok so here it is, tagged to v1.0.10: https://github.com/kor3k/flysystem-stream-wrapper

overriding packagist repo source in composer:

    "repositories": [
        {
            "type": "vcs",
            "url": "git@github.com:kor3k/flysystem-stream-wrapper.git"
        }
    ]
JoshuaBehrens commented 1 year ago

Hi @kor3k I just stumbled upon your fork and really like that you made sure to have a version of this with the available bug fixes merged. I want to refer to your version in my composer requirements. As only root composer files can override repositories I basically can't reference it. Would you mind submitting your version to packagist with your prefix so one can easily pull in your version as dependency?

I am a bit sad that @twistor neglects this. Solutions are available and there seems to be activity by @twistor on GitHub. If this is not updated anymore please mark it as archived so we can be sure that someone else needs to step in.

kor3k commented 1 year ago

@JoshuaBehrens hello and thank you.

yes i'll take a look on adding it to packagist.

but, this is for flysystem v1, which is obsolete now. (imho that's why twistor is neglecting it). there is actively maintained version of stream wrapper for flysystem v2|v3. i recommend you to upgrade it in your projects if possible, like

"league/flysystem": "^3.0",
"m2mtech/flysystem-stream-wrapper": "^1.3",
JoshuaBehrens commented 1 year ago

you're welcome @kor3k and thank you very much

I know I use the other wrapper already. In our library we use stream wrappers as abstraction layer. But at one point the library is used within shopware 6 and up to this point they depend on v1:

https://github.com/shopware/platform/blob/6.4.16.0/composer.json#L83-L85

        "league/flysystem": "~1.1.4",
        "league/flysystem-aws-s3-v3": "~1.0.29",
        "league/flysystem-memory": "~1.0.2",
kor3k commented 1 year ago

it is here: https://packagist.org/packages/kor3k/flysystem-stream-wrapper and tagged to v1.0.11

@JoshuaBehrens

JoshuaBehrens commented 1 year ago

@kor3k thank you very much :)