yiisoft/yii-core

Request to separate FileHelper from yii-core

razonyang opened this issue ยท 19 comments

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).

Yes. Should be done.

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

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

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

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

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.

  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! ๐Ÿ’ฏ

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

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.

The following methods do not appear in FlySystem API :

  • normalizePath
  • getMimeTypeByExtension
  • getExtensionsByMimeType
  • copyDirectory
  • matchBasename
  • matchPathname
  • findFiles
  • findDirectories

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

  1. It seems FlySystem API could be extended via plugins. These are good plugin candidates:
  • copyDirectory
  • findFiles
  • findDirectories
  1. I'd remove getMimeTypeByExtension and getExtensionsByMimeType for now.
  2. 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

So that leaves us with implementing plugins.

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

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?

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.

There are Flysystem's methods provided via plugins

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

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.

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.