search

silverstripe / silverstripe-assets

Silverstripe Assets component
BSD 3-Clause "New" or "Revised" License
9 stars 67 forks source link

`Image::Convert()` shows error in frontend instead of returning null #656

Open xini opened 2 weeks ago

xini commented 2 weeks ago

Module version(s) affected

2.3.0

Description

When the conversion of an image fails, the error logger shows the error output on the page.

I think the conversion should fail silently and just log the error, not show it on the page.

How to reproduce

add 

throw new FileConverterException('test error');

to the first line of InterventionImageFileConverter::convert()

Possible Solution

use default logger instead of error logger in https://github.com/silverstripe/silverstripe-assets/blob/2/src/ImageManipulation.php#L728

$logger = Injector::inst()->get(LoggerInterface::class);

instead of 

$logger = Injector::inst()->get(LoggerInterface::class . '.errorhandler');

Additional Context

No response

Validations

GuySartorelli commented 2 weeks ago

Couple of questions:

  1. Under what real world circumstances are you getting this error? When this was implemented, IIRC the assumption was that most (if not all) failures to convert files would be because of some misconfiguration (no converter for that file type, for example). Those sorts of errors should result in hard failures, and developers should resolve them.
  2. What would you have happen instead? Should we just pretend the conversion was never attempted and use the original file, or just return null?
    • In the former case, that could result in e.g. displaying a video instead of a thumbnail which at best is not what the developer intended - or download links could download the wrong file, or you could have a broken thumbnail because it's trying to show a pdf where an image goes, etc etc.
    • In the latter case, you'd have links with empty href attributes, images outright missing from the page or being broken because there's no src, etc.
xini commented 2 weeks ago

  1. This occurs for example when the original file doesn't exist, or if the third party library doing the conversion fails, e.g. with my converter from PDF to image for thumbnails (https://github.com/innowebau/silverstripe-pdf-image-converter) using imagick. Also, yes, I agree that these failures should be addressed by the dev, but logging them is enough IMO.

  2. Like all other image manipulations this should just return null and fail silently. For example, when you do a $Image.Pad(200,200) and something goes wrong, it returns null. At the moment, Convert() is the only file manipulation method that results in a hard failure.

(Apart from that, the comment for ImageManipulation::Convert() states

@throws FileConverterException If the conversion fails and $returnNullOnFailure is false.

, but $returnNullOnFailure does not exist anywhere in the Silverstripe code.)

GuySartorelli commented 2 weeks ago

This occurs for example when the original file doesn't exist

How do you get into that situation?

or if the third party library doing the conversion fails

What's the reason for the failure? Is it some intermittent problem, or is it a misconfiguration, or something else?

(Apart from that, the comment for ImageManipulation::Convert() states@throws FileConverterException If the conversion fails and $returnNullOnFailure is false., but $returnNullOnFailure does not exist anywhere in the Silverstripe code.)

That's actually a good argument. Looking back at the PR: https://github.com/silverstripe/silverstripe-assets/pull/595#discussion_r1556615756 The $returnNullOnFailure param was removed intentionally, and the intention was indeed that this should return null.

use default logger instead of error logger

Is the error logger somehow causing an error to be thrown? Will using the default logger still log to all places that the error logger logs to?

xini commented 2 weeks ago

This occurs for example when the original file doesn't exist

How do you get into that situation?

You tell me. ;) I have some older sites that have been upgraded a few times over time. It just happens, I guess.

or if the third party library doing the conversion fails

What's the reason for the failure? Is it some intermittent problem, or is it a misconfiguration, or something else?

The reason doesn't matter. The converter throws a FileConverterException in any case, which triggers the error.

Maybe there should be two different exceptions to be thrown for intermittent and permanent errors?

(Apart from that, the comment for ImageManipulation::Convert() states@throws FileConverterException If the conversion fails and $returnNullOnFailure is false., but $returnNullOnFailure does not exist anywhere in the Silverstripe code.)

That's actually a good argument. Looking back at the PR: #595 (comment) The $returnNullOnFailure param was removed intentionally, and the intention was indeed that this should return null.

right. that would make sense and would be in line with all other manipulations.

use default logger instead of error logger

Is the error logger somehow causing an error to be thrown? Will using the default logger still log to all places that the error logger logs to?

The default logger logs to the log file. the error logger halts execution and AFAIK there is no way to redirect it to just a log file.

I don't understand why there are two loggers anyway.

elliot-sawyer commented 2 days ago

I don't understand what is wrong with the submitted pull request. Not throwing an exception, and returning null, sounds like a sensible solution

I get these errors when a user uploads a file with a long name within upload limits - the resampled filename, however, is too long for the filesystem

League\Flysystem\UnableToWriteFile

Unable to write file at location: <truncated> Failed to open stream: File name too long

In my opinion, that's not a developer problem. It's a content issue and front-end users bear that impact.

Silently logging and returning nothing should be the way to go. Developers can still be alerted and fix the problem when there's time and budget to do so. In this case, developers can simply tell the content author to rename their file and try again.

GuySartorelli commented 2 days ago

The problem with the submitted pull request is there's no explanation of why the logger that's currently being used is incorrect. It feels like there's an underlying problem with that logger which is being ignored.

If that logger should not be used directly, then documentation needs to be updated to explicitly mention that (and give the reason why), and we need to check if we're incorrectly using it anywhere else - not just this one location.