callumbwhyte / super-value-converters

A collection of powerful property value converters for cleaner code in Umbraco
MIT License
14 stars 12 forks source link

MNTP Super Value Converter does not appear to be working #20

Closed NikRimington closed 2 years ago

NikRimington commented 2 years ago

Hi Callum,

I've just spun up a fresh install of Umbraco v9.2 and installed this package. I've created a simple doc type with no template called "Category" which can be created at root. I've also created a simple doc type called "Home" which can be created at root. I then created a data type using the MNTP and setting it to only allow Category types to be picked, then added this as a property to the Home doc type

Finally, for visibility, I switched Models Builder mode to be "SourceCodeAuto" and, after a restart, told the site to regenerate the models.

I had hoped that the property on the Home node would be of type IEnumerable<Category> but unfortunately it is still "just" IEnumerable<IPublishedContent>

[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Umbraco.ModelsBuilder.Embedded", "9.2.0+763cb70e677ac0c85557b19b5df09eccfa1b9dfb")]
        [global::System.Diagnostics.CodeAnalysis.MaybeNull]
        [ImplementPropertyType("cats")]
        public virtual global::System.Collections.Generic.IEnumerable<global::Umbraco.Cms.Core.Models.PublishedContent.IPublishedContent> Cats => this.Value<global::System.Collections.Generic.IEnumerable<global::Umbraco.Cms.Core.Models.PublishedContent.IPublishedContent>>(_publishedValueFallback, "cats");

image image

callumbwhyte commented 2 years ago

Thanks for the detail @NikRimington, taking a look!

NikRimington commented 2 years ago

Adding to this, I've copied the code out of the package into CS files directly in my project and the converter works. I suspect the issue is around disabling the core PVC and registering the custom one.

NikRimington commented 2 years ago

@callumbwhyte is there a reason you aren't explicitly adding the value converters?

callumbwhyte commented 2 years ago

Okay so this was a stupid mistake on my part... 😆

At startup SuperValueConverters removes each core converter it is replacing from the PropertyValueConvertersCollection, but also ensures that each type remains in the DI container - this is so we can inject the original core converter into our overriding container, for fallbacks etc.

In both SuperValueConverters and Umbraco the MNTP converter is called MultiNodeTreePickerValueConverter... However the Composer to override that at startup is looking at the wrong namespace, therefore trying to remove the converter we actually want! 🤦‍♂️

v3.0.1 will be up on NuGet very shortly @NikRimington.

NikRimington commented 2 years ago

I'll give it a test in the morning and let you know if it works :-D

NikRimington commented 2 years ago

Hey @callumbwhyte , I can confirm release 3.0.1 has fixed the issue :-) Sorry it took so long for me to test!