Thumbs: Consider throwing an error instead of returning the source image when creating the thumb failed

[lukasbestle]

See #3023 (comment) and 0db632a#r33938782.

I think it's a good point. The original image is not only often much too large in file size, it will also not have modifications such as grayscale and cropping. Also the site owner may not want to leak the original image file to the public. An HTTP 500 error will show clearly that something went wrong with generation of this thumb.

What do you think @getkirby/kirby-staff? What was the reason why we return the source image and is it still relevant today?

[iskrisis]

I think it's good idea. I've myself published websites that had thumb issues without me realizing it because i haven't noticed.

If it was setting i would be setting this on every website but setting it as default might bring upset people that relied on this without knowing :D.

[lukasbestle]

I wouldn't make this an option to be honest. Options always make Kirby more complex. If we can't think of any reason why someone might still want the current behavior, we might as well get rid of it completely.

[iskrisis]

Agree I wouldn't make this an option either. I am just thinking about backwards compatibility? This had to be deliberate decision for some reason. Maybe i just dont realize how it works. Kirby can't know if thumb creation failed right? It only knows the thumb is not there - but that also happens when the thumb haven't finished generation. Maybe this is better solution for first visits than flashing image errors. 🤷

[lukasbestle]

The thumb generation is blocking, so Kirby knows the thumb generation is either done or it failed.

Regarding the original motivation: That's the big question. Waiting for insights from the team, especially @bastianallgeier.

[andreasotto]

Good point!

Please link to #4145 i ran into trouble recently. Same topic imho.

I guess right now errors in thumbnail generation are quietly skipped?!
If there was error logging, we would have already gained a lot.

[iskrisis]

@lukasbestle i think the drivers throw the error correctly

kirby/src/Image/Darkroom/ImageMagick.php

Line 164 in 6b20fa1

throw new Exception('The imagemagick convert command could not be executed: ' . $command);

but if i understand correctly PHP error bubbling the erorr is swallowed somewhere here

kirby/src/Cms/Media.php

Line 131 in 6b20fa1

} catch (Throwable $e) {

and the source image is returned instead.

I know kirby doesn't do this anywhere but even if the source file fallback was kept i think it would be useful to log these. I've seen many people struggle with multi format generation just because they don't have recent im or it's build without the support. On shared hostings it's pain to debug since you often don't have shell access to try.

[lukasbestle]

@iskrisis Yes, see 0db632a#r33938782.

Before we can discuss this further, we need insights on why we return the source image in the first place.

[lukasbestle]

@bastianallgeier Could you take a look at this open question? Maybe we can solve this together with #4632 (different issue, but same area in the code).

What was the reason why we return the source image and is it still relevant today?

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. This is for us to prioritize issues that are still relevant to our community. It will be closed if no further activity occurs within the next 14 days. If this issue is still relevant to you, please leave a comment.

[jonaskuske]

Not stale

[bastianallgeier]

Sorry for the huge delay. I think the idea to not throw an error is an outdated concept. We had this in multiple places where we wanted to avoid unnecessary errors. But this often makes it a lot harder to actually debug problems. I would be completely open to throw errors instead.

[iskrisis]

When you create base64 string out of image thumbnail and your server is not creating thumbnails then Kirby creates the base64 from the original image. It's complete death for performance - much worse than serving the original image directly.
It's even trickier to realize whats going on with base64 images. If you decide to keep serving original image then atleast this has to throw error and not create base64 version.

[lukasbestle]

I still think that it would overall be the most solid solution to throw an error and respond with HTTP 500.

[distantnative]

This will be in v4