search

silverstripe / silverstripe-asset-admin

Silverstripe assets gallery for asset management
BSD 3-Clause "New" or "Revised" License
19 stars 78 forks source link

Image PreviewLink should not be used by asset-admin, because it ignores resample_images config setting #1374

Open p0lar-bear opened 1 year ago

p0lar-bear commented 1 year ago

Image::PreviewLink() always calls ImageManipulation::FitMax(), which in turn will always create a copy of an image if one doesn't exist, regardless of whether or not the image is large enough to be rescaled. I'm not exactly sure where in this chain of events it should happen, but I feel like there should be a check against the File::resample_images config setting to prevent this from happening and to just use the full-size image's URL if resample_images is set to false. At least, I can work around this for now by subclassing Image and editing the config for File::class_for_file_extension to override the default class definitions for the image extensions.

In my use-case, I'm not using SilverStripe as a traditional CMS - I do not need images to be optimized for quick delivery and consumption, and do not want the assets directory to be littered with thumbnails alongside the source images.

GuySartorelli commented 1 year ago

I'm not sure whether to label this a bug or an enhancement.... does this actually cause some failure of functionality at all or is the only downside that you have more files on the filesystem?

p0lar-bear commented 1 year ago

Sorry for the late response on this.

I personally feel this should probably be labeled as a bug. While there isn't a "failure" in the sense of something being rendered unusable, the fact remains that there is a switch here, and the functionality that it purportedly controls still happens regardless of whether it's switched "off". That is, by definition, a bug.

I'm aware I have a strange use-case and I don't kid myself for a moment thinking that there's any urgency or high priority to it, but when something doesn't do the thing it's described to do, it should be brought to attention.

GuySartorelli commented 1 year ago

Thank you for that additional information.

I'm just looking at the code now. The PHPDoc comment for the resample_images configuration property says:

Control whether images in the admin will be resampled

In other words this configuration must not affect the front-end. Really it should be declared somewhere in silverstripe/asset-admin... but in any case, the PreviewLink() method can be used on the front-end to get a thumbnail for display, so the problem isn't so much that PreviewLink() ignores the config setting, but that it is used by asset-admin regardless of the setting.

I'm going to rename the issue to reflect that and move it to the asset-admin repository.