Logging of storage operations

[tarjei]

Feature Request

If storage fails, then the errors should be logged.

For example in the FlySystemStorage module we got:

    protected function doRemove(PropertyMapping $mapping, ?string $dir, string $name): ?bool
    {
        $fs = $this->getFilesystem($mapping);
        $path = !empty($dir) ? $dir.'/'.$name : $name;

        try {
            $fs->delete($path);

            return true;
        } catch (FilesystemException $e) {
            return false;
        }
    }

This error is not propagated anywhere. It would be very usefull if it at least got logged.

Q	A
New Feature	yes
RFC	no
BC Break	no

Summary

Log errors in storagehandlers.

[garak]

You can easily add a listener for the exception and perform whatever you want (logging, etc.)

[tarjei]

@garak how? It seems to me it is just swallowed... :)

[garak]

Sorry, I misread, you're right.
I think the method should throw the same CannotWriteFileException thrown by the doUpload method.
By the way, the previous exception should be passed.

[tarjei]

@garak no problem :) This would require a try catch in UploadHandler wouldn't it? So that the exception is caught there and an event emitted? If the exception is thrown all the way then that would be a BC break - no?

[garak]

On second thought, it's better to keep it like it is now.
The handler expects an exception for the upload and doesn't expect it from the remove.

Maybe you can exploit a combination of pre-remove and post-remove events to reach your goal.
Otherwise, redefining your own storage is always a possibility.

[tarjei]

I would like to politely disagree. Debugging config errors etc in a storage handler is impossible the way it is setup now with silent fail.

The simplest solution would be a PR where the StorageHandlers get a logger and log each execption. This surfaces the error but does not change the overall behavior of the system. IMHO the other option is to let the UploadHandler catch the exception and emit an event.

[garak]

I'd rather avoid adding a new dependency.
Let's go with the new event.