-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v10: Update to ImageSharp v2 #12185
v10: Update to ImageSharp v2 #12185
Conversation
src/Umbraco.Web.Common/ImageProviders/FileProviderImageProvider.cs
Outdated
Show resolved
Hide resolved
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
ea06355
to
2de5520
Compare
@@ -23,7 +23,7 @@ public class ContentImagingSettings | |||
} | |||
}; | |||
|
|||
internal const string StaticImageFileTypes = "jpeg,jpg,gif,bmp,png,tiff,tif"; | |||
internal const string StaticImageFileTypes = "jpeg,jpg,gif,bmp,png,tiff,tif,webp"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setting doesn't seem to be used: the supported image file types are now retrieved from the IImageUrlGenerator.SupportedImageFileTypes
property and the ImageSharpImageUrlGenerator
implementation dynamically retrieves the supported ones from the ImageSharp configuration instead.
These image file types are then used to for more then just generating image URLs though (e.g. whether to extract image dimensions to auto-fill properties, disallow file uploads that are not images and automatically changing the media type from File
to Image
).
I suggest keeping this setting and create a separate PR to fix the mixed usage, so:
ContentImagingSettings.ImageFileTypes
is used to determine whether the uploaded file is a valid image and if it can automatically change the media type toImage
;IImageUrlGenerator.SupportedImageFileTypes
is used whether it can generate an image URL for a specified file;- A new
IImageDimensionExtractor.SupportedImageFileTypes
property is added that exposes whether it can get dimensions for a specified file (and have theImageSharpDimensionExtractor
populate this the same way as theImageSharpImageUrlGenerator
currently does).
|
||
if (options.CacheBusterValue is string cacheBusterValue && !string.IsNullOrWhiteSpace(cacheBusterValue)) | ||
{ | ||
queryString.Add("rnd", cacheBusterValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think v
would be a much better (and shorter) query string key, as this isn't a random value we're adding, but a versioning value. Changing this doesn't impact the generation of images: it would only invalidate any images in your browser cache.
And that might actually be for the better, as you should also clear your existing ImageSharp cache as part of the migration, because newly generated images can be different because of the automatic EXIF rotation.
queryString.Add("rnd", cacheBusterValue); | |
queryString.Add("v", cacheBusterValue); |
This should then also be updated in the corresponding tests.
Breaking changes
There are also some functional changes to be aware of:
|
@@ -200,7 +200,6 @@ public static IUmbracoBuilder AddCoreInitialServices(this IUmbracoBuilder builde | |||
// Add default ImageSharp configuration and service implementations | |||
builder.Services.AddSingleton(SixLabors.ImageSharp.Configuration.Default); | |||
builder.Services.AddSingleton<IImageDimensionExtractor, ImageSharpDimensionExtractor>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally we add a NoopImpementation so you can boot Infrastructure without the web assemblies
Prerequisites
If there's an existing issue for this PR then this fixes #11329. Also see the related discussion #11938 and the draft PR #11991 trying to update v9 in a backwards compatible way (spoiler: we can't).
Description
This PR updates the dependency versions of ImageSharp to 2.1.0 and ImageSharp.Web to 2.0.0 (alpha, currently waiting on the final release). Because these new major versions contain breaking changes, we can only update these for the upcoming Umbraco v10 release. Some of the breaking changes are:
CachedNameLength
has been renamed toCacheHashLength
;IDictionary<string, string> commands
has been changed toCommandCollection commands
;RequiresTrueColorPixelFormat()
method is added toIImageWebProcessor
.ImageSharp/ImageSharp.Web also has some functional changes as well:
PhysicalFileSystemProvider
doesn't use theWebRootFileProvider
anymore(it now only supports actual physical files OOTB)(an additionalWebRootImageProvider
is now available OOTB, but not used by default);ResizeWebProcessor
is now EXIF-orientation aware (as browsers now default to displaying images based on the EXIF orientation);PhysicalFileSystemCache
has a separate option for the cache folder depth (it previously used the file length to create nested cache folders);To-do:
CropWebProcessor
(waiting on PR Expose ExifOrientationUtilities and ICommandConverter implementations SixLabors/ImageSharp.Web#241 that adds an utility class to make this easier)Umbraco-CMS/src/Umbraco.Core/Configuration/Models/ContentImagingSettings.cs
Line 26 in 5bfab13