Skip to content

Serve Media and App_Plugins using WebRootFileProvider (and allow changing the physical media path) #11783

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

Conversation

bergmania
Copy link
Member

@bergmania bergmania commented Dec 21, 2021

Allows changing the physical path Umbraco media is stored and ensures ImageSharp is still handing requests if the media is outside of wwwroot folder.

Test scenarios:

  • Umbraco Cloud using Azure Blog Storage
  • Local when UmbracoMediaPhysicalRootPath is changed

This PR adds a new UmbracoMediaPhysicalRootPath setting that allows you to change the directory where the physical media files are stored. It's only advised to set this if you want to move the physical files outside of the wwwroot folder. If not set, the physical path will be resolved from the UmbracoMediaPath (default value: ~/media, relative to the web root). If you therefore want to change both the URL and physical folder from /media to e.g. /images, you only have to change the UmbracoMediaPath.

Keep in mind that changing the media path requires you to manually move the existing physical files to the new location. If you change the UmbracoMediaPath, all existing URLs will also need to be updated, otherwise all stored values (like references in File Upload, Media Picker and RTE editors) will point to non-existing files and not load anymore.

…esharp are handing requests outside of the wwwroot folder.
@ronaldbarendse
Copy link
Contributor

Having a distinction between the physical path and browsable URL to media makes total sense and the changes related to this look good. To make the new UmbracoMediaUrl setting backwards compatible, we could change the default value to null and if that's the case, set it to the configured value of UmbracoMediaPath using:

builder.Services.PostConfigure<GlobalSettings>(options =>
{
    if (string.IsNullOrEmpty(options.UmbracoMediaUrl))
    {
        options .UmbracoMediaUrl = options.UmbracoMediaPath;
    }
});

The new IUmbracoMediaFileProvider would require existing file system implementations (like Azure Blob Storage) to also implement this interface (and is currently registered in the AddUmbracoImageSharp() call, but required during RegisterDefaultRequiredMiddleware()). Having an abstraction that exposes Umbraco's IFileSystem as an IFileProvider would be a much more flexible approach and an implementation of this FileSystemFileProvider wrapper is already available in umbraco/Umbraco.StorageProviders#11. One downside would be that the added indirection might cause performance issues (compared to the PhysicalFileProvider that's currently used): this can be worked around by first checking whether the IFileSystem implementation can provide an IFileProvider (e.g. using a factory method) and in that case, use the direct implementation instead of wrapping it using the FileSystemFileProvider, e.g.:

public interface IFileProviderFactory
{
    IFileProvider Create();
}

// On the existing implementation:
public class PhysicalFileSystem : IPhysicalFileSystem, IFileProviderFactory
{
    // Keep all existing code...

    public IFileProvider Create() => new PhysicalFileProvider(_rootPath);
}

// To get the 'best performing' IFileProvider and keep backwards compatibility with all current IFileSystem implementations:
var fileProvider = mediaFileManager.FileSystem switch
{
    IFileProviderFactory fileProviderFactory => fileProviderFactory.Create(),
    var fileSystem => new FileSystemFileProvider(fileSystem)
};

@ronaldbarendse
Copy link
Contributor

On another note: we shouldn't clear/remove the existing ImageSharp IImageProvider, as that would break image processing outside of the media folder, as discussed in umbraco/Umbraco.StorageProviders#16.

@ronaldbarendse
Copy link
Contributor

ronaldbarendse commented Dec 28, 2021

I've done some testing and pushed my changes to the v9/feature/umbracomediaurl branch. If that looks good to you, this can get merged to this PR again and we can do a final review.

@bergmania
Copy link
Member Author

Code in v9/feature/umbracomediaurl looks good.. I tested locally and it works as expected.. I have merged and will test on an existing umbraco cloud site when the build server has the artifacts

Copy link
Contributor

@ronaldbarendse ronaldbarendse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed some changes that ensure backwards compatibility, most notably keeping the UmbracoMediaPath setting for the request path/URL and introduce a new UmbracoMediaPhysicalRootPath setting to change where media is stored on disk.

@bergmania
Copy link
Member Author

Still not working on cloud.. 404 on the image urls. eg. https://test911.euwest01.umbraco.io/media/ddlokfad/appelsin.png?width=500

@ronaldbarendse
Copy link
Contributor

@bergmania The latest changes should fix the issues on Umbraco Cloud/with the Azure Blob Storage provider. It also reduces the amount of registered services and middleware, as we don't need a custom IImageProvider for ImageSharp.Web to work and only a single StaticFileMiddleware to serve all the static files (from wwwroot, App_Plugins and Umbraco media).

This also allows easy customization of the response headers for static files, because we're now only calling app.UseStaticFiles() once and that overload fetches the StaticFileOptions from the service container. You can test this by adding the following code to your project, which will add a custom header to all static files:

public class StaticFilesComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.Services.Configure<StaticFileOptions>(options => options.OnPrepareResponse = context =>
        {
            context.Context.Response.Headers.Add("X-Umbraco-Date", DateTime.Now.ToString("o"));
        });
    }
}

Previously, this would only add this header to static files in your wwwroot folder, like /umbraco/assets/img/installer.jpg. With this PR applied, it will also add it for requests to /App_Plugins/.../some-view.html and /media/.../image.jpg. Keep in mind that ImageSharp.Web uses it's own middleware though, so to change that, you have to configure ImageSharpMiddlewareOptions.

@ronaldbarendse ronaldbarendse marked this pull request as ready for review January 5, 2022 14:36
@ronaldbarendse
Copy link
Contributor

Because Umbraco media is now available from the WebRootFileProvider, you can access the file info the same way as static files. This makes media compatible with lots of existing (tag/HTML) helpers, like asp-append-version (or our-svg):

<img src="@home.HeroBackgroundImage.GetCropUrl(200, 200)" width="200" height="200" alt="" asp-append-version="true" />

@bergmania
Copy link
Member Author

Now it works as expected.. Just a single comment

@bergmania
Copy link
Member Author

Changes looks good to me

@ronaldbarendse ronaldbarendse changed the title Allow changing UmbracoMediaPath to an absolute path. Serve Media and App_Plugins using WebRootFileProvider (and allow changing the physical media path) Jan 6, 2022
@bergmania bergmania merged commit 642c216 into v9/dev Jan 6, 2022
@bergmania bergmania deleted the v9/bugfix/allow_changing_UmbracoMediaPath_to_absolute_path branch January 6, 2022 12:35
@shearer3000
Copy link

yay 😁

@creativesuspects
Copy link

Great work!

@shearer3000
Copy link

shearer3000 commented Feb 10, 2022

hi guys - I just upgraded to umbraco 9.3.0 and set "UmbracoMediaPhysicalRootPath" as per the blog post with a UNC path but it didn't work, i just get 404s for all my media. I also can't find any documentation about these settings apart from what is in the blog post if there is anything extra i need to configure? thanks

@bergmania
Copy link
Member Author

@shearer3000, are you able to test while mapping the UNC path to a drive? I have tested using my NAS that that was mapped to Z:\.

@shearer3000
Copy link

Hi @bergmania is universal naming convention not supported for this setting? We don’t use mapped drives

@bergmania
Copy link
Member Author

bergmania commented Feb 11, 2022

I had expected it to work, but just tested and see the same issue as you 🤦‍♂️ . Currently the workaround is to map the UNC to a drive.

@bergmania
Copy link
Member Author

I think another workaround would be to call

  builder.SetMediaFileSystem(factory =>
            {
                IIOHelper ioHelper = factory.GetRequiredService<IIOHelper>();
                IHostingEnvironment hostingEnvironment = factory.GetRequiredService<IHostingEnvironment>();
                ILogger<PhysicalFileSystem> logger = factory.GetRequiredService<ILogger<PhysicalFileSystem>>();
                GlobalSettings globalSettings = factory.GetRequiredService<IOptions<GlobalSettings>>().Value;

                var rootPath = Path.IsPathRooted(globalSettings.UmbracoMediaPhysicalRootPath) ? globalSettings.UmbracoMediaPhysicalRootPath : hostingEnvironment.MapPathWebRoot(globalSettings.UmbracoMediaPhysicalRootPath);
                var rootUrl = hostingEnvironment.ToAbsolute(globalSettings.UmbracoMediaPath);
                return new PhysicalFileSystem(ioHelper, hostingEnvironment, logger, rootPath, rootUrl);
            });

Where builder is the IUmbracoBuilder

@AreYouSureYouKen
Copy link

Hi @bergmania @shearer3000 I came here from a different issue mentioned earlier. I had tried both using the UNC which places the new media files inside wwwroot/[ipaddress]/[rest of path] and mapping the UNC to a drive.
However if I map it to a drive, say (S:) I get the error that (part) of the path cannot be found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants