Umbraco-cms: Refurbish IPublishedContent (was: not created nodes are returned by children property)

Created on 5 Apr 2019  路  33Comments  路  Source: umbraco/Umbraco-CMS

If you add a language to a site with childnodes the childnodes are visible as "not created" nodes.
If you add content to the rootnode and publish it and do @Model.Children in a template you get all the childnodes including the "not created" children. If you do @Model.Children() you only get the published children.

Reproduction

Create a documenttype and create a rootnode with 4 children.
Add a new language
Publish the rootnode of the new language and only one of the children

In the rootnode template paste the following code:

<html xmlns="http://www.w3.org/1999/xhtml">
<head>
    <title>test template</title>
</head>
<body>
    @{
        IEnumerable<IPublishedContent> brokenChildren = Model.Children;
        IEnumerable<IPublishedContent> normalChildren = Model.Children();
    }
    <h4>Children property</h4>
    <ul>
        @foreach (IPublishedContent child in brokenChildren)
        {
            <li>childname = @child.Name</li>
        }
    </ul>

    <h4>Children method</h4>
    <ul>
        @foreach (IPublishedContent child in normalChildren)
        {
            <li>childname = @child.Name</li>
        }
    </ul>
</body>
</html>

The first list will return 4 list items where only one has a actual name.
The second list will return only the one published node.

I expect the results to be the same as in Umbraco v7

categorbreaking releas8.1.0 typbug
Source
picture FransdeJong

Most helpful comment

PR is #5312, up for testing and reviews and comments - we want this to be consistent!

_Contains Breaking Changes!_

Two new interfaces have been introduced: IPublishedContentType and IPublishedPropertyType, and IPublishedElement and IPublishedContent use them exclusively (vs PublishedContentType and PublishedPropertyType), thus making them easier to mock and test.

The following IPublishedContent members change:

  • Name: content.Name is gone, content.GetCulture(...).Name is gone, the proper way to get a name is content.Name(string culture = null)
  • UrlSegment: content.UrlSegment is gone, content.GetCulture(...).UrlSegment is gone, the proper way to get a Url segment is content.UrlSegment(string culture = null)
  • ItemType: content.ItemType is gone, the proper way to get the item type is content.ContentType.ItemType
  • Culture: content.Cultures now returns a collection of string representing the available cultures, and content.GetCulture(...) is gone, the proper way to get the culture date is content.CultureDate(string culture = null)
  • Parent: content.Parent is gone, the proper way to get the parent is content.Parent()
  • Children: content.Children is gone, the proper way to get the children is content.Children(string culture = null) and it always filters children according to culture
  • Url: content.Url, .GetUrl(...), .Url(), .UrlAbsolute() are all gone, the proper way to get the Url is content.Url(string culture = null, UrlMode mode = UrlMode.Auto) which produces a relative-or-absolute Url depending on what's "best", but can be forced to produce absolute Urls with UrlMode.Absolute

