-
Notifications
You must be signed in to change notification settings - Fork 21
Implement IFileProviderFactory, move shared infrastructure to Umbraco.StorageProviders and support Umbraco 10 #36
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
Conversation
… keys and remove restore condition)
bergmania
left a comment
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.
Overall this looks super good, but I think we should just make it depend on Umbraco 10 + .NET6.
I wonder if it make sense to make a composer to you are not forced to manually insert into your startup.cs. What do you think?
Also, Should we make the classes internal or at least sealed, when we do not expect people to extend them? Just to make our life easier in the future?
Can we use TypeForwardedToAttribute for those types now moved into the common assembly, or does it not support namespace changes too?
src/Umbraco.StorageProviders.AzureBlob/AzureBlobFileProvider.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.StorageProviders.AzureBlob/IO/AzureBlobFileSystem.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.StorageProviders.AzureBlob/Umbraco.StorageProviders.AzureBlob.csproj
Show resolved
Hide resolved
src/Umbraco.StorageProviders/DependencyInjection/CdnMediaUrlProviderExtensions.cs
Show resolved
Hide resolved
AndyButland
left a comment
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.
Looks good to me - two tiny typos fixed with suggestions inline.
Co-authored-by: Andy Butland <abutland73@gmail.com>
PR umbraco/Umbraco-CMS#11783 added a new
IFileProviderFactoryinterface that anIFileSystemcan implement to expose itself to theWebRootFileProvider, that in turn is used by theStaticFileMiddlewareto serve the files.Adding support for this to the
AzureBlobFileSystemhas the following benefits:IFileProviderabstraction, which is used by the static file middleware, Tag Helpers, etc.AzureBlobFileSystemMiddlewareis therefore removed;UseAzureBlobMediaFileSystem()is not needed anymore for the same reason, simplifying setup;Cache-Control: public, must-revalidate, max-age=604800header).IImageProviderin ImageSharp.Web v1 uses theWebRootFileProvider, removing the need for adding an additionalAzureBlobFileSystemImageProvider(one minor downside is that the max-age of the original image returned by Azure Blob Storage isn't used anymore and ImageSharp falls back to the configuredBrowserMaxAgesetting).Because the dependency on Umbraco was updated to
9.3.0 (the version introducing10.0.0-rc1 and I've moved shared infrastructure code (the CDN media URL provider) to a newIFileProviderFactory)Umbraco.StorageProvidersproject, I've bumped to version to v2.I've also included the changes in #34, so the build and versioning simplification can/should be reviewed and merged separately first.I've reverted the versioning changes (so we can align with the updated versioning in the CMS later), but kept the simplified build, as that doesn't require different release workflows and speeds up the build times significantly.With this PR is merged, adding additional storage providers (like AWS S3 as requested in #28) should be a lot easier, as you basically only have to implement an
IFileSystem(so Umbraco can store/retrieve media) and anIFileProvider(so the media can be served and retrieved by ImageSharp) that's exposed by theIFileProviderFactory.