U4-11227 - Build the IPublishedContent and UmbracoHelper APIs to support variants

Created by Shannon Deminick 16 Apr 2018, 10:23:23 Updated by Shannon Deminick 09 May 2018, 05:43:14

Subtask of: U4-11217

Based on other dicussions, we need to build these APIs. Some may be extension methods, we should keep IPublishedContent as simple as possible.

NOTE: We are not building the "fallback provider" in this task, the fallback provider implementation and design will be part of a future task.

  • How to get a property value for a specific language?
  • How to get a property value for a default language? Or do we already know the default language based on something in the request?
  • How to get a list of available language variants for the current "Site" node? Do we first get Site() then somehow list it's language variants or can i just get these language variants from the current node?
  • We will need to get the URLs for each of the Sites variants too, UmbracoHelper can have a method to get all URLs for a node and be able to get URLs for a node/culture combination

1 Attachments

Comments

Stephan 18 Apr 2018, 17:28:40

Notes: I have a draft implementation of "fallback" and "current language".

''Current, default language''

There's an IPublishedVariationContextAccessor of some sort, which provides access to the current PublishedVariationContext, which contains the "current" language and segment. Typically, it would be retrieved from HttpContext. And populated in the pipeline once we know which content item we're rendering, hence which language we're in. It would be injected in IPublishedElement instances, and used when getting values with no specific language. This may require that we slightly refactor getting values, but that should not be too complicated. All in all, not too complicated.

''Available languages''

That should already be available through the domains, we might just need to expose it properly.

''Urls''

Goes with routing. --Not sure I understand "''so the API should/could be able to return a collection of IPublishedContent items for the site per variant to get the URL''". Will need to discuss it.--

''Fallback''

There's an IPublishedValueFallback interface that is also injected into IPublishedElement instances and is invoked whenever we want a value for a specified language and segment, and get nothing. It has access to the entire published element, and can do whatever it pleases, and return something, or nothing. Not too complicated.


Shannon Deminick 24 Apr 2018, 03:17:16

Notes:

Name

When using the API for IPublishedContent, a user shouldn't need to know about the current culture when writing their template. If they want to output Model.Name it should be the contextual name - so either invariant if the doc type is invariant, or culture specific based on the matched culture in the request

Current Culture

I'm unsure if we really need something like IPublishedVariationContextAccessor. We can just use the combination of: IPublishedElement.ContentType.Variations and CultureInfo.CurrentCulture (which is the same as Thread.CurrentThread.CurrentCulture)


Stephan 25 Apr 2018, 12:08:01

have done a few changes to the POC late last night, works kinda well


Stephan 30 Apr 2018, 19:32:28

Code is in branch temp8-U4-11227, not a PR yet but getting there

So, on the front-end side, for culture & segment:

  • null means "the current one"
  • string.Empty means invariant / neutral
  • non-empty string is an actual value

Tests are all green, so I ''think'' that model.Value("alias") should now return the proper "current culture" value on the front end - but I still need to test.

And we need to align services with it, so that on the back-end, it's also string.Empty for invariant (and not null anymore) - in progress.


Stephan 02 May 2018, 15:57:26

Riiight... so it's a PR now. Requires a general code review. And tests: basically, model.Value("alias") should return the value for the "current" culture, which should be the Thread.CurrentUICulture as we register (in WebRuntimeComponent) a ThreadCultureVariationContextAccessor. And model.Value("alias", "fr-FR") should get you the french value (if any).

Then, model.GetCulture("fr-FR").Name will give you the french name, assuming french is published / available. And model.Cultures would list you the published / available cultures. This is also how we retrieve the url segment, and the published date, per culture.

I ''think'' it makes sense, but probably you'll unearth some oddities.


Stephan 02 May 2018, 16:00:57

PR https://github.com/umbraco/Umbraco-CMS/pull/2611


Shannon Deminick 08 May 2018, 02:13:04

The amount of code changes here is pretty hard to fully review. I can build the solution but there's problems with saving and publishing content in the back office with YSODs, but i have no idea if I've already fixed this in my branch. There's a lot of merging i'll need to do between this and my branch too.

For now I'm going to merge this into my branch. I don't want to merge this to temp8 since im getting ysods with most things in the back office on this branch.


Shannon Deminick 08 May 2018, 04:49:54

The property value is not returned contextually and still requires you to pass in a culture to get the correct property value. I can see IPublishedContent.Name and IPublishedContent.UrlSegment are contextual but properties are not. The notes above say this should work but it's not implemented and I can't see any code in there that would make this work.

I can see that if property.HasValue returns null it goes into the PublishedValueFallback.GetValue but that is a different case.

Just like IPublishedContent.Name uses VariationContextAccessor.VariationContext.Culture, so does the implementation of IPublishedProperty. However, i can see that the fallback is done in the extensions for the Value extensions, so i'll lave IPublishedProperty alone and implement this in the extensions.


Shannon Deminick 08 May 2018, 05:25:12

OK ... I've finally found ContextualizeVariation in the Property implementation. I'll have a look there. Currently i cannot get property values to return at all so something odd is going on.


Shannon Deminick 08 May 2018, 05:44:11

I think i found the problem which is partly to do with CompositeStringStringKey see screenshot


Shannon Deminick 08 May 2018, 06:08:33

Heading off for the day, some things need to be fixed:

*Please read all my comments on the PR itself *We will not merge this PR to temp8 - I've merged this into my branch U4-11282 and once all testing is completed there we can merge it all to temp8 *I've fixed up various things **no AdditionalData, the properties are on IDocumentEntitySlim **Fixed the dictionary in PublishedContent.cs to be InvariantCultureIgnoreCase **Added a hack to CompositeStringStringKey.cs, you'll see a fixme there but it's needed else you cannot resolve any property value for cultures because the keys don't match (see above) **Removes exception throwing on PropertyData.cs because if you throw, then nothing works including any deserialization and also since we are actively setting null values to these properties *Fixes need to be applied to HasValue since the currently implementation always fails and therefore you can never get the contextual value of a property using the Model.Value approach, though with the above fixes property.GetValue() will work

It seems that not much of this was tested (or maybe it just couldn't be tested?) since resolving any culture variant property value would definitely not work.

If this can be fixed today, i'll continue testing my branch tomorrow, fix any unit tests and put it in review.


Stephan 08 May 2018, 07:08:17

Going to work in U4-11282 branch from now on, considering the two branches merged, it makes no sense to maintain the two branches separate at that point. Especially as fully testing one requires, as you've noted, the other.

Dealing w/ fixes - eg we cannot "hack" CompositeStringStringKey, this is a very low level class that should not do magic. Going through all the fixes, etc. today.


Stephan 08 May 2018, 07:40:00

Have commented on comments on the PR. Fixing a few of them, points remaining open that I need to work on:

  • PropertyData culture should never be null - if something sets it to null, we need to fix the something, or turn nulls into string.Empty, or something, but we cannot leave it like it is now
  • General issue with how we use GetCulture(culture).UrlSegment and ignore the fact that a content item may very well not vary, in which case GetCulture(culture) is null


Shannon Deminick 09 May 2018, 05:43:00

Awesome, i got a lot further today with these changes. I'm going to close this task, since things are 'working', there is of course some remaining issues but we're fixing them as part of U4-11282


Priority: Task - Pri 1

Type: Task

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 8.0.0

Sprint: Sprint 84

Story Points: 5

Cycle: