Skip to content

Virtual Path Provider only works on media #188

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

Merged
merged 2 commits into from
Apr 6, 2021
Merged

Virtual Path Provider only works on media #188

merged 2 commits into from
Apr 6, 2021

Conversation

callumbwhyte
Copy link
Member

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:

  • Introduces an overload for the Configure method, taking an IFileSystem and a path
  • Moves the tight coupling to Current.MediaFileSystem into the ConfigureMedia method
  • Obsoletes the previous Configure method in favour of the more explicit ConfigureMedia method

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
Copy link
Collaborator

Jeavon commented Apr 6, 2021

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
Copy link
Member Author

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
Copy link
Collaborator

Jeavon commented Apr 6, 2021

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

@callumbwhyte
Copy link
Member Author

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
Copy link
Collaborator

Jeavon commented Apr 6, 2021

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
Copy link
Member Author

@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
Copy link
Collaborator

Jeavon commented Apr 6, 2021

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

@Jeavon Jeavon merged commit eb3fb29 into umbraco-community:develop-umbraco-version-8 Apr 6, 2021
@callumbwhyte callumbwhyte deleted the virtual-path-provider branch May 6, 2021 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants