Skip to content

Conversation

rasmusjp
Copy link
Member

@rasmusjp rasmusjp commented Apr 16, 2019

fixes #4942

Breaking changes

  • UrlProvider constructors now takes an IEnumerable<IMediaUrlProvider>
  • UmbracoContextFactory constructor now takes a MediaUrlProviderCollection

Things to test

  • media.Url returns the url for the umbracoFile property,
  • media.UrlAbsolute() returns the absolute url for the umbracoFile property.
  • media.GetCropUrl(200, 200) returns the crop url

Add a Umbraco.UploadField and a Umbraco.ImageCropper property to a content item and test the following both as invariant and variant

  • content.MediaUrl('propertyAlias') returns the url for the media in the propertyAlias property
  • content.MediaUrlAbsolute('propertyAlias') returns the absolute url for the media in the propertyAlias property
  • content.MediaUrl('propertyAlias', "culture") returns the url for the media in the propertyAlias property in the specified culture
  • content.MediaUrlAbsolute('propertyAlias', "culture") returns the absolute url for the media in the propertyAlias property in the specified culture
  • content.GetCropUrl(200, 200 propertyAlias: "propertyAlias") returns the crop url

Crete a new implementation IMediaUrlProvider which always returns a full url and add it as the first one in the MediaUrlProviderCollection e.g.

public class CdnMediaUrlProvider : DefaultMediaUrlProvider
{
    public override UrlInfo GetMediaUrl(UmbracoContext umbracoContext, IPublishedContent content, string propertyAlias,
        UrlProviderMode mode, string culture, Uri current)
    {
        var mediaUrl = base.GetMediaUrl(umbracoContext, content, propertyAlias, UrlProviderMode.Relative, culture, current);
        if (mediaUrl == null)
            return null;

        if (mediaUrl.IsUrl == false)
            return mediaUrl;

        var cdnBaseUrl = "https://my.cdn";

        return UrlInfo.Url(cdnBaseUrl + mediaUrl.Text, culture);
    }
}

public class CdnComposer : IUserComposer
{
    public void Compose(Composition composition)
    {
        composition
            .MediaUrlProviders()
            .Insert<CdnMediaUrlProvider>();
    }
}

and test that all the above url's now is prefixed with https://my.cdn

@ghost ghost assigned rasmusjp Apr 16, 2019
@rasmusjp rasmusjp removed their assignment Apr 16, 2019
@Shazwazza
Copy link
Contributor

nice, haven't had time to fully look at this one but just wanted to ping @zpqrtbnk since he'll also have an interest in this one too.

@sitereactor
Copy link
Contributor

What does this implementation mean for regular every day use of Umbraco?

  1. Anything that needs to be done differently to get the media path/url through this provider?
  2. Any scenarios that are not covered? Like content.GetPropertyValue("propertyAlias")

Just trying to get an idea of what would work and potentially what wouldn't when you have a CDN provider in place. Is there potential for getting media Urls without the CDN prefix.

@Shazwazza
Copy link
Contributor

@sitereactor not sure if you've read all of the related issues? This is what this is fixing: #4942, this is the epic that contains it: https://github.com/umbraco/Umbraco.Private/issues/283, this is the spike with some research done #4943
... otherwise please keep asking questions :)

@zpqrtbnk zpqrtbnk self-requested a review April 17, 2019 05:59
@sitereactor
Copy link
Contributor

Okay, I will try to think of another way to ask my question so you will understand. From your reply I gather its not clear 😅

@sitereactor
Copy link
Contributor

How about this:

Given all the tasks/subtasks mentioned in the #283 Epic are fully implemented. Will the use of getting a path/url to a media item be completely seemless to the user or is there anything that would require the developers to use different methods?

Say I'm on version 8.0.x and all the tasks from the Epic is implemented in 8.x and I upgrade to this version and configure a media provider to add a CDN to the site. Will everything Just Work™️ ?

@Shazwazza
Copy link
Contributor

Yes.. it will 'just work', that's the whole premise for these changes, we don't want the user to have to do new/different things.

@sitereactor
Copy link
Contributor

Great, thank you.

@rasmusjp
Copy link
Member Author

rasmusjp commented Apr 17, 2019

For media .Url and .GetCropUrl() will use the url provider instead of just returning the umbracoFile property value, but if you have a upload/image cropper field on your content and uses content.Value() it will return the property value (either a string or ImageCropperValue) and it's not using the url providers, so you'll need to use content.MediaUrl() instead.

I guess we could make content.Value() work by changing the value converters for upload and image cropper, but not sure what would be the best approach here, maybe @zpqrtbnk has some input?

@sitereactor
Copy link
Contributor

What about from a package developers perspective? Anything special they'd need to be aware of? Something around how to handle media paths within content.
Sorry about my lack of insight here. Just trying to understand.

@ghost ghost assigned zpqrtbnk Apr 23, 2019
throw new ArgumentOutOfRangeException(nameof(mode));
}

return uri.Rewrite(UriUtility.ToAbsolute(uri.GetSafeAbsolutePath()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this does what we want. Calling ToAbsolute on an already absolute url is... going to do weird things? Looking into it.

@zpqrtbnk
Copy link
Contributor

@rasmusjp I have pushed a commit that does some minor fixes, want to check I haven't done anything stupid?

Now running tests.

As for the behavior of Value() etc - see #5170 - we're going to refactor Url() and more generally the IPublishedContent interface in 8.1, so I think some of the methods defined in this PR will change again with #5170.

Ideally, Value() returns "the value" whatever that is, and Url() returns "the url". So I'd rather not use Value() to return a Url. And by default Url() should work with... umbracoFile if present, so ppl don't have to supply the property alias, by default. But can, if they want to override.

So in a view... <img src="@media.Url()" /> should "just" work - and if a dev replaces the media url provider, she can control what .Url() returns here.

Making sense?

@zpqrtbnk zpqrtbnk merged commit c19be0c into v8/dev Apr 23, 2019
@zpqrtbnk zpqrtbnk deleted the v8/feature/4942-media-url-provider branch April 23, 2019 17:33
@ronaldbarendse
Copy link
Contributor

How do you get an absolute crop URL? GetCropUrl() should probably get an additional parameter to specify the UrlMode that is passed to MediaUrl for this, right?

var mediaItemUrl = mediaItem.MediaUrl(propertyAlias: propertyAlias);

@ronaldbarendse
Copy link
Contributor

And all GetCropUrl() extension methods should probably also allow specifying a culture, so all MediaUrl() overloads are covered:

public static string MediaUrl(this IPublishedContent content, string culture = null, UrlMode mode = UrlMode.Default, string propertyAlias = Constants.Conventions.Media.File)

@ronaldbarendse
Copy link
Contributor

BTW: adding support for culture variant media types will be required first: #4771.

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.

Create media Url provider
5 participants