Generate extension methods for multi-lingual properties

[zpqrtbnk]

When a property is multi-lingual we should generate extension methods. Property:

public string Title 
  => this.Value<string>("title");

Extension method:

public string Title(this Foo model, string culture = null) 
  => model.Value<string>("title", culture);

(see discussion on umbraco/Umbraco-CMS#5170)

[ronaldbarendse]

Any reason why this should be extension methods and not just a regular methods in the model itself? It would need to generate an additional static class to hold the extension methods, probably in the same namespace (otherwise you wouldn't see the extension methods without adding the new namespace to your code), in the same or maybe even a new file... Generating this would be more complicated than just adding the method while iterating the properties.

And to cite the Microsoft documentation:

In general, we recommend that you implement extension methods sparingly and only when you have to.

Just like properties, make sure some method names are validated and excluded (e.g. Equals, GetHashCode, GetType, MemberwiseClone, ReferenceEquals and ToString from object and from Umbraco Value, GetModelContentType, Unwrap, GetProperty and many other methods). This is especially important if MB would generate extension methods, as an extension method with the same name and signature as an interface or class method will never be called (see linked documentation).

[Shazwazza]

It's the same reason for the changes of IPublishedContent umbraco/Umbraco-CMS#5170 (comment)

Initially we wanted to make everything methods but we were convinced that we shouldn't and instead support both properties and methods - which is only possible with extension methods since they are the same name.

Then this update is for MB to be consistent with the whole approach, you can access by property or by method with the same name, but if you are using variants, you'd probably want to use the method since it will give you the ability to request things from another culture.

[zpqrtbnk]

What Shannon says - in an ideal world, we'd just kill the .Title property and replace it with a .Title(string culture = null) method - but then this is breaking tons of sites - so the property + extension method provides some sort of in-between solution. It's far from perfect and probably violates a few guidelines, but that's a transition.

Generating these methods is not that complicated.

And then we'll work on a complete overhaul of MB to clean things up.

[deMD]

Is there any ETA for this feature to be released?

[zpqrtbnk]

Not yet. The Models Builder project is going to evolve (see the Umbraco 8.1 Release blog post) and a roadmap for the Community Models Builder is currently being put together. My goal is to publish something within the coming 1-3 weeks. And to have a release as soon as possible, including this feature. Stay tuned!

[zpqrtbnk]

Work in progress. Request for Comment:

When a property is variant, we generate an extension class:

public static class DocumentExtensions
{
  public static string Title(this Document that, string culture = null, string segment = null)
    => that.Value<string>("title", culture, segment);
}

We also generate the property, in the Document model class, as a simple proxy to the method:

[ImplementPropertyType("title")]
public string Title => this.Title();

This way, should you want to override the property implementation... all we have to do is provide our own extension in a class along models:

public static partial class DocumentExtensions
{
  public static string Title(this Document that, string culture = null, string segment = null)
    => that.Value<string>("title", culture, segment).ToUpperInvariant();
}

The override can deal with all variations in one place. Models Builder detects the extension automatically, and does not generate it.

If a property implementation is provided, then Models Builder does not generate the property (i.e. the Title thing) since it exists already, as has always been the case before. But it generates the extension method (i.e. the Title(culture, segment) thing) anyways. There is no way, currently, to prevent the extension methods from being generated.

Thoughts?

[ronaldbarendse]

The generated extension class will probably also be marked as partial, I guess? It's not necessary (as you can add extensions methods to any static class), but would allow adding your custom methods to the same class, not cluttering your project.

If the property alias is ignored (using the IgnorePropertyType attribute), it shouldn't generate the extension method, so that way it's possible to add your own. As that would also ignore generating the property, you need to manually add that back (as is already the case).

And maybe a setting could be introduced to remove generating variant properties, so only the extension methods are generated?

[zpqrtbnk]

1. Yes, generated as partial class, my bad - mistake in previous post.

2. Make sense that if the property is entirely ignored... we should not generate extension methods either - think it's already the case, going to check.

3. So, setting to just have the .Title(culture) extension and not the .Title property? But then... if we don't want the .Title property we can have the .Title(culture) method directly, no need for extensions?

But extensions allows me to define methods for an interface instead of duplicating them in classes. Convenient for composition, as it means less code is generated. Mmmm... interesting - thinking.

[zpqrtbnk]

1. tested, generated as partial
2. tested, fully ignored

as for 3... would you go full-methods for everything, or only for variant properties? doing it only for some properties is going to be confusing IMO, what do you think? we're keeping properties to reduce the impact of the breaking change, but we could have a setting to indicate that you don't care, and please go with methods everywhere?

[zpqrtbnk]

Going for a minimum viable product without point 3. If point 3 is important we'll create an issue for it.

[skttl]

I think it would be valuable to have an extension for using fallbacks.

So you could go .Title(Fallback.ToAncestors) or .Title(Fallback.ToDefaultValue, "My default title")

[zpqrtbnk]

Making sense. Currently, we generate:

public string Title(string culture = null, string segment = null) => ...

Would it make sense to align with the full Value overloads, ie

public string Title(string culture = null, string segment = null, Fallback fallback, string defaultValue) => ...

So that any other kind of fallback would also be possible?

?

[skttl]

Yes, that was what I would expect from this functionality :)

[ronaldbarendse]

@skttl So that would mean the extension methods should also be generated for invariant properties, only without the culture parameter? Makes total sense!

And will segments have to be enabled on a property? If so, it would also make sense to remove that parameter if it's not, right? So you would only generate overloads with acceptable parameters for that property...

So that would mean the following extension methods would be generated for an Umbraco.Textbox property title:

Culture variant	Segmented	Extension method
❌	❌	Title(Fallback fallback = default, string defaultValue = default)
✔️	❌	Title(string culture = null, Fallback fallback = default, string defaultValue = default)
❌	✔️	Title(string segment = null, Fallback fallback = default, string defaultValue = default)
✔️	✔️	Title(string culture = null, string segment = null, Fallback fallback = default, string defaultValue = default)

Changing a culture invariant segmented property to culture variant segmented would mean the overloads would change (causing the segment to be interpreted as culture), but such a change would probably mean the views must be rewritten anyway. And if you're really concerned about this, you should only use named parameters.

@zpqrtbnk Regarding not generating the properties: how will this work with compositions/interfaces? Would that mean the compositions become decorator interfaces?

[zpqrtbnk]

@ronaldbarendse wow - makes total sense of course - no point generating the segment parameter if we don't vary by segment - and yes, we could generate methods even for non-varying properties.

and then, having a setting to just plainly disable properties generation suddenly makes more sense.

as for compositions/interfaces: the beauty of methods is they can be extension methods, so they can totally be defined on interfaces, and apply to everything implementing that interface (composition). and yet, if you do implement a method on a class, it will take over.

back to work, will report

[zpqrtbnk]

implemented - generate all the nice methods, all as extension methods, according to the table above

oh, and then you can fully disable the property getters generation with

[assembly:ModelsBuilderConfigure(GeneratePropertyGetters = false)]

looks great

[LennardF1989]

@zpqrtbnk I added support for this to the v8/dev branch on my fork. Is that something you would merge or not? If so, I will formalize it and make a PR.

Also contains code to add aliases of properties as constants. Still contemplating if the root of the class is a good choice, or if I should make a nested Constants-class.

[ronaldbarendse]

@zpqrtbnk I added support for this to the v8/dev branch on my fork. Is that something you would merge or not? If so, I will formalize it and make a PR.

@LennardF1989 These changes are already implemented/committed into the default v4/dev branch, which should actually get merged entirely into v8/dev, as version 4 was released as version 8 (to align with the Umbraco CMS version, for whatever reason) and keeping this branch makes no sense anymore. After this is done, v8/dev can also be made the default branch (and a v3/dev branch should probably also be created for bugfixing MB for Umbraco 7).

Also contains code to add aliases of properties as constants. Still contemplating if the root of the class is a good choice, or if I should make a nested Constants-class.

This too is already implemented in v4/dev and discussed in #215.

[ronaldbarendse]

Merging v4/dev into v8/dev will require quite some conflict resolving, especially after all the renames. I also can't wrap my head around the revisions (branches/tags), as version 8.0.4 was already released after it diverged into v4/dev:

[ronaldbarendse]

Also, why isn't this repository maintained within https://github.com/umbraco-community as Umbraco.ModelsBuilder? Would make sense to have such a valuable package more closely managed by HQ and community members, without directly putting it in HQs control and support...

[jomehmet]

Hi, @ronaldbarendse could you please point me to how I install a version of this package that supports language variants?
Currently I've installed 8.1.6 from nuget and don't see anything about variants in the generated models.

[ronaldbarendse]

@jomehmet As discussed above, this feature isn't released yet, but only merged to the v4/dev branch (the default branch, while the current releases use the v8/dev branch)...

Maybe @Shazwazza or @nul800sebastiaan can give more info on when this will get merged to v8/dev and released?

[jomehmet]

Thanks for your answer @ronaldbarendse . I got what I needed from Umbraco Cloud Customer service. I didn't see how to use variants with ModelsBuilder but this article helped me: https://our.umbraco.com/documentation/Reference/Language-Variation/