Account for broken images when generating placeholders

[heidkaemper]

Very rarely it happens that images in the Glide cache cannot be read correctly. In this case the read method of Flysystem throws an UnableToReadFile exception.

This can break the whole website, because the Responsive Images addon uses the read method to generate placeholders without further checking:

https://github.com/spatie/statamic-responsive-images/blob/v2.14.4/src/Breakpoint.php#L269

A broken website can be reproduced like this:

• Install Responsive Images addon (default settings)
• Upload an image
• Use the 'responsive' tag your antlers template to output the image
• Open the website
• Delete/rename cached images in the filesystem (storage/statamic/glide/containers/...)
• Open the website again

The situation that an image in the cache cannot be read should not actually occur. However, we already had this case twice.

Therefore, I suggest using the read method in a try catch statement. As it is suggested in the Flysystem docs:

https://flysystem.thephpleague.com/docs/usage/filesystem-api/#filesystemreaderread

To be clear, this PR does not automatically rebuild broken image caches. This should be tackled by Statamic itself. But after the modification, the entire website is no longer broken. Only the affected image.

[ncla]

Hi, thanks for opening this PR.

I am bit cautious about this change, let me explain why and maybe you have some thoughts too about this.

I think there should be better way than having a generous exception catcher. It is something that the core Glide tag also does here too and I am not a fan personally. It gives off an illusion that everything is "just working", but in reality an error is silenced (logged). Seems like a bandage solution to me.

The situation that an image in the cache cannot be read should not actually occur. However, we already had this case twice.

I think you should investigate this as find out if there is some other bug in the core instead and maybe have a fix for that instead, on this repo or on core.

Is there an actual use case for users deleting, clearing files in that manipulated Glide image cache directory?

If you delete an image in Glide image cache directory, then there is still Path Cache Store (PCS for short) which stores what image manipulations have been done and what is their path in file system. https://statamic.dev/image-manipulation#path-cache-store Because PCS still thinks there is a cached image, you get the error mentioned in this PR. If there was no entry in PCS, it would create this image manipulation and store it.

If this error does occur, proper way to deal with this would be to run php please glide:clear which will clear PCS and cached Glide manipulated images, both at the same time. This comes with a downside cause now all manipulations need to be redone.

Now for why this error happens? Maybe there have been config changes, filesystem changes between deploys and PCS simply is not aware of that. Similar issue happened here, where they were changing their cache settings: https://github.com/spatie/statamic-responsive-images/issues/171

[heidkaemper]

Thanks for the speedy feedback. In fact, I understand your concerns very well!

It would be great to be able to always rely on the file system and cache. However, our experience is unfortunately a different one. Both our incidents came after normal content changes in the Statamic backend. When replacing an image and when changing the focus point.

After such content changes, a lot happens at the same time. PCS is cleared, the cache data is deleted, metadata is regenerated and, if necessary, Git commits are made. And if several users are working in the CMS at the same time, that multiplies even more.

One thing I would like to point out: The error would not have been silent, even with this PR. The affected image would have been displayed as broken in the browser and in our case the problem would still have been monitored. The only difference would be that the page would still have been accessible.

From this perspective, I consider a "safety net" acceptable. Even though I'm totally with you and wished it wasn't necessary.

[ncla]

I agree this would be nice for production environment.

Both our incidents came after normal content changes in the Statamic backend. When replacing an image and when changing the focus point.

This seems like a hint. Do you have time to spend on providing reproducible steps on this bug?

The affected image would have been displayed as broken in the browser and in our case the problem would still have been monitored.

Being pedantic here, I think the browser would just select the other images once picture container width is determined. At worst you would see a flash of broken image.

[heidkaemper]

This seems like a hint. Do you have time to spend on providing reproducible steps on this bug?

That's the tricky part. I have yet to find a way to reproduce that. But I'm going to keep testing and report the outcome over in the Statamic repo.

Regarding this addon: We could throw the exception in development right away. And log it only in production. That would be fine with me! What do you think?

[ncla]

Yep, sounds good.

To make it cleaner, if placeholder is empty (due to Flysystem read failure), then the 32w placeholder width should not exist at all in the output. Can you adjust that?

[heidkaemper]

Sure, I've added the changes to this PR.

[ncla]

Last thing and then it looks good I think, I will take a spin locally sometime this week.

[ncla]

Thanks!