Skip to content
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.

Sign up for GitHub

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

V8 - Add Media Tracking of items added via Macro Parameters in RTE + Grid - issue #8131 #8614

Closed
wants to merge 9 commits into from
Closed

V8 - Add Media Tracking of items added via Macro Parameters in RTE + Grid - issue #8131 #8614

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4eb54e4
Add Parser to find Udi's in Macro Parameters in Macros in RTE + Grid
marcemarc Aug 10, 2020
bf5b356
Register HtmlMacroParameterParser with DI
marcemarc Aug 10, 2020
5a0bc3d
Some Tests to cover the parsing of text/json for Macros and their par…
marcemarc Aug 10, 2020
3b38bc6
Inject the HtmlMacroParameterParser into GridPropertyEditor + RichTex…
marcemarc Aug 10, 2020
b7c937b
Add GetAll implementation to MacroService for multiple Macro Aliases
marcemarc Aug 17, 2020
ed0180f
Remove previous tests based on scraping the embedded parameter udis
marcemarc Aug 17, 2020
e398926
Update Macro Parameter Parser, to use the Macro Configuration, to fin…
marcemarc Aug 17, 2020
7ab1028
Update Grid + RichTextEditor Property Editors to use the MacroParamet…
marcemarc Aug 17, 2020
27007ce
Update MultiMediaPicker Parameter editor to have it's own ValueEditor…
marcemarc Aug 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 154 additions & 0 deletions src/Umbraco.Tests/Templates/HtmlMacroParameterParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
using Umbraco.Core.Logging;
using Moq;
using NUnit.Framework;
using Umbraco.Tests.Testing.Objects.Accessors;
using Umbraco.Web.Templates;
using Umbraco.Web;
using Umbraco.Core.Models.PublishedContent;
using Umbraco.Web.Routing;
using Umbraco.Tests.Testing.Objects;
using System.Web;
using System;
using System.Linq;
using Umbraco.Core.Models;
using Umbraco.Core;
using System.Diagnostics;
using Newtonsoft.Json.Linq;
using System.Collections.Generic;

namespace Umbraco.Tests.Templates
{
[TestFixture]
public class HtmlMacroParameterParserTests
{
[Test]
public void Returns_Udis_From_Single_MediaPicker_Macro_Parameters_In_Macros_In_Html()
{
//Two macros, one single parameter, single Media Picker, one multiple paramters of single media picker
var input = @"<p>This is some normal text before the macro</p>
<?UMBRACO_MACRO macroAlias=""ThreeSingleMediaPickers"" singleMediaPicker1=""umb://media/eee91c05b2e84031a056dcd7f28eff89"" singleMediaPicker2=""umb://media/fa763e0d0ceb408c8720365d57e06e32"" singleMediaPicker3="""" />
<p>This is a paragraph after the macro and before the next</p>
<?UMBRACO_MACRO macroAlias=""SingleMediaPicker"" singleMediaPicker=""umb://media/90ba0d3dba6e4c9fa1953db78352ba73"" />
<p>some more text</p>";

var macroParameterParser = new HtmlMacroParameterParser();

var result = macroParameterParser.FindUdisFromMacroParameters(input).ToList();
Assert.AreEqual(3, result.Count);
Assert.AreEqual(Udi.Parse("umb://media/eee91c05b2e84031a056dcd7f28eff89"), result[0]);
Assert.AreEqual(Udi.Parse("umb://media/fa763e0d0ceb408c8720365d57e06e32"), result[1]);
Assert.AreEqual(Udi.Parse("umb://media/90ba0d3dba6e4c9fa1953db78352ba73"), result[2]);
}
[Test]
public void Returns_Empty_With_From_Single_MediaPicker_With_No_Macro_Parameters_In_Macros_In_Html()
{
//Two macros, one single parameter, single Media Picker, one multiple paramters of single media picker
var input = @"<p>This is some normal text before the macro</p>
<?UMBRACO_MACRO macroAlias=""ThreeSingleMediaPickers"" />
<p>This is a paragraph after the macro and before the next</p>
<?UMBRACO_MACRO macroAlias=""AnotherParameter"" singleMediaPicker=""<p>Some other value</p>"" />
<p>some more text</p>";

var macroParameterParser = new HtmlMacroParameterParser();

var result = macroParameterParser.FindUdisFromMacroParameters(input).ToList();
Assert.AreEqual(0, result.Count);
}
//NB: When multiple media pickers store udis instead of ints! - see https://github.com/umbraco/Umbraco-CMS/pull/8388
[Test]
public void Returns_Empty_When_No_Macros_In_Html()
{
//Two macros, one single parameter, single Media Picker, one multiple paramters of single media picker
var input = @"<p>This is some normal text before the macro</p>
<p>This is a paragraph after the macro and before the next</p>
<p>some more text</p>";

var macroParameterParser = new HtmlMacroParameterParser();

var result = macroParameterParser.FindUdisFromMacroParameters(input).ToList();
Assert.AreEqual(0, result.Count);
}
[Test]
public void Returns_Udis_From_Multiple_MediaPicker_Macro_Parameters_In_Macros_In_Html()
{
//Two macros, one single parameter, single Media Picker, one multiple paramters of single media picker
var input = @"<p>This is some normal text before the macro</p>
<?UMBRACO_MACRO macroAlias=""MultipleMediaPickers"" multipleMediaPicker1=""umb://media/eee91c05b2e84031a056dcd7f28eff89,umb://media/fa763e0d0ceb408c8720365d57e06e32,umb://media/bb763e0d0ceb408c8720365d57e06444"" />
<p>This is a paragraph after the macro</p>";

var macroParameterParser = new HtmlMacroParameterParser();

var result = macroParameterParser.FindUdisFromMacroParameters(input).ToList();
Assert.AreEqual(3, result.Count);
Assert.AreEqual(Udi.Parse("umb://media/eee91c05b2e84031a056dcd7f28eff89"), result[0]);
Assert.AreEqual(Udi.Parse("umb://media/fa763e0d0ceb408c8720365d57e06e32"), result[1]);
Assert.AreEqual(Udi.Parse("umb://media/bb763e0d0ceb408c8720365d57e06444"), result[2]);
}
[Test]
public void Returns_Udis_From_Single_MediaPicker_Macro_Parameters_In_Grid_Macros()
{
// create a list of GridValue.GridControls with Editor GridEditor alias macro
List<GridValue.GridControl> gridControls = new List<GridValue.GridControl>();

// single media picker macro parameter
var macroGridControl = GetMacroGridControl(@"{ ""macroAlias"": ""SingleMediaPicker"", ""macroParamsDictionary"": { ""singleMediaPicker"": ""umb://media/90ba0d3dba6e4c9fa1953db78352ba73"" }}");
gridControls.Add(macroGridControl);

var macroParameterParser = new HtmlMacroParameterParser();

var result = macroParameterParser.FindUdisFromGridControlMacroParameters(gridControls).ToList();
Assert.AreEqual(1, result.Count);
Assert.AreEqual(Udi.Parse("umb://media/90ba0d3dba6e4c9fa1953db78352ba73"), result[0]);

}
[Test]
public void Returns_Empty_From_Single_MediaPicker_With_No_Macro_Parameters_In_Grid_Macros()
{
// create a list of GridValue.GridControls with Editor GridEditor alias macro
List<GridValue.GridControl> gridControls = new List<GridValue.GridControl>();

// single media picker macro parameter
var macroGridControl = GetMacroGridControl(@"{ ""macroAlias"": ""SingleMediaPicker"", ""macroParamsDictionary"": {}}");
gridControls.Add(macroGridControl);

var macroParameterParser = new HtmlMacroParameterParser();

var result = macroParameterParser.FindUdisFromGridControlMacroParameters(gridControls).ToList();
Assert.AreEqual(0, result.Count);

}
//NB: When multiple media pickers store udis instead of ints! - see https://github.com/umbraco/Umbraco-CMS/pull/8388
[Test]
public void Returns_Udis_From_Multiple_MediaPicker_Macro_Parameters_In_Grid_Macros()
{
// create a list of GridValue.GridControls with Editor GridEditor alias macro
List<GridValue.GridControl> gridControls = new List<GridValue.GridControl>();

// multiple media picker macro parameter
var macroGridControl = GetMacroGridControl(@"{ ""macroAlias"": ""SingleMediaPicker"", ""macroParamsDictionary"": { ""multipleMediaPicker"": ""umb://media/eee91c05b2e84031a056dcd7f28eff89,umb://media/fa763e0d0ceb408c8720365d57e06e32,umb://media/bb763e0d0ceb408c8720365d57e06444"" }}");
gridControls.Add(macroGridControl);

var macroParameterParser = new HtmlMacroParameterParser();

var result = macroParameterParser.FindUdisFromGridControlMacroParameters(gridControls).ToList();
Assert.AreEqual(3, result.Count);
Assert.AreEqual(Udi.Parse("umb://media/eee91c05b2e84031a056dcd7f28eff89"), result[0]);
Assert.AreEqual(Udi.Parse("umb://media/fa763e0d0ceb408c8720365d57e06e32"), result[1]);
Assert.AreEqual(Udi.Parse("umb://media/bb763e0d0ceb408c8720365d57e06444"), result[2]);

}

//setup a Macro Grid Control based on Json of the Macro
private GridValue.GridControl GetMacroGridControl(string macroJson)
{
var macroGridEditor = new GridValue.GridEditor();
macroGridEditor.Alias = "macro";
macroGridEditor.View = "macro";
var macroGridControl = new GridValue.GridControl();
macroGridControl.Editor = macroGridEditor;
macroGridControl.Value = JToken.Parse(macroJson);
return macroGridControl;
}

}
}
1 change: 1 addition & 0 deletions src/Umbraco.Tests/Umbraco.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@
<Compile Include="Services\MemberGroupServiceTests.cs" />
<Compile Include="Services\MediaTypeServiceTests.cs" />
<Compile Include="Services\PropertyValidationServiceTests.cs" />
<Compile Include="Templates\HtmlMacroParameterParserTests.cs" />
<Compile Include="Templates\HtmlLocalLinkParserTests.cs" />
<Compile Include="TestHelpers\RandomIdRamDirectory.cs" />
<Compile Include="Testing\Objects\TestDataSource.cs" />
Expand Down
57 changes: 47 additions & 10 deletions src/Umbraco.Web/PropertyEditors/GridPropertyEditor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public class GridPropertyEditor : DataEditor
{
private IUmbracoContextAccessor _umbracoContextAccessor;
private readonly HtmlImageSourceParser _imageSourceParser;
private readonly HtmlMacroParameterParser _macroParameterParser;
private readonly RichTextEditorPastedImages _pastedImages;
private readonly HtmlLocalLinkParser _localLinkParser;
private readonly IImageUrlGenerator _imageUrlGenerator;
Expand All @@ -39,22 +40,33 @@ public GridPropertyEditor(ILogger logger,
HtmlImageSourceParser imageSourceParser,
RichTextEditorPastedImages pastedImages,
HtmlLocalLinkParser localLinkParser)
: this(logger, umbracoContextAccessor, imageSourceParser, pastedImages, localLinkParser, Current.ImageUrlGenerator)
: this(logger, umbracoContextAccessor, imageSourceParser, pastedImages, localLinkParser, Current.Factory.GetInstance<HtmlMacroParameterParser>(),Current.ImageUrlGenerator)
{
}
[Obsolete("Use the constructor which takes an HtmlMacroParameterParser")]
public GridPropertyEditor(ILogger logger,
IUmbracoContextAccessor umbracoContextAccessor,
HtmlImageSourceParser imageSourceParser,
RichTextEditorPastedImages pastedImages,
HtmlLocalLinkParser localLinkParser,
IImageUrlGenerator imageUrlGenerator)
: this(logger, umbracoContextAccessor, imageSourceParser, pastedImages, localLinkParser, Current.Factory.GetInstance<HtmlMacroParameterParser>(), imageUrlGenerator)
{
}

public GridPropertyEditor(ILogger logger,
IUmbracoContextAccessor umbracoContextAccessor,
HtmlImageSourceParser imageSourceParser,
RichTextEditorPastedImages pastedImages,
HtmlLocalLinkParser localLinkParser,
HtmlMacroParameterParser macroParameterParser,
IImageUrlGenerator imageUrlGenerator)
: base(logger)
{
_umbracoContextAccessor = umbracoContextAccessor;
_imageSourceParser = imageSourceParser;
_pastedImages = pastedImages;
_localLinkParser = localLinkParser;
_macroParameterParser = macroParameterParser;
_imageUrlGenerator = imageUrlGenerator;
}

Expand All @@ -72,6 +84,7 @@ internal class GridPropertyValueEditor : DataValueEditor, IDataValueReference
{
private readonly IUmbracoContextAccessor _umbracoContextAccessor;
private readonly HtmlImageSourceParser _imageSourceParser;
private readonly HtmlMacroParameterParser _macroParameterParser;
private readonly RichTextEditorPastedImages _pastedImages;
private readonly RichTextPropertyEditor.RichTextPropertyValueEditor _richTextPropertyValueEditor;
private readonly MediaPickerPropertyEditor.MediaPickerPropertyValueEditor _mediaPickerPropertyValueEditor;
Expand All @@ -83,23 +96,34 @@ public GridPropertyValueEditor(DataEditorAttribute attribute,
HtmlImageSourceParser imageSourceParser,
RichTextEditorPastedImages pastedImages,
HtmlLocalLinkParser localLinkParser)
: this(attribute, umbracoContextAccessor, imageSourceParser, pastedImages, localLinkParser, Current.ImageUrlGenerator)
: this(attribute, umbracoContextAccessor, imageSourceParser, pastedImages, localLinkParser,Current.Factory.GetInstance<HtmlMacroParameterParser>(), Current.ImageUrlGenerator)
{
}
[Obsolete("Use the constructor which takes an HtmlMacroParameterParser")]
public GridPropertyValueEditor(DataEditorAttribute attribute,
IUmbracoContextAccessor umbracoContextAccessor,
HtmlImageSourceParser imageSourceParser,
RichTextEditorPastedImages pastedImages,
HtmlLocalLinkParser localLinkParser,
IImageUrlGenerator imageUrlGenerator)
: this(attribute, umbracoContextAccessor, imageSourceParser, pastedImages, localLinkParser, Current.Factory.GetInstance<HtmlMacroParameterParser>(), imageUrlGenerator)
{
}

public GridPropertyValueEditor(DataEditorAttribute attribute,
IUmbracoContextAccessor umbracoContextAccessor,
HtmlImageSourceParser imageSourceParser,
RichTextEditorPastedImages pastedImages,
HtmlLocalLinkParser localLinkParser,
HtmlMacroParameterParser macroParameterParser,
IImageUrlGenerator imageUrlGenerator)
: base(attribute)
{
_umbracoContextAccessor = umbracoContextAccessor;
_imageSourceParser = imageSourceParser;
_pastedImages = pastedImages;
_richTextPropertyValueEditor = new RichTextPropertyEditor.RichTextPropertyValueEditor(attribute, umbracoContextAccessor, imageSourceParser, localLinkParser, pastedImages, _imageUrlGenerator);
_richTextPropertyValueEditor = new RichTextPropertyEditor.RichTextPropertyValueEditor(attribute, umbracoContextAccessor, imageSourceParser, localLinkParser, macroParameterParser, pastedImages, _imageUrlGenerator);
_mediaPickerPropertyValueEditor = new MediaPickerPropertyEditor.MediaPickerPropertyValueEditor(attribute);
_macroParameterParser = macroParameterParser;
_imageUrlGenerator = imageUrlGenerator;
}

Expand All @@ -125,7 +149,7 @@ public override object FromEditor(ContentPropertyData editorValue, object curren
var mediaParent = config?.MediaParentId;
var mediaParentId = mediaParent == null ? Guid.Empty : mediaParent.Guid;

var grid = DeserializeGridValue(rawJson, out var rtes, out _);
var grid = DeserializeGridValue(rawJson, out var rtes, out _, out _);

var userId = _umbracoContextAccessor.UmbracoContext?.Security?.CurrentUser?.Id ?? Constants.Security.SuperUserId;

Expand Down Expand Up @@ -158,7 +182,7 @@ public override object ToEditor(Property property, IDataTypeService dataTypeServ
var val = property.GetValue(culture, segment)?.ToString();
if (val.IsNullOrWhiteSpace()) return string.Empty;

var grid = DeserializeGridValue(val, out var rtes, out _);
var grid = DeserializeGridValue(val, out var rtes, out _, out _);

//process the rte values
foreach (var rte in rtes.ToList())
Expand All @@ -172,15 +196,16 @@ public override object ToEditor(Property property, IDataTypeService dataTypeServ
return grid;
}

private GridValue DeserializeGridValue(string rawJson, out IEnumerable<GridValue.GridControl> richTextValues, out IEnumerable<GridValue.GridControl> mediaValues)
private GridValue DeserializeGridValue(string rawJson, out IEnumerable<GridValue.GridControl> richTextValues, out IEnumerable<GridValue.GridControl> mediaValues, out IEnumerable<GridValue.GridControl> macroValues)
{
var grid = JsonConvert.DeserializeObject<GridValue>(rawJson);

// Find all controls that use the RTE editor
var controls = grid.Sections.SelectMany(x => x.Rows.SelectMany(r => r.Areas).SelectMany(a => a.Controls)).ToArray();
richTextValues = controls.Where(x => x.Editor.Alias.ToLowerInvariant() == "rte");
mediaValues = controls.Where(x => x.Editor.Alias.ToLowerInvariant() == "media");

// Find all the macros
macroValues = controls.Where(x => x.Editor.Alias.ToLowerInvariant() == "macro");
return grid;
}

Expand All @@ -196,7 +221,7 @@ public IEnumerable<UmbracoEntityReference> GetReferences(object value)
if (rawJson.IsNullOrWhiteSpace())
yield break;

DeserializeGridValue(rawJson, out var richTextEditorValues, out var mediaValues);
DeserializeGridValue(rawJson, out var richTextEditorValues, out var mediaValues, out var macroValues);

foreach (var umbracoEntityReference in richTextEditorValues.SelectMany(x =>
_richTextPropertyValueEditor.GetReferences(x.Value)))
Expand All @@ -205,6 +230,18 @@ public IEnumerable<UmbracoEntityReference> GetReferences(object value)
foreach (var umbracoEntityReference in mediaValues.SelectMany(x =>
_mediaPickerPropertyValueEditor.GetReferences(x.Value["udi"])))
yield return umbracoEntityReference;

//Macros don't really have a Property Editor for which we can call GetRefererences
//where does the responsibility lie for MacroParametersEditors to report their references?
//when we don't easily know the property type for a parameter - without 'looking up' which would be expensive?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but eventually we want to track all sorts of things... basically anything that can have a UDI reference. Could be content -> content, etc... Macros do have a property editor, they are the same. They are just property editors flagged with also being a macro parameter editor. This is exposed by IDataEditor.EditorType enum. But you can 'just' resolve all macro parameter editors from the collection ParameterEditorCollection. Don't think that would be expensive get the editor based on the editor alias ... if you have it. IIRC that is stored as part of the embedded macro parameters?

Copy link
Contributor Author

@marcemarc marcemarc Aug 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What I'm trying to say is that the Grid, has a property editor - the class is called GridPropertyEditor, it is responsible for the references to udis in the Grid - The Rich Text Editor has a property editor - the class is called RichTextPropertyEditor - but the Macro isn't a 'property editor' in the 'same way' it doesn't have a MacroPropertyEditor class (or maybe I can't find it! :-)) so it doesn't have it's own GetReferences implementation (which following the pattern would 'call' all the 'GetReferences' for each of the Macro Parameters, which of course are tied to their own PropertyEditor implementations...

Maybe we should have a 'Macro Picker' property editor that people can use for Doc Type properties? (like the old Macro Container)

So what I'm trying to say, is a Macro is a bit different, as it isn't an editor in its own right - and I'm not sure how to make it follow the same pattern of responsibility (a property editor is responsible for finding the references!) - which is why I created the HtmlMacroParameterParser to take on this task...

Having created that, and with the warnings about performance:

image

it occurred to me that pragmatically having the values in udi format means you 'know' what type they are - even if you want to add content item tracking at a later date - it will have a udi//document format ... you will know it's type without needing to know how it was picked... (the argument against this approach is what about complex custom macro property editors that store Json... but ignore that for now)

... but I'm aware this doesn't follow the pattern and the notion of property editor responsibility (which is good, but I don't think Macros follow this pattern which is the problem I'm encountering or misunderstanding).

The embedded Macro markup looks like this:

<?UMBRACO_MACRO macroAlias=""InsertImageCarousel"" carouselImages=""umb://media/eee91c05b2e84031a056dcd7f28eff89,umb://media/fa763e0d0ceb408c8720365d57e06e32,umb://media/bb763e0d0ceb408c8720365d57e06444"" />

and in the case of the grid:

{ ""macroAlias"": ""InsertImageCarousel"", ""macroParamsDictionary"": { ""carouselImages"": ""umb://media/eee91c05b2e84031a056dcd7f28eff89,umb://media/fa763e0d0ceb408c8720365d57e06e32,umb://media/bb763e0d0ceb408c8720365d57e06444"" }}

So you DO not have the alias of the property editor stored as part of the embedded macro parameters - just the alias of the Macro, and the name given by the developer to the parameter...

... so 'including' the property editor alias in the embedded markup would be another way of making this much easier!! :-)

but in the absence of this, my thinking here is we'd need to lookup the configured Macro, by Macro Alias, read the parameter configuration, establish the editor type for each parameter, get the editor from the ParameterEditorCollection, call it's GetReferences method - passing in the value of that parameter from the embedded macro...

... I don't think the ParameterEditorCollection look up is the expensive bit... it's the querying to get the Macro configuration? which will happen for every macro - even when it doesn't have a media picker, just to find out, it doesn't have a media picker - unless there is a better way?

whereas the pragmatic thing here is we know from the Macro Parameter Value what 'type' of udi it is, and it is all we need to add the tracking! Macros are special!

But can rework the PR to try and lookup the macro, establish parameter editor type, and call getreferences if you confirm that is 'just' the preferred approach for the longer term, but if there is a better way to do the lookup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the gist of what it would be looking up the macro each time:

 private IEnumerable<UmbracoEntityReference> GetMacroParameterUmbracoEntityReferences(string macroAlias, Dictionary<string,string> macroParameters)
        { 
                //lookup macro configuration to find type of parameter editor
                // could we store/cache this somewhere
                // is there a better way of getting this info?
                IMacro macroConfig = _macroService.GetByAlias(macroAlias);
                foreach (var parameter in macroConfig.Properties)
                {
                    var parameterValue = macroParameters[parameter.Alias];
                    var parameterEditorAlias = parameter.EditorAlias;
                    //lookup propertyEditor from core current ParameterEditorCollection
                    var parameterEditor = _parameterEditors.FirstOrDefault(f => f.Alias == parameterEditorAlias);
                    if (parameterEditor != null)
                    {
                        //get the ParameterValueEditor for this PropertyEditor (where the GetReferences method is implemented) - cast As IDataValueReference to determine if it is implemented
                        IDataValueReference parameterValueEditor = parameterEditor.GetValueEditor() as IDataValueReference;
                        if (parameterValueEditor != null)
                        {
                            foreach (var entityReference in parameterValueEditor.GetReferences(parameterValue))
                            {
                                yield return entityReference;
                            }                            
                        }
                    }
                }
            }

it's certainly neater, but does that seem ok, with using the MacroService?

I can update along these lines if it make sense (and I can work out how to inject the ParameterEditorCollection properly).

Copy link
Contributor

@Shazwazza Shazwazza Aug 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah yes the grid is it's own beast as well since 'grid editors' aren't property editors either. I think there's probably another issue with the grid, references and macros too since the GridPropertyEditor.GetReferences is not taking into account the macro picker grid editor either.

I do much prefer your 2nd approach above using the macro service. We are most likely going to have to do this in a few places moving forward like for the grid + rte, the RTE itself (there's already a TODO there: //TODO: Detect Macros too ... but we can save that for a later date, right now need to do media refs) , nested content + rte, block editor + rte (perhaps the block editor has a macro picker too, can't remember) so doing this the right way and having a central bit of code to do this all will be good.

Regarding performance, yes that will cause quite a few queries to execute since we are doing N+1 all over the place. But we can deal with this, there's a couple options and perhaps they are both things we might want to do:

  1. Adding a macro service method IEnumerable<IMacro> GetAll(params string[] aliases) which will query for all macros by those aliases. This wouldn't require any changes to the macro repository because we can construct a query to pass to the repo for that. Then instead of doing N+1 queries, we can just get all macros by all aliases at once. This would greatly reduce the queries but still means one query to this new macro service per RTE and macro editor. I think that would still be ok though.
  2. Add caching by alias to the macro repository and change the macro service to call this new repository method. I just did a similar thing to member's by username in this branch, see the diff here v8/8.6...v8/bugfix/8433-member-login-sql-locks , specifically this is how to do custom caching v8/8.6...v8/bugfix/8433-member-login-sql-locks#diff-713b853430e7cca3f31d799ccdcc9671R689 with a custom cache policy but that also means we need to update the macro cache refresher too :) ... I'd be happy to help out or point you in the right direction for this but there's a lot of examples in that branch

1 ) above is pretty easy. I'd start with that. 2 ) is probably good to do anyways because we use the GetByAlias call in quite a few places in the core and we already have hacked in a cache for it inside of MacroRenderer.Render and it's definitely less than ideal to have this hacked cache in there when it should definitely be part of the macro repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks @Shazwazza for the direction, I'll rework the PR to take this approach! I was just wary of the n+1, but as you say, there are ways around reducing the impact of that - and this is much neater, the responsibility of finding the Udis is with the property editor implementation which feels right.

The PR also handled the Grid, and the Macro Grid Controls and their parameters, which were parsed in the same 'pragmatic' way, this can remain but instead call this new common method to do the entity reference finding.

Let's see how far I get!

//pragmatically we only care if the parameter has a value that is a media udi eg umb://media
//so we 'could' just loop through all parameter values here and add references for any values that are media udis... eg they are in use! however they are picked
//Is the HtmlMacroParameterParser the right place to put this method?
var udis = _macroParameterParser.FindUdisFromGridControlMacroParameters(macroValues);
foreach (var udi in udis)
{
yield return new UmbracoEntityReference(udi);
}
}
}
}
Expand Down
Loading