umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.42k stars 2.67k forks source link

Return IEnumerable<T> in Nested Content with single document type #4739

Closed bjarnef closed 5 years ago

bjarnef commented 5 years ago

At the moment when using Nested Content in core and with ModelsBuilder, you can use the following to get a collection as IEnumerable<IPublishedContent>.

FrontPage site = Model.Content.Site() as FrontPage;
IEnumerable<IPublishedContent> advertisements = site.Advertisements;

or the following to cast IPublishedContent when you only use a single document type in Nested Content.

IEnumerable<Advertisement> advertisements = site.Advertisements.OfType<Advertisement>();

However when Nested Content only is configurated to use a single document type it would be great if the converter generated IEnumerable<T> instead of IEnumerable<IPublishedContent> similar to how the MultiUrl Picker (now in core) generates Link or IEnumerable<Link> based on the configuration. Example from v8 (but should be pretty much the same in latest v7). https://github.com/umbraco/Umbraco-CMS/blob/91c52cffc8b7c70dc956f6c6610460be2d1adc51/src/Umbraco.Web/PropertyEditors/ValueConverters/MultiUrlPickerValueConverter.cs#L28-L31

kjac commented 5 years ago

@bjarnef Nested Content already does that.

You can see it if you set Umbraco.ModelsBuilder.ModelsMode to LiveAppData and inspect the generated models.

Here's how the generated model looks with a single element type (of type NC1) configured in the Nested Content property called list:

image

..and here's how the same model looks with two or more element types configured in the Nested Content property:

image

bjarnef commented 5 years ago

@kjac okay, then it might be a bug when using Dll mode as we use with ModelsBuilder. When Nested Content is configurated only with a single document type it always return IEnumerable<IPublishedContent>> and we then need to cast this to the specific model type.

bjarnef commented 5 years ago

@kjac I had a look at the default startkerkit and how the Features (which use Nested Content) on Product nodes are generated.

image

Using PureLive mode:

///<summary>
/// Features
///</summary>
[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Umbraco.ModelsBuilder", "8.0.1")]
[ImplementPropertyType("features")]
public IEnumerable<Feature> Features => this.Value<IEnumerable<Feature>>("features");

Will have a look at Dll mode.

bjarnef commented 5 years ago

Switching this to Dll mode an generate this property on the model.

///<summary>
/// Features
///</summary>
[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Umbraco.ModelsBuilder", "8.0.1")]
[ImplementPropertyType("features")]
public IEnumerable<Feature> Features => this.Value<IEnumerable<Feature>>("features");

Maybe it has been updated in v8, since I get this in Umbraco v7.13.2 running Umbraco.ModelsBuilder v3.0.10

image

I guess the ModelsBuilder version in v8 might be a bit different from ModelsBuilder v3.0.10 in Umbraco v7. https://www.nuget.org/packages/Umbraco.ModelsBuilder/

@zpqrtbnk are there some major changes betweeen ModelsBuilder v3.0.10 and v8.0.0? Is MB v8 supported in Umbraco v7? Or could MB v3.0.10 get an update so it returns e.g. IEnumerable<Feature> instead of IEnumerable<IPublishedContent>> in case Nested Content only is configurated with a single document type.

... or maybe it is not actually an issue with ModelsBuilder but the property value converter for Nested Content, which might have this update in v8, but not in latest v7.

bjarnef commented 5 years ago

Okay, it seems to be some differences in Nested Content value converter from v7 to v8.

In v7

https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Web/PropertyEditors/ValueConverters/NestedContentSingleValueConverter.cs

https://github.com/umbraco/Umbraco-CMS/blob/dev-v7/src/Umbraco.Web/PropertyEditors/ValueConverters/NestedContentPublishedPropertyTypeExtensions.cs

In v8

https://github.com/umbraco/Umbraco-CMS/blob/dev-v8/src/Umbraco.Web/PropertyEditors/ValueConverters/NestedContentSingleValueConverter.cs

https://github.com/umbraco/Umbraco-CMS/blob/dev-v8/src/Umbraco.Web/PropertyEditors/ValueConverters/NestedContentValueConverterBase.cs

zpqrtbnk commented 5 years ago

Catching up on this issue.

Models Builder in v8 has been enhanced to do precisely what you want. If you pick only 1 possible element type, say Thing, you'd get an IEnumerable<Thing> out of Nested Content.

Running Dll mode or another mode should not make a difference, the mode only changes what we do with the generated code and how we compile it - not how it's generated.

Backporting the changes to v7 would be tricky, as we had to break backward compatibility in Core in various places in order to support this. I don't think we'll want to invest the time to do it.

Making sense?

kjac commented 5 years ago

@zpqrtbnk I couldn't agree more. The backwards compatibility issue is potentially too big to justify implementing this in V7.

bjarnef commented 5 years ago

@zpqrtbnk yes, it makes sense and great it now has been added to v8. It makes it much easier and in v7 I already cast the collection to the specific type, when Nested Content only use a single document type. It also make it easier from code to discover if Nested Content use single or multiple document types.

zpqrtbnk commented 5 years ago

cool - closing this issue then, feel free to open another one if you have any further question

ronaldbarendse commented 5 years ago

@bjarnef It's possible to override the default NestedContentSingleValueConverter with a custom one that includes the changes from the V8 implementation. Issue https://github.com/umbraco/Umbraco-CMS/issues/2925 shows how and when the PR gets merged, it would be even easier (please include your comments/thoughts there to speed it up 😉).