umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.45k stars 2.68k forks source link

TemplateId is never null #8635

Closed callumbwhyte closed 2 years ago

callumbwhyte commented 4 years ago

The TemplateId property on IPublishedContent is a int? but never seems to return a null value... In cases where no template is assigned the value is 0.

I have tested on a couple of my sites and confirmed that while there is no templateId stored against the document, 0 is still returned rather than null. This means that checking if a content item has a template requires checking the value...

content.TemplateId.HasValue && content.TemplateId > 0

...which is a little messier than simply checking if it has a value at all:

content.TemplateId.HasValue

Interestingly, Umbraco falls foul of this issue internally too. For example, the GetTemplateAlias extension method will always try and lookup a template regardless.

If the expected behaviour is 0 should be returned, then I don't see why the property is nullable... From looking at core, and a few places where the property is directly set to null, I would assume this is a bug 😊

bjarnef commented 4 years ago

@callumbwhyte I haven't checked this with recent changes and if it has changed in Umbraco v8, but previous I think TemplateId was null when no template had been assigned to the document type or selected on the content node, but instead used route hijacking to return a view from a controller inheriting from RenderMvcController.

callumbwhyte commented 4 years ago

"I think TemplateId was null when no template had been assigned to the document type or selected on the content node"

Yes, this is the behaviour I'm expecting but doesn't seem to be the case. Not entirely sure what you're saying about route hijacking?

In V7 TemplateId wasn't nullable and so would return 0 if nothing was assigned - this was an equally good solution, at least a simple >= check can be performed against it.

bjarnef commented 4 years ago

You could have no template selected on content node and using CurrentTemplate, but you could also return View("my-view.cshtml, model) if not templates allowed have been selected on the document type. https://our.umbraco.com/documentation/reference/routing/custom-controllers

I had a quick look at v7 and it seems you're right it wasn't nullable. Not sure if it potentially can be null in v8 though (maybe when using variants?).

callumbwhyte commented 4 years ago

Certainly, I think I'm just misunderstanding what that has to do with the TemplateId being null / not? 😊 Unless you're talking in the context of the PR to my package which I reference above?

Either way, I would expect TemplateId to be null if no template is assigned to the IContent/IPublishedContent entity. Routing should have no bearing on the actual entity. And it seems the core also works on this assumption in places, e.g. the GetTemplateAlias extension method...

bjarnef commented 4 years ago

Do you get a different TemplateId in the following scenarios?

callumbwhyte commented 4 years ago

In both of the above cases 0 is returned. Checking the database there is no entry in the umbracoDocumentVersion table (where templateID is assigned to nodeId) in either case, so it's not like 0 is being persisted anywhere...

nul800sebastiaan commented 4 years ago

The change was introduced here, and first included in 8.8.0: https://github.com/umbraco/Umbraco-CMS/pull/3583

To me it sounds like the bug is that in our codebase we still do checks for 0 where we could just do a null check. Or the bug is that we set the TemplateId to 0 instead of null? I haven't looked into it.

umbrabot commented 2 years ago

Hiya @callumbwhyte,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face: