U4-8751 - GetCropUrl should append the quality parameters as the last parameter to the URL (after furtherOptions) when the format is specified

Created by Asbjørn Riis-Knudsen 20 Jul 2016, 10:27:34 Updated by Sebastiaan Janssen 04 Aug 2016, 09:40:29

Tags: Up For Grabs PR

The GetCropUrl method currently has a parameter for setting the image quality. However, if you also want to specify the format, you have to use the furtherOptions parameter, which is appended to the end of the query string.

This, however, results in the quality parameter being put before the format parameter, and per ImageProcessor's design, the default quality value of the Jpeg format will then override whatever you have specified (the order matters). So you end up having to put both quality and format in the furtherOptions parameter.

Quality should always come after format, never the other way around.

The GetCropUrl method should have an additional parameter for format, so that it can be ensured that the paramters are output in the proper order. Or, alternatively, the quality parameter should be removed, but I don't think that's a good idea.

Comments

Sebastiaan Janssen 20 Jul 2016, 10:32:08

Sounds good to me! Are you up for helping out with this?

Yeah, don't remove parameters, that's going to break things for people already using it! :-)


Asbjørn Riis-Knudsen 20 Jul 2016, 10:33:01

@sebastiaan Yep, will have a look at it sometime this week, then :) Should be straightforward.


Jeavon Leopold 20 Jul 2016, 10:53:30

We should add the format parameter to ensure it's before quality, also the parameter should probably be a string rather than a Enum as ImageProcessor is pluggable for example the webp plugin. There are a lot of different methods that need updating with the new parameter though and you need to be careful about breaking dynamics support.


Sebastiaan Janssen 20 Jul 2016, 11:01:33

Wouldn't it be as "easy" as adding another GetCropUrl overload that has the format parameter and then changing the one method that generates the querystring to put the format before the quality?


Jeavon Leopold 20 Jul 2016, 11:13:36

We will still need to add a new parameter to the existing method in order for it to ensure that format goes in before quality though, is that considered breaking otherwise you would have to duplicate the entire method for a new overload?????

Also there are a few GetCropUrl methods

The methods that actually create the ImageProcessor strings (and are used as legacy IPublishedContent Extensions for dynamics support) https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Web/ImageCropperTemplateExtensions.cs

The URL Helpers https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Web/UrlHelperRenderExtensions.cs

Obsolete HTML Helpers (don't update these) https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Web/HtmlHelperRenderExtensions.cs


Asbjørn Riis-Knudsen 20 Jul 2016, 11:35:58

Is it a problem to add an extra optional parameter at the end? That was certainly was I was thinking of doing - adding a new parameter and setting it to null by default. You obviously can't add it in the middle, because you would be changing the order (though I highly doubt that anyone does not use named parameters for all the optional settings in that method).


Sebastiaan Janssen 20 Jul 2016, 12:00:18

Yes, one extra optional paremeter is a breaking change because you change the signature of the method. We need an extra method with and extra parameter unfortunately.

You don't have to duplicate the method, just defer to the new method from the old method. So if you had this before:

public string Whatever(string something) { var new = something + ", woohoo!"; }

then you can add a method and for example do:

public string Whatever(string something) { return Whatever(something, string.Empty); }

public string Whatever(string something, string somethingElse) { var new = something + ", woohoo!"; if(string.IsNullOrEmpty(somethingElse) == false) { new = somethingElse + ": " + new; } return new; }


Asbjørn Riis-Knudsen 20 Jul 2016, 12:15:26

@sebastiaan Thanks for the information. That's what I'll do then. But you would still put the extra parameter at the end when making the new method? I'd rather have it next to quality, would that break compatibility?


Sebastiaan Janssen 20 Jul 2016, 12:18:29

It will just be confusing for people trying to use autocomplete. Try it out, see how it goes or if it's even possible. That method has so many params, not even sure how that's going to turn out! ;-)


Jeavon Leopold 20 Jul 2016, 12:52:22

I think it should go at the end for sure otherwise it will be mega confusing as one overload would have a different order to another


Asbjørn Riis-Knudsen 23 Jul 2016, 16:02:12

I've been working on a PR for this, but I'm starting to think it's not possible to do so. If I add new method as discussed above, keeping the existing one in place as a proxy, I'm getting ambiguous method errors, because it can't choose between the two methods with all the same optional parameters. If you don't have the format parameter specified (as would be the case with all existing code), it cannot figure out which of the two methods you want.

An alternate fix would be to change the implementation of the method that generates the URL so that the quality parameters is appended as the last parameter to the URL (after furtherOptions) and then do the fix properly in v8. How does that sound to you?


Jeavon Leopold 23 Jul 2016, 19:27:13

I would suggest we parse furtherOptions for format and prepend before quality if found in there for v7 and in v8 add it in its proper place


Asbjørn Riis-Knudsen 24 Jul 2016, 16:12:58

A PR implementing the alternate approach is here: https://github.com/umbraco/Umbraco-CMS/pull/1398


Sebastiaan Janssen 02 Aug 2016, 13:08:09

Thanks @arknu ! That's much easier. Updated two small things : https://github.com/umbraco/Umbraco-CMS/commit/e020779a147feb37893f0c859eb71a3e4974b8b9


Jeavon Leopold 04 Aug 2016, 08:27:17

Thanks @arknu !


Asbjørn Riis-Knudsen 04 Aug 2016, 08:35:27

@sebastiaan Should we maybe change the title of this issue to reflect that we haven't actually added the format parameter, but merely fixed the order in v7 and then file a new issue for adding the parameter in v8? Just so people aren't confused? I'd be happy to do the PR for v8 as well :)


Sebastiaan Janssen 04 Aug 2016, 09:40:29

@arknu Done! Make sure to create a new issue related to this one for v8, would be wonderful!! Thanks!


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.5.0, 7.4.2, 7.4.3

Due in version: 7.5.0

Sprint: Sprint 39

Story Points:

Cycle: