Filesystem/CascadingFilesystem

[WinterSilence]

Fixes:

1. Cache should not be mandatory - __construct(array $base_paths, Cache $cache = null)
2. Need set setBasePaths and isHiddenFile as public
3. Why listFiles not supported subdirectories? Old Kohana::list_files() more powerfull than this.

    public function isHiddenFile($filename)
    {
        return in_array($filename[0], ['.', '~']);
    }

1. load: include better than require_once, because we can't catch fatal errors, we use it's not only for class files.

[lenton]

Cache should not be mandatory - __construct(array $base_paths, Cache $cache = null)

You can pass an in-memory cache called ArrayCache() if you don't want persistent caching. The advantage of this is that the CascadingFilesystem class doesn't have to have lots of if ($cache !== null) checks.

Need set setBasePaths and isHiddenFile as public

Base paths were intended to be set once on construction and then not changed again which is why setBasePaths() is for internal use only. As for isHiddenFile(), it is just a helper method to determine whether a filename (string) is considered to be a hidden file. I'm not sure it would be very useful to users.

Why listFiles not supported subdirectories? Old Kohana::list_files() more powerfull than this.

I changed it to not list sub directories because that's completely unnecessary and inefficient; if you want a sub directory's contents, then execute the function again. However, a new function called dump() (or similar) which returns the whole CFS as an array would be good for debugging.

load: include better than require_once, because we can't catch fatal errors, we use it's not only for class files.

The load method only exists to make the tests easier to write, it's not really intended for use by users. The method Kohana::load() should still exist after the cascading filesystem is implemented anyway.

Thanks for the review, it has given me a few ideas for improvements.

[acoulton]

@WinterSilence I don't think it's helpful to have these all in one thread.

Re 1. Optional dependencies are bad. They complicate dependent-class logic by forcing everything to be wrapped in conditionals eg if ($this->cache). Also complicate setting up DI containers etc. Much better to have a mandatory dependency, you can always inject a NullCache or ArrayCache if you want.

Re 5. load in this method is really just a wrapper that's used by the autoloader, it's only intended to be called with an (existing) file path you have already retrieved eg from getPath. I guess @lenton only put it in CascadingFilesystem so he could spy on it in specs? Maybe it's in the wrong place, or badly named?

Can you create separate issues/PRs for setBasePaths, isHiddenFile, 3, 4, and 5? I think 1 is a wontfix but if you feel strongly make a case in an issue.

[acoulton]

Oops overlapped with @lenton

[WinterSilence]

You can pass an in-memory cache called ArrayCache() if you don't want persistent caching. The advantage of this is that the CascadingFilesystem class doesn't have to have lots of if ($cache !== null) checks.

agree

As for isHiddenFile(), it is just a helper method to determine whether a filename (string) is considered to be a hidden file. I'm not sure it would be very useful to users.

I do not understand why need create separate method, why not write if (! $hidden_files && in_array($filename[0], ['.', '~'])) { ?

I changed it to not list sub directories because that's completely unnecessary and inefficient; if you want a sub directory's contents, then execute the function again.

Easier to make a recursive method than every time a recursive call.

The load method only exists to make the tests easier to write, it's not really intended for use by users. The method Kohana::load() should still exist after the cascading filesystem is implemented anyway.

Not a suitable place for the method