search

umbraco-community / UmbracoFileSystemProviders.Azure

:cloud: An Azure Blob Storage IFileSystem provider for Umbraco
96 stars 67 forks source link

Virtual Path Provider only works on media #188

Closed callumbwhyte closed 3 years ago

callumbwhyte commented 3 years ago

One of our projects has multiple "file systems" based off the Azure File System, e.g. for storing content other than Umbraco media.

Whilst trying to configure the contents of one of these file systems to be publicly routable I discovered it's not directly possible with the built in methods to configure a virtual path for a file system other than IMediaFileSystem.

This is because FileSystemVirtualPathProvider.Configure() looks up the media provider from the service locator, regardless of the path passed in. There are no other methods on FileSystemVirtualPathProvider to register a provider, so you have to call HostingEnvironment.RegisterVirtualPathProvider directly...

This PR:

Looking at it, I'm unsure why 95% of this class lives within the UmbracoFileSystemProviders.Azure.Media package as it should be globally usable. As the namespace for this class is still UmbracoFileSystemProviders.Azure it could be moved without creating too much of a breaking change - the ConfigureMedia method could live on as a static extension inside UmbracoFileSystemProviders.Azure.Media and call back to the moved class in order to keep responsibilities clean. I haven't done this here, but would be open to making a PR if it makes sense.

Jeavon commented 3 years ago

Looks great @callumbwhyte

The VPP has always been viewed as a specific feature for virtually mapping the media paths but you are right it could be reused for other implementations like XML sitemaps but does it make sense to be moved into the UmbracoFileSystemProviders.Azure package or should it be another package 😱

callumbwhyte commented 3 years ago

Thanks @Jeavon

If you'll accept a PR to move this class back into the UmbracoFileSystemProviders.Azure package I'll happily make one, or a modification to this PR? Not sure what the best approach would be here with regards to where the methods to configure the media provider would reside...

Jeavon commented 3 years ago

hmm, would we even need to keep ConfigureMedia, could the logic be moved into the media component?

callumbwhyte commented 3 years ago

Fantastic point - that's much cleaner. Probably a good idea to also introduce an optional (and hidden OOTB) app setting to allow the path to be configured, else the component needs replacing in it's entirety?

Jeavon commented 3 years ago

If installed to not use the default route then it seems to use the container name - https://github.com/umbraco-community/UmbracoFileSystemProviders.Azure/blob/develop-umbraco-version-8/src/UmbracoFileSystemProviders.Azure.Media/AzureMediaFileSystemComponent.cs#L36

It's possible you might want to manually set it in an appsetting?

callumbwhyte commented 3 years ago

@Jeavon PR updated moving the code into the core project. I left the decision of which path name to use to the container name, as before, to minimise changes.

While the ConfigureMedia method has been removed and the Configure method signature changing does constitute a breaking change, I do think the impact of this is minimal. Worth noting somewhere though...

Jeavon commented 3 years ago

Looks great and yeah will release as v2.1 for the breaking changes, thank you!