Finally, v7 had content.GetCulture() that was deriving a culture from domains. Somehow that was lost when working on v8 (issue #5269). It has been re-instated as content.GetCultureFromDomains().

_Remaining Work_

While we are breaking IPublishedContent we should deal with #3667 (move Children() and Parent() to extension methods - not sure we really want this)

Then, need to cleanup Url getters on UmbracoContext

picture zpqrtbnk on 23 Apr 2019
👍3

All 33 comments

That does sound odd indeed but will need to check with @zpqrtbnk to see if this is a design decision or not?

Shazwazza picture Shazwazza on 8 Apr 2019

So...

  • model.Children returns every child which is published (ie published for at least one culture)
  • model.Children() is defined as Children(string culture = null) and returns model.Children.WhereIsInvariantOrHasCulture(culture)

What this means is that... if you have a root node which is published for L1 and L2, and has 4 children published for L1, and only 1 of these children is published for L2... and you visit the page in the context of L2... Children is going to return all 4 children, even though 3 of them are published, but not available for the L2 culture - whereas Children() is going to expand as "children having the current culture" and therefore is going to return only 1 child.

So I'd say it work as designed, but that's unfortunate. We should fix Children to act as Children() - have to be careful though, cannot "just" to it = valid issue, needs to be fixed.

zpqrtbnk picture zpqrtbnk on 8 Apr 2019

Well, that's annoying. model.Children(string culture = null) accepts a culture... and model.Children cannot, so it would do the same as model.Children() ie model.Children(null). But then we don't have a way to retrieve all published children, regardless of the culture.

Ideally, I would love to rename model.Children into model.AllChildren and force people to use model.Children() to get what they expect.

But... that's breaking, isn't it?

zpqrtbnk picture zpqrtbnk on 8 Apr 2019

Oh my... I want...

  • model.Name(string culture = null) and kill model.Name property (which, at the moment, returns the name for the current culture)
  • model.Url(string culture = null) and kill model.Url property (which, at the moment, returns the url for the current culture)
  • model.Children(string culture = null) and kill model.Children property (which, at the moment returns all published children, regardless of whether they are available in the current culture)

And I probably don't want

  • model.GetUrl(string culture = null) because we have model.Value(...) which does not have a Get prefix and we should be consistent (have a prefix, or not have a prefix)

but all these changes are breaking

zpqrtbnk picture zpqrtbnk on 8 Apr 2019

Is it 'bad' that the current model.Name and model.Url return the values for the current culture? I think that is an expected behaviour isn't it?

I agree with the naming of the property .Children and in order for the .Children() method to work, the IPublishedContent instance must have access to "All" Children so that will still have to be part of the interface one way or another.

If only we had the time to implement #3667 before release, we then wouldn't have this issue.

I don't think just obsoleting the .Children property is going to suffice in this case because people will continue to use it by accident and it will return unexpected results. If we can't implement #3667 somehow alongside our current implementation without destroying everything, then I think we will need to change .Children. to .AllChildren and document this breaking change for 8.x (potentially 8.1)

Shazwazza picture Shazwazza on 9 Apr 2019

It is 'bad' that the "current culture" name is model.Name but "another culture" name is model.GetCulture(culture).Name - already seeing questions and "what were you thinking" about this. One unique model.Name(culture) would be simpler.

zpqrtbnk picture zpqrtbnk on 9 Apr 2019

And don't mention the .Url, .GetUrl etc situation

zpqrtbnk picture zpqrtbnk on 9 Apr 2019

If the properties give me weird results I would rather not have them at all. It confused my colleague who is starting with Umbraco and it also took me a while to find out there suddenly is a difference between the property and the method.

In V7 it was a logical choice to get the default set ready because if I get the IPublishedContent for a node I was for shure going to work with that "variant".

In V8 with the variants the default set doesn't make that much sense anymore because I could easily want to get information of a different culture for instance. So with that in mind it seems logic to only keep the non variant properties (like Id)on the IPublishedContent and put the rest in methods.

If you don't include this change in the next version I'm afraid it's going to hunt you throughout V8?

FransdeJong picture FransdeJong on 9 Apr 2019

If you don't include this change in the next version I'm afraid it's going to hunt you throughout V8?

Exactly - but then the required changes are breaking changes, breaking the very exposed IPublishedContent interface - don't think we have a choice really, and I do want them done, but it leads us to... what breaking changes are acceptable in 8.1, when, how should we communicate about them, etc.

zpqrtbnk picture zpqrtbnk on 9 Apr 2019

If I look back at V7.6, V7.6.3 and specifically V7.0.0 to 7.0.1 it's not a problem to add a breaking change if you document the solution in the version specific update guide.

FransdeJong picture FransdeJong on 9 Apr 2019

All makes sense and we have a meeting about this tomorrow. I'm all for making things consistent so will push for changing this correctly.

I'm just going to list some ugly options here though for reference:

  • Create a new AllChildren property (we'll need it anyways) - adding a property to this interface is not very breaking since 99% of people won't have implemented IPublishedContent
  • Obsolete and hide ([EditorBrowsable(false)]) the Children property
  • Obsolete and hide the Name property
  • Obsolete and hide the Url property
  • Add the new methods that we want - again adding to this interface is not very breaking currenlty

I understand that people might still use the obsoleted props by accident and this will result in confusion and incorrect data displayed and this will definitely haunt us for the remainder of the 8.x series.

Else ... We could make another IPublishedContent2 with the better implementation but this would have drastic consequences on the amount of code and config hacks that would be required to support both IPublishedContent (that we want to remove) and IPublishedContent2 including that we'd need to have new view classes like UmbracoViewPage2, etc... This will end up incredibly ugly!

Shazwazza picture Shazwazza on 10 Apr 2019
👍1

I would certainly pick the first option. Add to that good documentation and a talk about choices at codegarden or a small section in the keynote about continuous improvements and I think you'r all set. The community can handle the questions on our and I think you'll never hear about it after a year ;-)

By the way, I almost feel sorry I brought this up :-)

