V8 - Update multi media picker macro parameter to store Udis

[marcemarc]

This has come up as part of my investigation into making Media picked in a Macro - tracked using the new Media Tracking features - see #8131

Essentially I've found a weird anomaly, which might have a perfectly good reason for why it is like this or it might have just been missed in the switch to udis, I'm illustrating it in this PR, but this might be a 'breaking change' - so would need some thought.

The anomaly is a Single Media Picker used as a 'Macro Parameter' stores it's picked value as a Udi, but the Multiple Media Picker Macro Parameter stores it's picked values in the old way as integers.

eg:

<div class="umb-macro-holder mceNonEditable loading"><!-- <?UMBRACO_MACRO macroAlias="latestBlogposts" numberOfPosts="4" startNodeId="umb://document/1d770f10d1ca4a269d68071e2c9f7ac1" multipleMediaPicker="1141,1142,1143" singleMediaPicker="umb://media/90ba0d3dba6e4c9fa1953db78352ba73" /> --> <ins>Macro alias: <strong>latestBlogposts</strong></ins></div>

(now I want to parse this using regexes for the Media Tracking, but that will be much easier if the references are stores as Udis, and much more performant, not having to convert ids to udis)

The Multiple Media Picker is based on the same underlying property editor for picking Media as the Single Media Picker. But there is a configuration option to tell it to store udis or not - so to make it do the Udis I want, I just add the following to the ParameterEditor configuration:

DefaultConfiguration.Add("idType", "udi");

And we get Udis!!

<div class="umb-macro-holder mceNonEditable"><!-- <?UMBRACO_MACRO macroAlias="latestBlogposts" numberOfPosts="4" startNodeId="umb://document/1d770f10d1ca4a269d68071e2c9f7ac1" multipleMediaPicker="umb://media/eee91c05b2e84031a056dcd7f28eff89,umb://media/fa763e0d0ceb408c8720365d57e06e32,umb://media/c0969cab13ab4de9819a848619ac2b5d" singleMediaPicker="umb://media/90ba0d3dba6e4c9fa1953db78352ba73" /> --> <ins>

But is it set to be integer ids for a reason? - eg length of parameter string could get quite long? is it purposeful to use ints here?

Then I was thinking, we won't 'just be able to change this' - it will break people's site on upgrade, or will it be so bad? Won't the code to read in the string of picked ids, 'just work' - will people be parsing the picked items as integers?

We don't have ValueConverters for Macro Parameter reading, so something like this:

var pickedMediaParameter = Model.GetParameterValue
("multipleMediaPicker");
var pickedMediaIds = pickedMediaParameter.Split(new char[]{','},StringSplitOptions.RemoveEmptyEntries);
var pickedMedia = Umbraco.Media(pickedMediaIds);

Would work for both "1141,1142,1143" AND "umb://media/eee91c05b2e84031a056dcd7f28eff89,umb://media/fa763e0d0ceb408c8720365d57e06e32,umb://media/c0969cab13ab4de9819a848619ac2b5d"

So I tried it... and found that it didn't! so I dug into it a little bit to find out why...

... and I found that the problem was in the UmbracoHelper Media method that takes in an array of objects...

public IEnumerable<IPublishedContent> Media(params object[] ids)

This first tries to parse the array as integers, and then falls back to parse the array as Guids...

... but I have a string array of Udis...

and the act of parsing these as Guids, returned nothing and broke the Macro...

... if I add the option of parsing the id as a Udi before falling through to try to parse as a Guid, all was ok:

   internal static bool ConvertIdObjectToGuid(object id, out Guid guidId)
        {
            switch (id)
            {
                case string s:
                    //could be a Udi string and not a pure Guid
                    if (Udi.TryParse(s, out Udi udi))
                    {
                        var guidUdi = udi as GuidUdi;
                        if (guidUdi != null)
                        {
                            guidId = guidUdi.Guid;
                            return true;
                        }
                    }               
                    return Guid.TryParse(s, out guidId);

                case Guid g:
                    guidId = g;
                    return true;

                default:
                    guidId = default;
                    return false;
            }
        }

But I'm aware that this is in a much-used part of Umbraco and so wonder if instead I've gotten confused and missing something obvious. I've added the update to UmbracoHelper in this PR too.

So with this in place, IF people were reading and using multiple media picker parameters this way in their macros then it wouldn't be MUCH of a breaking change? or do we add a new Multi Media Picker 2, macro parameter and make the existing version obsolete? or is it ints for a good reason and we should leave alone?

Prerequisites

• [ y] I have added steps to test this contribution in the description below

To Test this contribution, Spin up a version of the site with a starter kit, create a new macro that has two macro parameters, one a multi media picker parameter, another a single media picker - implement the macro to write out any media images picked by the macro, and insert the macro into a page and pick some images. You'll see the anomaly with the multi media picker storing as ids, and the single picker storing as udi. Then pull this branch, and point at the same site - you should find without updating anything content wise the page will still render the macro, even though it's still got the integer ids picked, and this fix is in place. Then in the backoffice open the macro inside the rich text area, and submit and save. The picked ids will switched to udis (but the important thing is the items that were 'picked' are still displayed as 'picked' you don't need to repick them as part of the udi storage change. Now publish the page, and your Macro should still render the images.

Thanks for considering this contribution to Umbraco CMS!

[nul800sebastiaan]

Thanks @marcemarc - As far as I can tell this is a perfectly valid way of doing it and: yay, no more int ids in one more place.

However, you did guess this already, some of our depenendents will expect an int to be returned here, I would guess @KevinJump will run into this and also our very own Deploy will need to be updated. I am really not in favor of adding a Multi Media Picker 2 as we know how easily this leads to confusion (f.ex. we get into these types of areas https://www.owain.codes/blog/posts/2020/july/obsolete-umbraco-property-types/).

Deploy is expecting either an int or a Guid, I'm thinking maybe it's good to switch to using a Guid here instead? Then Deploy doesn't need updating and we KNOW that this is a media picker so we don't need the full Udi with schema here. What do you think?

[marcemarc]

@nul800sebastiaan agreed on the whole Media Picker 2 thing...

But... I really want the comma-delimited string of picked values here to be stored as full Udis, it's because the context here is I want to go fishing for those udis using regexes, in order to add them to the list of Media that is being tracked as being used on the page.

With full Udis I wouldn't need to know that the particular parameter is a media picker

(I'm trying to avoid further 'lookups' at the stage of parsing the rich text editor, that would be required for each parameter in each macro to establish it's type - because this info isn't stored in the markup that gets injected into the rich text editor for a Macro).

Because if it's not a Media Picker parameter editor - and it's somehow storing a parameter with a Udi string that contains 'media' - then iI don't care it's not a media picker - it's still a Media item that is in use on the page and needs to be tracked! or I'm very lazy :-)

but here is the thing, and with respect to Umbraco Deploy

The Single Media Picker Parameter Editor and the Multiple Media Picker Parameter Editor use the same underlying editor and the Single Media Picker has been updated to use Udis.

So Deploy is currently working (presumably) with a Parameter editor that picks a single media item and stores a Udi - so for consistency sakes it would make sense to make this switch to store Udis for the Multi Media Picker parameter editor as well?
(remember we're only talking about the Multi Media Picker when it's used as a parameter editor - so it would be the part of Deploy that handles moving rich text editor around and the parameters embedded in macros rather than the bit that handles a stand alone Multi Media Picker.)

So it must be close to 'just working' or possibly currently broken if it's relying on int ids!

But agree we should alert @KevinJump to the situation

[nul800sebastiaan]

I'll need to carefully read your comments here @marcemarc but for clarification, this is the code for macros in Deploy that is expecting int identifiers currently, as noted in the code comment:

// stores: 1061,1062
if (editorAlias == "Umbraco.MultipleMediaPicker")
{
	var newValues = new StringBuilder();
	foreach (var v in value.Split(new[] { ","}, StringSplitOptions.RemoveEmptyEntries))
	{
		if (direction == Direction.ToArtifact)
		{

			int id;
			if (!int.TryParse(v, out id))
				return v;

			var guidAttempt = _entityService.GetKey(id, UmbracoObjectTypes.Media);
			var guid = guidAttempt.Success ? guidAttempt.Result : Guid.Empty;

			if (guid != Guid.Empty)
			{
				var udi = new GuidUdi(Constants.UdiEntityType.Media, guid);
				if (dependencies.Contains(udi) == false)
					dependencies.Add(udi);
				newValues.Append(udi + ",");
			}
		}
		else
		{
			GuidUdi udi;
			if (!GuidUdi.TryParse(v, out udi))
				return v;

			if (udi.Guid == Guid.Empty)
				return string.Empty;

			var idAttempt = _entityService.GetId(udi.Guid, UmbracoObjectTypes.Media);
			var id = idAttempt.Success ? idAttempt.Result : 0;

			if (id != 0)
			{
				newValues.Append(id + ",");
			}
		}
	}
	return newValues.ToString().TrimEnd(',');
}

Does that clarify the problem? We'd need to add Udi to this and make sure that causes v8.8.0+ to have a hard minimal dependency on whatever Deploy version this will be fixed in.

IIRC, you can easily get a Udi from a Guid anyway, no?

[marcemarc]

@nul800sebastiaan Yes, but at the part of doing the regex parsing - I DON'T KNOW IF IT's A MEDIA ITEM :-P ... but if it's a Udi it has media in the string - I do!

Otherwise the parameter might be a content item guid...

or I'll have to use the parameter alias to lookup to determine if it's a media picker...

The code in Umbraco Deploy is taking either integer or Guid, but IT IS converting it into a Udi in order to use it...

... so you'd save all that effort, and make Deploy quicker! if we made this change...

but appreciate the dependency of Umbraco Deploy on when this was merged in - but worth it in the long run?

[nul800sebastiaan]

Ah I get it, in order to generate a Udi, you need to give it a type..

Can the regex be super smart and do a lookback for multipleMediaPicker? Does it need to be a regex, it's XML, use your regex to find the whole <div class="umb-macro-holder... </div> element and parse as XML? Sorry, I'm just throwing wild thoughts out there and will need to re-read the use case later. Just trying to do 3 things at a time at the moment 🙈

[marcemarc]

@nul800sebastiaan The embedded Macro looks like this:

<div class="umb-macro-holder mceNonEditable loading"><!-- <?UMBRACO_MACRO macroAlias="latestBlogposts" numberOfPosts="4" startNodeId="umb://document/1d770f10d1ca4a269d68071e2c9f7ac1" gallery="1141,1142,1143" mainSnap="umb://media/90ba0d3dba6e4c9fa1953db78352ba73" /> --> <ins>Macro alias: <strong>latestBlogposts</strong></ins></div>

Within the rich text area...

so you can see, you can't tell if it's a media picker... but if the parameters store the Udi... you can be sure it's related media!

[nul800sebastiaan]

gallery="1141,1142,1143"

CLICK. Got it. Will give it a think soon! 👍

[marcemarc]

thanks @nul800sebastiaan - I can appreciate the rigmarole of coordinating deploy + a release - but I reckon it might be the right thing to do...

[umbrabot]

Hiya @marcemarc!

We wanted to thank you for your work and let you know that we have spent some time reviewing the contribution and have decided that we are going to close this one.

While reviewing this PR, the team left the following additional note:

Causes issues around changing data with existing editors and deploy - better to change in a new major version - remove legacy int ids and changes to udi.

We appreciate that you have worked hard to build and document this contribution to create the changes you want to see but given the status of Umbraco 8, moving into LTS and Umbraco 9 being ready and stable and packed with new features, we are working through the open PRs and deciding which are a good fit for the last version 8 minor and which are not.

If you'd like to know a little more about this process, please check out this blog post, explaining how we came to the decisions we have made here. We'd like to reassure you though that the CMS team, along with some special guests, took the time to assess each PR that is being closed and decided how to proceed - whilst the message you're receiving is automatic, the work behind it was done by the people that make up the CMS team, along with community members.

If there are specifics around this PR that you'd like explained that are not covered by the article, please let us know.

All the best to you, our wonderful contributor.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[marcemarc]

Thanks for taking the time to consider this PR

It's fascinating how little mistakes like this become baked into the product for such a long period of time, but agree you can't 'just' change it now... hope we'd all agree though that for the SAME underlying parameter editor, it's nuts if you configure it to be single, it stores udis, and configure it to be multiple it stores integer ids!

(and deploy is doing extra work here... and Ids... well they are not guaranteed to be unique between Umbraco instances so... anomalies!!)

How do you track issues like this for making this kind of change in an upcoming major version? this has been closed rather than labeled with 'Must Do In Next Version'??? - There are too many open PRs for you to be able to get a snapshot in an instant of those issues that can only be done in a major release... so inevitable, as has happened here... you will work on the new version, be pressured with time to get it ready for a release deadline... then feel guilty about community PRs against the old version, and go through and say oh, that can only be done in a Major version change....

How can we break that cycle?

Would it be better strategy to label PRs with a 'Needs Major Version' label and then review these labelled PRs before you start the next version? and then those that you want to include don't get lost in the PR swamp?

(Also it's completely fine to close this PR, and say we never want this change. We like the crazy quirkiness of multimedia macro parameters, it's what gives the product its charm... or to say in v-next... we are killing macros anyway etc... )

It's not clear if you want this PR retargeted for V10 or to just go away! happy either way!

[nul800sebastiaan]

@marcemarc I'm sure this could have been worded better but the idea, when we don't encourage it for a next version is "it's not a priority and not a big enough problem for us to encourage more work on it". That's the most honest answer we have for these types of PRs at the moment. Other PRs have been closed with an explicit encouragement to try again for v9+ and the group that evaluated those did think it would be something we'd want help changing soon(-ish).

In this case, I was not in this group evaluating, I think it makes total sense to do this for v10. Again, not a priority for us, so we'd love to see a new PR, but won't be tracking this actively.

Hope that makes sense!