U4-10778 - FirstChild extension returns IPublishedContent rather than model of type T

Created by Dan Booth 20 Dec 2017, 10:38:27 Updated by Sebastiaan Janssen 28 Jun 2018, 18:07:29

Tags: PR

Relates to: U4-11478

Umbraco has a number of useful extension methods that hang off IPublishedContent in Umbraco.Web.PublishedContentExtensions. Most of these methods now how a generic overload that returns a strongly typed model instead of just IPublishedContent. For example:

Descendants returns IEnumerable

Descendant returns

Next returns

However, the FirstChild<T> method returns IPublishedContent. For consistency it should really return <T>. eg.

var child = Model.FirstChild<BlogPost>() current returns child as IPublishedContent when it should return it as type BlogPost (when BlogPost implements IPublishedContent interface).

Methods can be seen: https://github.com/umbraco/Umbraco-CMS/blob/e0025db56d52b770d2b3aedbd48a3b804fd15ef0/src/Umbraco.Web/PublishedContentExtensions.cs#L1808

Comments

Bjarne Fyrstenborg 20 Dec 2017, 11:10:28

I have noticed this as well. It would be great to return instance of T when working with ModelsBuilder, e.g.

var root = Model.Content.Site();
var basket = root.FirstChild<Umbraco.Web.PublishedContentModels.Basket>()

In this case the variable basket is of type IPublishedContent instead of Basket.


Dan Booth 11 May 2018, 09:30:26

@Bjarne.Fyrstenborg I've made a pull request to change the behaviour - https://github.com/umbraco/Umbraco-CMS/pull/2623


Shannon Deminick 20 Jun 2018, 05:35:42

This seems odd. Of course it's going to return some of type IPublishedContent because that is what it is, but it still returns type T, it can't not return type T because the method explicitly returns type T.

I've tested this PR and removed the OfType<T> call and all tests still work as expected which goes to show that this extra code doesn't need to exist. I've also added a custom property to the Home class called Hello and in the test FirstChildT if I do var asdf = doc.FirstChild<Home>(x => true); // predicate I can also do var blah = asdf.Hello; and it compiles and works just fine because this method can only return type T.

The extra tests are great! But the additional OfType<T> isn't needed (unless I'm missing something?)


Bjarne Fyrstenborg 20 Jun 2018, 07:43:46

@Shandem so I guess in your example asdf is of type Home?

In a current project on Umbraco 7.7.13 and with ModelsBuilder I have to do on of the following.

var root = Model.Content.Site();
Basket basket = root.FirstChild<Basket>().OfType<Basket>();
var root = Model.Content.Site();
Basket basket = root.FirstChild<Basket>() as Basket;


Shannon Deminick 20 Jun 2018, 08:11:08

yes correct. The call to root.FirstChild<Basket>() must return the type Basket, it's how the generic is compiled, there's no way that it cannot return Basket (type T) ... unless there's something odd with older versions, i tested with 7.11


Dan Booth 20 Jun 2018, 08:17:21

@Shandem I think the issue was with the signature of the original methods, as the return type was IPublishedContent rather than T.

For the overload with predicate ie. FirstChild<T>(this IPublishedContent content, Func<T, bool> predicate) then you are probably right that it doesn't need the additional .OfType<T>() "cast" at the end. I think I was just following the conventions in the rest of the code.

Basically, the end result we want is to be able to call FirstChild<T> and return the actual type without the need to cast. If the code can be reduced to enable that, then all the better!


Shannon Deminick 20 Jun 2018, 08:21:35

Ahhhh, yes i see what's happened here, i didn't notice the signature change in the PR.

Well... one would argue this is a (un)breaking change because the method signature is changing which means if you are using this in compiled code and you drop in a new DLL with this signature change you'll get a YSOD saying that it cannot find the same method and you'd need to compile.

But yeah, the issue is this signature public static IPublishedContent FirstChild<T>(this IPublishedContent content)

I'm not sure what to do, we can't change this signature since we won't be able to upgrade sites without people recompiling if this is used in their compiled code.


Dan Booth 20 Jun 2018, 08:28:00

@Shandem If that is the case then all I could suggest is making these new methods ie. FirstChildTyped<T> or FirstChildAs<T> ?


Sebastiaan Janssen 25 Jun 2018, 09:02:22

@DanDiplo FirstChildAs<T> would be awesome! 👍


Dan Booth 25 Jun 2018, 10:55:59

@sebastiaan OK, so how would I proceed with this? Make a new PR or is there a way of amending the current PR? Or would you guys just change the method?


Sebastiaan Janssen 25 Jun 2018, 11:02:47

Ah, just go back to your temp-U4-10778 branch, make changes, commit and push! That will update the existing PR with the latest changes for us to review.


Dan Booth 25 Jun 2018, 12:09:25

@sebastiaan Ahh, cool, I'll do that then! One question: Should I mark the original FirstChild<T> as deprecated now (using the Obsolete attribute?), or just leave it?


Sebastiaan Janssen 25 Jun 2018, 12:13:21

@DanDiplo I think we want to leave it, this will be a temporary fix and in v8 we'll need to make sure that FirstChild works as expected. I'll create an issue for that now.


Sebastiaan Janssen 25 Jun 2018, 12:15:41

http://issues.umbraco.org/issue/U4-11478


Dan Booth 25 Jun 2018, 12:29:15

@sebastiaan OK, makes sense! I've added the new method and updated the PR and pushed, so you can review.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category: Architecture

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.7.5, 7.6.12, 7.7.6, 7.7.7, 7.7.8, 7.7.9, 7.7.10, 7.7.11, 7.7.12, 7.7.13

Due in version: 7.12.0

Sprint:

Story Points:

Cycle: