Custom composition properties not implemented in composed classes

[zpqrtbnk]

If type DocType is composed (not parent, just composition) of type Compo, then when class DocType is generated it implements the properties defined in the ICompo interface. However, if these properties are customized by the user in her own Compo partial, then they are ignored and not implemented in class DocType, and the user has to implement them manually.

[hfloyd]

Example copied from other issue:

The CompPageSettings DocType:

The Homepage DocType:

Customized Model code:

namespace MySite.Web.Models
{
    using Umbraco.Core.Models;
    using Umbraco.Web;

    using Zbu.ModelsBuilder;

    public partial interface ICompPageSettings : IPublishedContent
    {
        /// <summary>Meta Description</summary>
        string MetaDescription { get; }

        /// <summary>Meta Keywords</summary>
        string MetaKeywords { get; }

        /// <summary>Meta Title</summary>
        string MetaTitle { get; }
    }

    public partial class CompPageSettings
    {
        /// <summary>Meta Description</summary>
        [ImplementPropertyType("MetaDescription")]
        public string MetaDescription
        {
            get { return GetMetaDescription(this); }
        }

        /// <summary>Static getter for Meta Description</summary>
        public static string GetMetaDescription(ICompPageSettings that)
        {
            var meta = that.GetPropertyValue("MetaDescription").ToString();
            return meta;
        }

        /// <summary>Meta Keywords</summary>
        [ImplementPropertyType("MetaKeywords")]
        public string MetaKeywords
        {
            get { return GetMetaKeywords(this); }
        }

        /// <summary>Static getter for Meta Keywords</summary>
        public static string GetMetaKeywords(ICompPageSettings that)
        {
            var meta = that.GetPropertyValue("MetaKeywords").ToString();
            return meta;
        }

        /// <summary>Meta Title</summary>
        [ImplementPropertyType("MetaTitle")]
        public string MetaTitle
        {
            get { return GetMetaTitle(this); }
        }

        /// <summary>Static getter for Meta Title</summary>
        public static string GetMetaTitle(ICompPageSettings that)
        {
            var meta = that.GetPropertyValue("MetaTitle").ToString();
            return meta != "" ? meta : that.Name;
        }

    }
}

[zpqrtbnk]

The difficulty is, as soon as a property is customized, basically anything can happen, and so it is very difficult to figure out what to generate. We cannot "just" generate a call to the composition static class mixin getter, because if you customized the property, then maybe there isn't even such a getter.

In the case of the linked issue, it is only the type of the property that changes, and the static getter is still here, but that's only a specific case. So I think the rule: "if you customize it, you are on your own" is valid.

Yet I understand that the situation can be painful.

Right now, the easier way to deal with it would be, in my mind, to create proper IPropertyValueConverter implementations for these properties, so the Models Builder knows that they are strings and not objects.

Need to think about ways to deal with it though. We could have an attribute to redefine the CLR type of a property, without having to re-implement it. Eg:

[ForcePropertyType("metaTitle", typeof (string))]
public class CompSettings
{}

[hfloyd]

Ideally, for any DocTypes which implement the interface of a Composition (ICompPageSettings), the following would be added automatically to the generated code:

/// <summary>Meta Description</summary>
[ImplementPropertyType("MetaDescription")]
public string MetaDescription
{
    get { return CompPageSettings.GetMetaDescription(this); }
}

/// <summary>Meta Keywords</summary>
[ImplementPropertyType("MetaKeywords")]
public string MetaKeywords
{
    get { return CompPageSettings.GetMetaKeywords(this); }
}

/// <summary>Meta Title</summary>
[ImplementPropertyType("MetaTitle")]
public string MetaTitle
{
    get { return CompPageSettings.GetMetaTitle(this); }
}

So the properties are implemented on the DocType, but referencing the TYPE defined in the customized interface partial and calling the "Get...()" methods from the customized file as well.

[zpqrtbnk]

Not sure we can do this. It assumes that CompPageSettigns.GetMetaTitle exists, but because you indicated that you were implementing the property, we have no (reliable) way to be sure.

