yiisoft / yii-core

Yii Framework 3.0 core
https://www.yiiframework.com/
433 stars 75 forks source link

Request to separate FileHelper from yii-core #204

Closed razonyang closed 5 years ago

razonyang commented 5 years ago

It would be helpful for some Yii libraries(does not depends on yii-core) to write test cases, for example, create temporary files and delete those files(createDirectory, removeDirectory).

samdark commented 5 years ago

Yes. Should be done.

samdark commented 5 years ago

One thing is that I suspect we need a file system abstraction instead of direct helper.

samdark commented 5 years ago

https://github.com/yiisoft/filesystem-plugins. If you want to design interfaces for it, let me know.

schmunk42 commented 5 years ago

What apart from https://flysystem.thephpleague.com/docs/ is needed?

samdark commented 5 years ago

Maybe nothing. PHP 7 type-hints but that's not mandatory.

razonyang commented 5 years ago

Are we gonna to implements our own file system abstraction API instead of using flysystem?

samdark commented 5 years ago

There are two separate things:

  1. Implementation itself. I don't think it's a good idea to implement our own because flysystem is well-maintained, has 100% code coverage and many adapters.
  2. Interfaces. That may make sense to introduce our own to be extra-safe and have PHP 7 type hints but it's not really necessary since flysystem is the only library of its kind for now.

Overall, it seems it makes no sense to do the wrapper. As for our file helper, we need to inspect if it's something missing from flysystem. If it is then it should be turned into non-static thing that uses flysystem. If it's not then I'd drop it completely.

schmunk42 commented 5 years ago
  1. Implementation itself. I don't think it's a good idea to implement our own because flysystem is well-maintained, has 100% code coverage and many adapters.

The quality of flysystem is outstanding! :100:

@rob006 IMO, this would be a good example for an awesome list.

samdark commented 5 years ago

So to sum up:

  1. Check what's missing from flysystem that present in file helper.
  2. Write it down here in the issue comment.
razonyang commented 5 years ago

The following methods do not appear in FlySystem API :

FlySystem API contains listContents method, but not suitable for findFiles and findDirectories.

samdark commented 5 years ago
  1. It seems FlySystem API could be extended via plugins. These are good plugin candidates:

    • copyDirectory
    • findFiles
    • findDirectories
  2. I'd remove getMimeTypeByExtension and getExtensionsByMimeType for now.

  3. normalizePath may be not so useful. It seems FlySystem API normalizes paths already. Also, there's similar method: https://github.com/thephpleague/flysystem/blob/master/src/Util.php#L88

samdark commented 5 years ago

So that leaves us with implementing plugins.

samdark commented 5 years ago

As for our own abstraction, I think it's OK to use FlySystem directly.

razonyang commented 5 years ago

I'm a little confused, if we implements these plugins without caring about which adapter is used, findFiles and findDirectories could be implemented by listContents, and copyDirectory can also be achieved by copy, but it maybe causes performance issues?

Are we talking about implementing these plugins separately for each adapter?

samdark commented 5 years ago

I think we can start by using APIs provided by FlySystem itself to achieve convenience. If performance would be too bad, we'll see what could be done about it.

yus-ham commented 5 years ago

There are Flysystem's methods provided via plugins

samdark commented 5 years ago

I've re-read original issue and it looks like I've left out important part "to write test cases" focusing on application use of file system. For test cases having simple file helper makes sense.

samdark commented 5 years ago

Done at https://github.com/yiisoft/files

razonyang commented 5 years ago

There are Flysystem's methods provided via plugins

  • listFiles() --> findFiles()
  • listWith(['type' => 'dir']) --> findDirectories()

Just a remind: the listWith usage is incorrect, it is for ensuring presence of specific metadata. :) I've also created PR thephpleague/flysystem#1044 for findDirectories.