umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
Other
4.49k stars 2.69k forks source link

NetCore: Use API to get Image URLs in BackOffice instead of hardcoded querystrings #7540

Closed bergmania closed 4 years ago

bergmania commented 4 years ago

Background History

For the .NET Core transformation, we need to be able to abstract the image URL generation, because ImageProcessor is not supported in .NET Core.

The task

This task is about changing a variety of Umbraco back-office JavaScript to use the ImageUrlGenerationController introduced in issue #7539. This will generate image URLs with various parameters in a way that doesn’t hardcode the parameter names or format. The following JavaScript files currently generate image URLs by appending query string parameters and will need to be changed to use the controller method:

Media.controller.js - lines 68-75, 82
Tinymce.service.js - line 308, 1496
Fileupload.controller.js - line 54
Imagecropper.controller.js - line 222

Even though this change is for the .NET Core transition, it should be a non-breaking change that makes sense to implement on Umbraco 8 (v8/dev).

bielu commented 4 years ago

@bergmaniain .net core we could use that instead: https://github.com/SixLabors/ImageSharp/issues maybe it makes sense to look how we could use ImageSharp .net core and Image processor still in framework?

bergmania commented 4 years ago

Hi @bielu.. we are aware of ImageSharp, but the change in license is not a perfect match. We expect that it will be possible to implement multiple different image manipulation libraries, such that a ImageSharp could make sense for companies that would like the commercial license.

bielu commented 4 years ago

@bergmania from my understanding of change of licence as long they will not release RC, all OSS could reference them and use them even after release RC on same Apache licence, based on that what was said here: https://github.com/SixLabors/ImageSharp/issues/1024 @JimBobSquarePants could you correct me if I am wrong about that?

JimBobSquarePants commented 4 years ago

@bielu Introduce it as a dependency now and you get to preserve the Apache license.

bielu commented 4 years ago

@JimBobSquarePants thanks for confirmation :)

bielu commented 4 years ago

@bergmania, in that case, should we look into how to integrate imagesharp into Umbraco with the possibility to switch to any image processor?

bergmania commented 4 years ago

@bielu, I think it makes sense to do. I theory it is a task a little bit like this https://github.com/umbraco/Umbraco-CMS/issues/7541, just with ImageSharp instead of SkiaSharp.

For now, I'm afraid we cannot use ImageSharp.Web and the middleware provided there. because we don't have updated the executable to ASP.NET Core yet. But I'm sure we can still use the functionality in ImageSharp in a Module or a Controller that justs delegates the work.

Some work is already started here: https://github.com/umbraco/Umbraco-CMS/pull/7573

bielu commented 4 years ago

@bergmania I think we still could do that using .web but it is starting beeing more tricky as the best would be to use here conditional targets.

bergmania commented 4 years ago

@bielu, IMO it doesn't make sense to introduce conditional targets for this. When we have updated Umbraco.Web.UI to ASP.NET Core, I bet it is pretty easy to use the middleware as intended.

bielu commented 4 years ago

@bergmania depends on the roadmap if we would cover both (.net framework, .net core), it makes sense to use conditional to introduce ImageSharp only in case .core, and still use ImageProcessor on .net framework implementation.

bergmania commented 4 years ago

@bielu, ImageSharp is .NETStandard 2.0, so I'm not sure why that could not be used for .NET Framework 4.6.1+`

bielu commented 4 years ago

@bergmania for the case of ImageSharp.Web answer: https://github.com/SixLabors/ImageSharp.Web/issues/39

bergmania commented 4 years ago

@bielu,

Alright, from what I can see, the issue is more that we expect to target ASP.NET Core 3.1 and not ASP.NET Core 2.1 that still had support for .NET Framework?

Overall we don't aim for having multiple targets - Only .NET Core. We just try to keep as many doors open to support SqlCE as possible, because changing this to e.g. SqlLite is a big task our of scope for the .NET Core project.

IMO it makes sense to wait with ImageSharp.Web to Umbraco.Web.UI is updated.

JimBobSquarePants commented 4 years ago

My advice.

Treat url and general image manipulation as a separate feature. Invest in a single IMediaFileProvider that can resolve and push media based upon an id.

Something like... (Very simple examples, just to show pattern)

/// <summary>
/// Specifies the contract for managing media from different locations.
/// </summary>
public interface IMediaFileProvider
{
    /// <summary>
    /// Gets the media item by id.
    /// </summary>
    /// <param name="id">The id of the media to retrieve.</param>
    /// <returns>The <see cref="Task{TResult}"/>.</returns>
    Task<IMediaFileInfo> GetAsync(Guid id);

    /// <summary>
    /// Adds a media item to the provider.
    /// </summary>
    /// <param name="media">The media to add.</param>
    /// <returns>The <see cref="Task"/>.</returns>
    Task AddAsync(IMediaFileInfo media);

    /// <summary>
    /// Updates a media item in the provider.
    /// </summary>
    /// <param name="media">The media to update.</param>
    /// <returns>The <see cref="Task"/>.</returns>
    Task UpdateAsync(IMediaFileInfo media);

    /// <summary>
    /// Delets a media item from the provider.
    /// </summary>
    /// <param name="id">The id of the media to delete.</param>
    /// <returns>The <see cref="Task"/>.</returns>
    Task<bool> DeleteAsync(Guid id);
}

Your media file info should be something like this...

public interface IMediaFileInfo : IMedia
{
    /// <summary>
    /// Gets the id.
    /// </summary>
    Guid Id { get; }

    /// <summary>
    /// Gets the name.
    /// </summary>
    string Name { get; }

    /// <summary>
    /// Asynchronously opens the media stream.
    /// </summary>
    /// <returns>The <see cref="Task{Stream}"/>.</returns>
    Task<Stream> OpenReadStreamAsync();
}

This method below. Is the key to a successful, performant, API. It brings you the power to lazily resolve the actual media data.

Task<Stream> OpenReadStreamAsync()

byte[] is an absolute no-go for performance. (I saw that in the linked PR).

You want all your image requests to go through a single pipeline that, in turn, relies on the IMediaFileProvider. Multiple service/controller implementations are a bed of snakes.

Anything client side should rely on a URL pipeline would ultimately be Middleware (Net Core) or a HttpModule (NET Framework). Anything else happens far too late in the request lifecycle.

JimBobSquarePants commented 4 years ago

SqlCE support from Microsoft ends in July 2021 btw.

bergmania commented 4 years ago

SqlCE support from Microsoft ends in July 2021 btw.

Thanks, we are aware. The SqlCE support is just to make it as easy to get started as possible. - Even for non-developers.

benjaminc commented 4 years ago

I implemented the precursor issue #7539 in PR #7616, but I'll be honest, I'm not entirely sure the best way to implement the needed changes to the four JavaScript files listed on this ticket. All the existing calls are synchronous, of course, but with the updates a server call is needed. If someone else wanted to take a stab at the JavaScript changes, that'd be great.

ylinssen commented 4 years ago

Working on the altered implementation for the JS-files.

ronaldbarendse commented 3 years ago

Where doing PR https://github.com/umbraco/Umbraco-CMS/pull/10623, I noticed we still have some hardcoded URLs in the back-office and there are actions to generate crop URLs in both ImagesController and ImageUrlGenerationController.