FransdeJong picture FransdeJong on 10 Apr 2019

I am in favor of breaking the interface once and for all and for good in 8.1 - and then there are other questions and changes that have been mentioned to me that we'd need to do as well.

zpqrtbnk picture zpqrtbnk on 10 Apr 2019
👍2

And now... something that has been in my mind for a while... models (such as those created by ModelsBuilder) implement IPublishedContent and define C# properties corresponding to Umbraco properties.

This can cause collisions. Imagine creating an Umbraco property with alias "Parent" - which should become C# property Parent - and will not build, because IPublishedContent already has a built-in Parent property.

The more we add to IPublishedContent... the worse it becomes.

Long time ago, we discussed moving every built-in things to a container C# property - so IPublishedContent would have only 1 single property, say UmbracoThings, and one would have to do content.UmbracoThings.Parent. Not exactly pretty and not exactly the right way but... thoughts?

zpqrtbnk picture zpqrtbnk on 10 Apr 2019

Well, really we should be doing this https://github.com/umbraco/Umbraco-CMS/issues/3667 , but i think that's is v9 sort of breaking at this stage.

I'm not sure about the container thing, that means that IPublishedContent is just a shell, for those not in MB land, this is super ugly, and what about methods? They would collide too

Shazwazza picture Shazwazza on 10 Apr 2019

Renaming the issue and targeting 8.1 - let this be a larger discussion about how to properly refurbish IPublishedContent - and identify sub-tasks.

zpqrtbnk picture zpqrtbnk on 10 Apr 2019

Proposal for IPublishedContent changes, step 1, work-in-progress.

  • Anything - is removed
  • Anything + is added
  • Anything = is unchanged

~~~~

  • .Name
  • .GetCulture(...).Name
  • .Name(string culture = null)
  • .UrlSegment
  • .UrlSegment(string culture = null)
  • .GetCulture(...).UrlSegment

  • .GetUrlSegment(string culture = null) # extension

  • .Url

  • .GetUrl(string culture = null)
  • .Url() # extension
  • .UrlAbsolute() # extension

  • .Url(string culture = null, ...)

  • .ItemType
    = .ContentType.ItemType

  • .Parent

  • .Parent() # unless we move to ?

  • .Children
    = .Children(string culture = null) # unless we move navigation to ?

  • .AllChildren # unless we move this to snapshot? not sure yet.

  • .Cultures # returning PublishedCultureInfo*

  • .Cultures # returning string*

  • .GetCulture(...).Date

  • .CultureDate(string culture = null)

= .HasCulture(string culture = null)

  • .GetCulture(string culture = null) # returning PublishedCultureInfo
    ? .GetCulture() # was returning CultureInfo in v7
    ~~~~
zpqrtbnk picture zpqrtbnk on 16 Apr 2019

The proposed changes make sense, although I foresee a lot of people having issues with removing .Name despite the reasoning. But again, it makes sense.

If .UrlAbsolute() is removed, please make sure that .Url(string culture = null, ...) provides a way to retrieve absolute URLs for the current culture. They're necessary for various micro data and meta data.

kjac picture kjac on 21 Apr 2019

The .Name change is sure going to be a little annoying... but maybe for the best?

Was thinking .Url(string culture = null, UrlMode mode = UrlMode.Auto) but UrlMode can also be Relative or Absolute, would that make sense? And ppl can always create an .UrlAbsolute() ext method if they really want it

zpqrtbnk picture zpqrtbnk on 21 Apr 2019

Makes sense indeed 馃憤 and yes it'll be for the best. Hopefully people will see it 馃槅

kjac picture kjac on 21 Apr 2019