See my other proposals though.

[hfloyd]

I understand your concern with the customizing causing a free-for-all...

[hfloyd]

One possibility would be that if properties were not being implemented using the "standard" defined, the developer would need to use "IgnorePropertyType" to have it not generated automatically?

[zpqrtbnk]

Yes, I like the idea that customizing => up to you. But we can help deal with your situation. Implementing the ForcePropertyType attribute should not be too complicated, and it's easy to understand. Though a converter would be nicer, probably.

[hfloyd]

The ForcePropertyType could work when the type needs to be changed. Would that be set on the CLASS or on the INTERFACE?

[zpqrtbnk]

Not sure yet, need to think about it.

[hfloyd]

Also, what about customized props where the TYPE doesn't need to change - but the logic for the "Get...() function would be altered - like the GetMetaTitle() example?

[zpqrtbnk]

Ah... for that last one you want to also override the static getter implementation, yet tell me that there is a static getter and please use it? So we would need a [ImplementStaticGetter] attribute of some sort?

I need to go through all these attributes and try to come with something that makes sense ;-)

[hfloyd]

Hi Stephan, Yes, I suppose that the main advantage of Compositions in the umbraco back-office is to easily add groups of properties to multiple Doctypes, without having to add the same "new" property in multiple locations...

And the corresponding advantage of Compositions on the Models side would be the ability to define custom logic once, and have it apply across all related DocTypes utilizing the Composition...

Thanks for your consideration of this issue :-)

[hfloyd]

Hi Stephan,

I had another different idea about this today...

What if instead of an interface implementation... the Composition Properties were treated as their own sub-object?

I haven't quite worked this out in VS (would screw up my current project royally, I think, considering the current generation logic), but I'd imagine that it would be something like this...

The "Comp..." classes would be generated similarly to non-Comp classes, with an additional ObjectGetter(?)...

public partial class CompPageSettings
{
    ...
    /// <summary>Meta Description</summary>
    [ImplementPropertyType("MetaDescription")]
    public string MetaDescription
    {
        get { return this.GetPropertyValue("MetaDescription").ToString(); }
    }

    /// <summary>Meta Keywords</summary>
    [ImplementPropertyType("MetaKeywords")]
    public string MetaKeywords
    {
        get { return this.GetPropertyValue("MetaKeywords").ToString(); }
    }

    /// <summary>Meta Title</summary>
    [ImplementPropertyType("MetaTitle")]
    public string MetaTitle
    {
        get { 
            var meta = this.GetPropertyValue("MetaTitle").ToString();
            return meta != "" ? meta : this.Name;
        }
    }

    ...

    /// <summary>Static getter for CompPageSettings Object</summary>
    public static CompPageSettings GetObject(IPublishedContent that)
    {
        ...code to populate the sub-object and return it - Just like the standard classes
    }
}

Then the Classes which utilize the Comp would just have a single additional property added:

[RenameCompositionProperty("CompPageSettings", "PageSettings")]
public partial class Homepage
{
    ...

    /// <summary>CompPageSettings Object</summary>
    [ImplementComposition("CompPageSettings")]
    public CompPageSettings PageSettings
    {
        get { return return CompPageSettings.GetObject(this); }
    }

    ...
}

All this pseudo-code of course would be split between the ".generated" and custom files... but perhaps you get the idea.

Referencing the composition properties then would be like this:

@Model.PageSettings.MetaTitle

rather than

@Model.MetaTitle

Anyway, I know it's a departure from the current way it works, so perhaps should require a web.config option to generate Comps like this...

Also, we'd need a way to "define" which classes ARE Comps, right? Or does the current code already figure that out? Well, if not, it could be added to the "ModelBuilder.cs" file... like

[assembly: Composition("CompPageSettings", "PageSettings")]

(Hey - even handle the "universal renaming" of the "property", rather than defining it at the class level...)

What do you think of this new insanity? ;-)

[zpqrtbnk]

Hey. Sorry I haven't been able to find more time recently to work on that issue / improvement. That being said:

We do automatically figure out which classes are compositions (it is, well, those classes that are used in compositions). It would be pretty easy to generate the code you suggest, so I certainly should look into making it an option.

Not sure everybody would love the content.Meta.MetaTitle especially when compositions are used to inject some very common properties such as title or author... Need to think about it!

[hfloyd]

Hi Stephan! No worries, I know you have other things to work on ;-) It was just something I came up against again yesterday so it was on my mind and I know our conversation before had been related to managing whatever property customization would be added by developers. I guess the quantity of additional attributes we were discussing needing to add to implement the feature made me think of an alternate way to handle it, with less heavy-lifting required on the part of the generation code.

I can understand that people might not like to have the extra "levels" of properties, though some people might like the natural "groupings" of common properties. It's a personal preference thing.

Thanks for your continued engagement with this :-)

[Korayem]

This amazing plugin deserves some work to support nesting levels as suggested by @hfloyd

In addition to easier overriding of Data Type generation across any Property on Document Types. For instance, I am using Nested Content along with Vorto Multilingual extensively which requires usage of HasVortoValue() and GetVortoValue() instead of HasValue() and GetPropertyValue(). To setup things.

The amount of manual work required renders Zbu.ModelsBuilder almost useless for me. This tool deserves developing the project further to allow extensibility over custom Data Types by decorating classes with [ImplementDataType("VortoValue")] to be universal as well as supporting nested Document types.

[zpqrtbnk]

@Korayem Cannot work on ModelsBuilder before the end of the months but have plans to start a new iteration in January. @hfloyd has given pretty detailed infos about what she would need. I'd love if you could elaborate here (ie... almost write specs?) and if we can discuss what we want.

[Korayem]

Awesome news @zpqrtbnk! According to my discussion here https://github.com/lars-erik/Umbraco.CodeGen/issues/13, it came to my knowledge that Zbu.ModelsBuilder will be part of Umbraco in the next release?

[TomSteer]

Hey... I've just come across this issue, now i'm looking at using compositions more with the Umb 7.4 change. I think the introduction of the ForcePropertyType attribute would be a good solution as this should then cover all grounds along with the work that was in done for issue 51, as that covers the custom logic by simply writing your own static Get method in your custom partial class and this covers the type issue :+1:

[zpqrtbnk]

@Korayem (and all) - we are indeed looking into shipping ModelsBuilder with Umbraco (with a config option of some sort to disable it entirely) - I am resuming work on ModelsBuilder now.

The goal is to nicely support compositions.

Must say I'm a bit lost re. the nested content / vorto changes that would be required. Would be great if you could give me more details (concrete code examples, etc).

[zpqrtbnk]

Attention all... can I have one very simple use case for implementing ForcePropertyType ie for forcing the type of an implemented property to something different than what the property value converter for that property would report?

[TomSteer]

Hello...The one i've come across the most is Multiline Textbox, to set it's type to string rather than an object although this could easily be done via a property value converter. A more realistic use case i guess would be where you want the value of a property to be of a custom type but no for every use of that property editor e.g. You have an Archetype and you want to map this to some custom type (view model) that is relevant to that particular property but not to the Archetype property editor in general. Hopefully that makes sense.

[zpqrtbnk]

@Tom: thanks! The first one is "nice to have" but should really be fixed with proper value converter. The second one OTOH makes a lot of sense. Of course you can tweak a property value converter to return different types depending on the property alias, prevalues, whatever but quite probably, in many cases, just overriding the type at property level would be easier.

[jbreuer]

Hi @Korayem. In this blog I'm using GetVortoValue() with the ModelsBuiler too: http://24days.in/umbraco/2015/multilingual-vorto-nested-content/. Can you maybe explain how [ImplementDataType("VortoValue")] would make the example in the blog easier?

[zpqrtbnk]

About Vorto

