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.

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

Generate code using language fallback #236

Open
wants to merge 1 commit into
base: v8/dev
Choose a base branch
from

Conversation

jvtroyen
Copy link

  • Added appSetting to enable generation of language fallback
  • Added property on PropertyModel to indicate Variant property
  • Added fallback parameter to this.Value<>() for Variant properties, when appSetting is true

@jvtroyen
Copy link
Author

This is a PR for #235, any way to link them correctly?

@jvtroyen jvtroyen changed the title #235 Generate code using language fallback Generate code using language fallback Apr 16, 2020
@ronaldbarendse
Copy link

Isn't this already implemented in #206 and available in the latest source?

@jvtroyen
Copy link
Author

Hi @ronaldbarendse ,
I tried reading up on #206 and related issues/changes. They relate to v4, I worked on v8, can you explain the difference between the v8 and v4 branch? (Just so I work with the correct information)
@Shazwazza also commented (on #235, as I messed up the PR)

Back on topic. As a developer, I want to use Model.Title so that I get the title in the current culture, or, if empty, the fallback culture (or chain).
The extension-methods discussed mention all kind of possibilities, but fallback would have to be manual, as in Model.Title ?? Model.Title("en-us"), implying knowledge about the model, it's properties and the fallback chain.

The view-designer (who is a separate front-end developer, not the back-end doctype-designer) would have to chain fallback for each property in his views.
Alternatively, the backend developer would have to override each of hundreds properties, simply to include fallback.
By optionally (driven by a setting or attribute) generating the additional fallback-attribute, all this is avoided and everyone can use Model.Title knowing they will fallback if it's Variant, but they would not have to be aware.

One final example. What if the fallback chain was

  • pt -> es -> en
  • fr -> en

If I understand correctly the argumentation for extension methods:

  • to get the Pt title (+ fallback), you would use model.Title ?? model.Title("es-es") ?? Model.Title("en-us")
  • to get the Fr value, you would use model.Title ?? Model.Title("en-us")

In the view, that means you need to code depending in the given culture, whereas the fallback-parameter should simply be allowed to do it's work and present the use of Model.Title with fallback baked in.

@ronaldbarendse
Copy link

@jvtroyen As per #206 (comment), you should get the title within the current culture or using the language fallback (as configured on the languages themselves) with Model.Title(fallback: Fallback.ToLanguage).

If you want a custom language fallback (not using the fallback chain configured on the languages), you should use a default value, e.g. Model.Title(fallback: Fallback.ToDefaultValue, defaultValue: Model.Title("en-US")).

IMHO ModelsBuilder should keep its API as close to the Umbraco core Value extension methods as possible and also try to keep configuration settings to a minimum, so I would recommend closing this PR (even though it has its merits).

@jvtroyen
Copy link
Author

I see your point about the usefulness of these extension-methods, but I'd still counter by stating that the people designing the views (not developers, but they manage) would need to use Model.Title(fallback: Fallback.ToLanguage) instead of Model.Title, for EVERY property in EVERY view, instead of relying on the model to do this for you. It's just asking to be forgotten.

On the other hand : I've read many threads and discussions about these extension-methods and version 4. I saw the different branches in the code, but what version on nuget contains these?
We're using 8.1 (https://www.nuget.org/packages/Umbraco.ModelsBuilder/). Does it have a new name?

@ronaldbarendse
Copy link

The new extension methods in PR #206 aren't released yet, but will probably be included in the next Our.Umbraco.ModelsBuilder release.

You currently don't have to add Fallback.ToLanguage in every view if you implement the property yourself in a partial class with this overload (like your own example in the linked issue #235).

If ModelsBuilder starts generating default fallbacks, it should be generic (and also include Fallback.ToAncestors, possibly Fallback.ToDefaultValue with specified value) and configurable per property (e.g. if you don't want a default fallback for a specific property). This might be implemented using an attribute, similair to RenamePropertyType.

E.g. a DefaultFallbackPropertyType attribute might be added as assembly attribute to change the default fallback of all properties, but also specified/overridden on the MB class for that content/document type or even a specific property alias.

@jvtroyen
Copy link
Author

In the meantime, I'm more up to speed with what v4 is.

The things I want to avoid, are:

  • view-editors needing to be aware of fallback or not
  • implementing partial classes for every doctype just to enable fallback.

I like what you're proposing, though. An assembly attribute to pick the default fallback. I'll give implementing it a try, both in v8 and v4.

@ronaldbarendse
Copy link

@jvtroyen There's another approach: implement your own IPublishedValueFallback (have a look at the default PublishedValueFallback) and register that in a composer using composition.RegisterUnique<IPublishedValueFallback, CustomPublishedValueFallback>();.

@jvtroyen
Copy link
Author

That is an excellent tip. Trying it out right now.

I guess we can close this as we are no longer in the realm of ModelsBuilder.

@ronaldbarendse
Copy link

@jvtroyen As per discussion above: this can be implemented by implementing/overriding the property in the generated model or change the registered IPublishedValueFallback. Implementing this the 'right' way would be to use attributes, either assembly, class or property attributes, just like the existing ones: https://github.com/modelsbuilder/ModelsBuilder.Original/wiki/Control-Generation/. So I think this PR can be closed indeed 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants