enl / flysystem-cloudinary

Cloudinary adapter for Flysystem
MIT License
27 stars 20 forks source link

Delete directory should remove all files within this directory #3

Closed tdutrion closed 9 years ago

tdutrion commented 9 years ago

According to the interface, we can not define the expected behaviour of the deleteDir method, but by looking at the FTP adapter for instance, it seems like every files under the specified directory should be removed as well as the directory.

public function deleteDir($dirname)
{
    $connection = $this->getConnection();
    $contents = array_reverse($this->listDirectoryContents($dirname));
    foreach ($contents as $object) {
        if ($object['type'] === 'file') {
            if (! ftp_delete($connection, $object['path'])) {
                return false;
            }
        } elseif (! ftp_rmdir($connection, $object['path'])) {
            return false;
        }
    }
    return ftp_rmdir($connection, $dirname);
}

For Cloudinary, directories are not supported, so the current implementation is raising an Exception (which by the way is not declared in the interface docblock).

/**
 * Delete a directory.
 *
 * @param string $dirname
 * @return bool
 * @throws NotSupportedException
 */
public function deleteDir($dirname)
{
    throw new NotSupportedException('Cloudinary API does not support folders management.');
}

vs

/**
 * Delete a directory.
 *
 * @param string $dirname
 *
 * @return bool
 */
public function deleteDir($dirname);

The problem is when someone tries to use the adapter to replace another one, the exception will be thrown where one expects to have a return false; or see all the files under the directory be deleted.

I can make a PR but I first need to know what direction we should consider: either return false or delete all files from the path and return true. In the latter case, should we include an optional PSR-3 LoggerInterface using the LoggerAwareInterface and the LoggerAwareTrait? Optionally we could initialise the logger to NullLogger to avoid having to test whether a logger has been set.

enl commented 9 years ago

There is API request to delete all files by prefix. We can consider this as deleteDir command in terms of Flysystem. It sounds pretty ok for me.

About Logging... As for me, it is not Adapter's responsibility. If Flysystem needs to log something, it should do it by itself, without sharing this functions with Adapters.

frankdejonge commented 9 years ago

@tdutrion the suggestion by @enl is correct. This is also how it's handled in other flat filesystems (like s3), directories are second grade citizens in Flysystem and are only used as a point of reference, like the "prefix" in cloudinary. In addition, methods like createDir can just return true since directories are an implicit concept in the underlying architecture.

tdutrion commented 9 years ago

Thanks for both your opinions! I'll create a PR accordingly.