I realize that whenever a property is a Vorto thing, you cannot just use GetPropertyValue<T>(alias) but have to use GetVortoValue<T>(alias) which means that you have to manually override every Vorto property. @Korayem would that be "the amount of manual work" you mentioned?

In an ideal world, we would not need GetVortoValue because Core would know how to manage multiple cultures - but we don't live in an ideal world and the required Core changes will not happen before v8 because of backward compatibility.

So, assuming that some properties will always need some special getter to replace GetPropertyValue... how can we make it simple? I am not sure yet. There are two issues here: identifying that a special getter is required, and identifying the CLR type of the property. The second one is tricky as it would require the Vorto value converter to be able to tell us what CLR type it will return... eg either an integer or a string or ... just about anything.

It all would need to be definitively simpler but I fail to see how to do it without altering Core in a non backward-compatible way?

About Nested Content

Proper support for nested content require some Core changes, otherwise it's always going to be an incomplete hack, and those changes will not happen before v8 because of backward compatibility. I think there is little I can do for nested content in ModelsBuilder at the moment, unfortunately. Unless someone has bright ideas?

One thing though. Today, if you know that a property is going to return a strongly typed MyNestedContent but the converter declares it returns IPublishedContent (because that's all it knows, really) you have to write:

public partial class MyContent
{
  [ImplementPropertyType("myProperty")]
  public MyNestedContent MyProperty
  {
    get { return this.GetPropertyValue<IPublishedContent>("myProperty") as MyNestedContent; }
  }
}

Some people have requested a way to simply override the type of some properties. I'm thinking about introducing the GeneratePropertyType attribute that would work as follows:

[GeneratePropertyType("myProperty", Type = typeof(MyNestedContent))]
public partial class MyContent
{ }

Not ideal, but better?

[jbreuer]

Hi @zpqrtbnk. Sorry this might be bit offtopic. The following won't work. return this.GetPropertyValue<IPublishedContent>("myProperty") as MyNestedContent; Because the Nested Content IPublishedContent isn't a real node it won't go through the IPublishedContentModelFactory. So you have to do: return new MyNestedContent(this.GetPropertyValue<IPublishedContent>("myProperty"));

The GeneratePropertyType attribute sounds like a good idea. That could make things more simple. Will that also work if you want to override interface properties. Like we discussed here: https://github.com/zpqrtbnk/Zbu.ModelsBuilder/issues/40

[zpqrtbnk]

As for interface properties: I'm currently fixing the builder so that whatever you do to a property on a class is taken in account at interface level (and in every composed type) - so if you change the CLR name, CLR type, override the implementation... it all should work. That is spec'ed already and is work-in-progress.

As for the nested content thing... you are right. I was thinking about an "improved" nested content that would be able to tunnel all content through the factory. Frustrating, but the only way to get nested content right is to make breaking changes in Core.

The current work effort aims at improving the builder with regards to compositions, etc - target being v7.4. I'm open to all suggestions. But re-engineering Core, Vorto & NestedContent is out of scope ;-)

[Korayem]

I was suggesting a new way to override Data Types across ANY Document Type. In my case, we'll be using Vorto in a lot of Document types mixed with Composite Document type and Nested Content which means we'll have to repeat the following code mentioned by @jbreuer in his blog post across several properties across several Documents

public partial class UmbNewsItem
{
    [ImplementPropertyType("news")]
    public Newsdetached News
    {
        get
        {
            var news = this.GetVortoValue<IPublishedContent>("news");

            return news == null ? null : new Newsdetached(news);
        }
    }
}

What I am suggesting is similar to what @zpqrtbnk has suggested in his recent reply above as [GeneratePropertyType("myProperty", Type = typeof(MyNestedContent))] but I was looking forward for a solution to be implemented on assembly level such that any Document using that Data Type, ModelsBuilder will automatically us that implementation. Something like [assembly:ModelsNamespace("MyProject.ContentModels")] but focused on specific Data Type of Type Vorto.

[zpqrtbnk]

@Korayem I understand the idea of specifying how to implement the property per data type however how would we know (1) how to implement the property and (2) the CLR type of the property?

I do not want the ModelsBuilder to embed special code for Vorto, Archetype, etc - so I would need to create a way for ppl to register helpers corresponding to data types, that would

• tell me what's the CLR type of the property
• generate the inner code for the property getter

Generating the inner code should not be too difficult (just write to a StringBuilder) and so Vorto could replace the GetPropertyValue by its own GetVortoValue. However, figuring out the returning type is more difficult. How can we know that in the example, the property is of type Newsdetached, as Vorto itself can return pretty much anything (integer, string, object...) and reports that its converter returns VortoValue object?

I will try to think about such an extension. It may be possible and even be interesting for Archetype. But I am not quite sure how Vorto itself would determine the return type?

[zpqrtbnk]

Thinking: Vorto embeds one data type. We would need to find the property value converter for that data type, and ask the converter to tell us its returned type. Which leads us to NestedContent: the converter would tell us it returns IPublishedContent. NestedContent converter has no way to return the alias of the specific content type it would return. There would be solutions if we made changes to Core but that's out of today's scope. Thoughts?

[jbreuer]

I agree that core changes should not be a part of this discussion. V8 is still far away. In my opinion it's not that bad that we need to implement properties manually if we want to use GetVortoValue. In the examples from the blog I only need to implement GetVortoValue once. On the Newsdetached model I don't need to use GetVortoValue. Because the IPublishedContent is already translated GetPropertyValue can just be used: https://github.com/jbreuer/1-1-multilingual-example/blob/master/Sources/Umbraco.Extensions/Models/Newsdetached.generated.cs.

[hfloyd]

Hi Stephan, I see that you added some additional support for Compositions in the new version for Umbraco 7.4.

Related to my comment (https://github.com/zpqrtbnk/Zbu.ModelsBuilder/issues/72#issuecomment-153078305)... It seems that Doctypes using a customized Composition are not (yet?) automatically getting their matching "Implementation" code.

Example:

/// <summary>Meta Title</summary>
[ImplementPropertyType("MetaTitle")]
public string MetaTitle
{
    get { return CompPageSettings.GetMetaTitle(this); }
}

I say this because (even though I have a fully-customized interface & class in "CompPageSettings.cs") after I 'Run Custom Tool' to regenerate the models, and Rebuild, I am seeing the errors:

'MySite.Web.Models.TextPage' does not implement interface member 'MySite.Web.Models.ICompPageSettings.MetaTitle'

'MySite.Web.Models.Homepage' does not implement interface member 'MySite.Web.Models.ICompPageSettings.MetaTitle'    

Is there a plan to add that? Or should I assume that I will need to continue to add the "custom" implementation for each property which calls "{Comp}.Get{MyProperty}(this)"?

I saw there is a new web.config option:

<add key="Umbraco.ModelsBuilder.StaticMixinGetterPattern" value="Get{0}" />

Which I thought would be for this purpose... But perhaps I am misunderstanding?

[zpqrtbnk]

I need to verify. The 3.0.x versions of ModelsBuilder do not change the generation rules (from 2.x) at all, so if there was some issues, they are still there. I have some work-in-progress to refactor & cleanup the generation rules so that compositions are fully & better supported.

[hfloyd]

Okay, so I will continue adding those calls into my custom models, and see what happens int he enxt version :-) Thanks!

[kipusoep]

Just started using Vorto with Models Builder and ran into the issue that properties are of type VortoValue. Is there an easier way to use both without having to implement every property yourself?

[zpbd]

We are also looking at using Vorto with ModelsBuilder. I'm guessing that Vorto is a prime example of where we need to be able to customise the generation rules rather than just return types. Has the thinking moved on much? Are we still looking at just manually implementing properties or is there any other more sensible way to override GetPropertyValue with GetVortoValue?

[zpqrtbnk]

/me is triaging issues

This issue... is quite old and touching tons of different topics, and I don't know what to do of it, so I am going to close it. If there is any particular topic that you do not want to see being closed, please create a new issue. Thanks!