We have moved to GitHub Issues
Created by Dan Booth 20 Dec 2017, 10:38:27 Updated by Sebastiaan Janssen 28 Jun 2018, 18:07:29Tags: PR
Relates to: U4-11478
Umbraco has a number of useful extension methods that hang off
Umbraco.Web.PublishedContentExtensions. Most of these methods now how a generic overload that returns a strongly typed model instead of just
IPublishedContent. For example:
FirstChild<T> method returns
IPublishedContent. For consistency it should really return
var child = Model.FirstChild<BlogPost>() current returns
IPublishedContent when it should return it as type
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.
@Bjarne.Fyrstenborg I've made a pull request to change the behaviour - https://github.com/umbraco/Umbraco-CMS/pull/2623
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
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
The extra tests are great! But the additional
OfType<T> isn't needed (unless I'm missing something?)
@Shandem so I guess in your example
asdf is of type
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;
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
T) ... unless there's something odd with older versions, i tested with 7.11
@Shandem I think the issue was with the signature of the original methods, as the return type was
IPublishedContent rather than
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!
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.
@Shandem If that is the case then all I could suggest is making these new methods ie.
FirstChildAs<T> would be awesome! 👍
@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?
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.
@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?
@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
@sebastiaan OK, makes sense! I've added the new method and updated the PR and pushed, so you can review.
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