PR is #5312, up for testing and reviews and comments - we want this to be consistent!

_Contains Breaking Changes!_

Two new interfaces have been introduced: IPublishedContentType and IPublishedPropertyType, and IPublishedElement and IPublishedContent use them exclusively (vs PublishedContentType and PublishedPropertyType), thus making them easier to mock and test.

The following IPublishedContent members change:

  • Name: content.Name is gone, content.GetCulture(...).Name is gone, the proper way to get a name is content.Name(string culture = null)
  • UrlSegment: content.UrlSegment is gone, content.GetCulture(...).UrlSegment is gone, the proper way to get a Url segment is content.UrlSegment(string culture = null)
  • ItemType: content.ItemType is gone, the proper way to get the item type is content.ContentType.ItemType
  • Culture: content.Cultures now returns a collection of string representing the available cultures, and content.GetCulture(...) is gone, the proper way to get the culture date is content.CultureDate(string culture = null)
  • Parent: content.Parent is gone, the proper way to get the parent is content.Parent()
  • Children: content.Children is gone, the proper way to get the children is content.Children(string culture = null) and it always filters children according to culture
  • Url: content.Url, .GetUrl(...), .Url(), .UrlAbsolute() are all gone, the proper way to get the Url is content.Url(string culture = null, UrlMode mode = UrlMode.Auto) which produces a relative-or-absolute Url depending on what's "best", but can be forced to produce absolute Urls with UrlMode.Absolute

Finally, v7 had content.GetCulture() that was deriving a culture from domains. Somehow that was lost when working on v8 (issue #5269). It has been re-instated as content.GetCultureFromDomains().

_Remaining Work_

While we are breaking IPublishedContent we should deal with #3667 (move Children() and Parent() to extension methods - not sure we really want this)

Then, need to cleanup Url getters on UmbracoContext

zpqrtbnk picture zpqrtbnk on 23 Apr 2019
👍3

@zpqrtbnk Braking changes are always a bummer, but I think the changes make good sense. I can see the reasons for doing so. Are the plans to ship this for 8.1?

I'll see if I can find some time to help test this 馃槈

abjerner picture abjerner on 23 Apr 2019

Plans is indeed to ship with 8.1.

zpqrtbnk picture zpqrtbnk on 23 Apr 2019
🚀1 🎉1

OK, I think this PR is big enough and makes sense, moving to review.

Reviewers, please do a code review but also think about whether this is all consistent or not!

zpqrtbnk picture zpqrtbnk on 24 Apr 2019

This has been added to our backlog AB#385

Shazwazza picture Shazwazza on 7 May 2019

Not completely sure why removing the properties (Name, UrlSegment, Parent, Children and Url) is needed, as it breaks more than the interface: it breaks knowledge/expectations and a convention used in a lot of examples, documentation, existing code and a lot more.

This also makes getting these values different than the properties generated by Models Builder (e.g. public string Title => this.Value<string>("title");), which return the value in the current culture. And don't forget: not all content is variant by culture (especially if V7 sites can be migrated soon), where this issue doesn't even occur!

And there should be no need to introduce GetCultureFromDomains(): just name it GetCulture(Uri current = null) and return the current culture as string (using IVariationContextAccessor or domains like V7, still allowing to pass a custom URL for when multiple domains are set). Besides, domains are still used if you need to map domains to culture variants, right?

Back to the real issue: Children currently returns all children, even when it doesn't have any published culture variants for the current culture. As this property is used for navigation/traversal of content, for which the content itself can (optionally) vary by culture, I would argue the current implementation is correct and Children(null) should actually also return all children.

Content != structure, so to get the content in the current culture, just use the properties, but when navigating/traversing the structure return all and only filter content if a specific culture is specified!

ronaldbarendse picture ronaldbarendse on 6 Jun 2019
👍1

This is the type of feedback where you think... no! Or... the guy has a point? Oh my. He may be right ;-)

Your comments about properties + aligning with Models Builder do make sense indeed. The changes were really breaking a lot. And so... the PR has been updated:

  • The methods are extension methods, such as document.Name(string culture = null) - and, when culture is null, they return the value for the current culture
  • The properties are still there, and return the value for the current culture (basically, Name returns Name(null)) - same for Url, Parent, Children, UrlSegment...

This way, we greatly reduce the amount of breaking changes, while still providing a consistent and easy to explain experience. Yes, Intellisense will show both a property and a method, but the idea is: it does not matter, they're the same.

As for Children... the idea is that most people assume, probably rightfully, that it is consistent with other properties, and only return those children that are available for the current culture (same as Children(null)), and that no further filtering is required. This was, in fact, the reason for the original issue being reported.

In order to be consistent, Children would be refactored to work like other properties, and return Children(null) i.e. children for the current culture. Still, because getting all children makes sense, a new property is introduced, currently named ChildrenForAllCultures just to be explicit (want to suggest a better name?).

As for GetCultureFromDomains(), this really is a different thing. If one wants to know the "current" culture, there are other, better, ways. That method, as was the case in v7, takes any document and figures out, by walking up the tree, and based upon a Uri and hostnames, what the culture would be. I think we'd like to keep the name, to clearly say it's something different.

zpqrtbnk picture zpqrtbnk on 7 Jun 2019

More: and then of course, it would totally make sense to add a feature to Models Builder, to also generate extension methods... so the model itself still contains a property:

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

but we would also have an extension method:

public string Title(this Foo model, string culture = null) 
  => model.Value<string>("title", culture);
zpqrtbnk picture zpqrtbnk on 7 Jun 2019 
* The properties are still there, and return the value for the current culture (basically, `Name` returns `Name(null)`) - same for `Url`, `Parent`, `Children`, `UrlSegment`...

Removing them in a future release is imo better as this way you still get polution with mutliple functions that have the same result. I would suggest adding the Obsolete Attribute to the properties so people will be informed that there is a new (better) option

deMD picture deMD on 3 Jul 2019 
* The properties are still there, and return the value for the current culture (basically, `Name` returns `Name(null)`) - same for `Url`, `Parent`, `Children`, `UrlSegment`...

Removing them in a future release is imo better as this way you still get polution with mutliple functions that have the same result. I would suggest adding the Obsolete Attribute to the properties so people will be informed that there is a new (better) option

Let me re-iterate: not all content/properties are culture variant! And most of the time, you just need the value in the current culture, so properties make a lot of sense (and saves you some keystrokes) :wink:

And IPublishedContent just has the Value([culture]) (extension) method, the properties are generated by ModelsBuilder (as that's the entire feature of this package)... Adding (extension) methods for every property that is culture variant would allow you to have a type-safe way to get the value in another culture (instead of falling back to the Value() method).

ronaldbarendse picture ronaldbarendse on 3 Jul 2019

Sure for now I understand keeping them for backwards compatibility, but for a future release removing them makes sense, especially if you flag them with the obsolete attribute now so that people that create new project can get used to the new way. And with future release it might as well be v9.

And as far as keystrokes go, the difference between model.Name and model.Name() is so small, and most IDE's autocomplete it anyways. And to add to that, the compiler compiles properties to be methods anyways, so writing it as a methods as opposed to a property doesn't cause performance hits or anything.

deMD picture deMD on 3 Jul 2019

Removing the properties doesn't make sense for non culture variant content and auto-complete in Visual Studio doesn't add the parentheses at the end by default (see https://stackoverflow.com/questions/21466150/autocomplete-method-brackets).

Anyway, this is more related to Models Builder than Umbraco, as that package generates the classes and properties 馃憤

ronaldbarendse picture ronaldbarendse on 3 Jul 2019

PR is #5312, up for testing and reviews and comments - we want this to be consistent!

_Contains Breaking Changes!_

  • Culture: content.Cultures now returns a collection of string representing the available cultures, and content.GetCulture(...) is gone, the proper way to get the culture date is content.CultureDate(string culture = null)

What is the proper way to GetCulture() when you need a PublishedCultureInfo and not a DateTime?
Is there a new method for that?

Emetico picture Emetico on 11 Jul 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

creativesuspects picture creativesuspects  路  3Comments
Myronics picture Myronics  路  3Comments
HannesWinston picture HannesWinston  路  3Comments
glombek picture glombek  路  4Comments
callumbwhyte picture callumbwhyte  路  3Comments