Unpublishing and editing content item with an uninitialized ContentPickerField breaks (Lombiq Technologies: OCORE-92)

[sarahelsaig]

Fixes #11527

[Skrypt]

Just some thought here. Could we do the same but without using Newtonsoft.Json and replace it with System.Text.Json while we are at it?

[sarahelsaig]

The ContentItem.Content points to OrchardCore.ContentManagement.ContentElement.Data which is a Newtonsoft JObject. This class doesn't do any serializing, just access this existing property so trying to use System.Text.Json here would require conversion. Better table it until ContentElement itself is ported to STJ, I think.

[Skrypt]

Ok, forget it for now then.

[ns8482e]

Will Linq be concern here for allocation and performance?

[sarahelsaig]

We pass on the enumerables without using an intermediary list like before, so that should improve allocation. I'm not sure about the performance, but we don't do anything complex so it should be fine. Also these providers are only called when you update a content item's field; that's relatively rare so you shouldn't be concerned.

[jtkech]

Why not just replacing (JObject) castings by as JObject.

Hmm, maybe from the beginning would have been better to use the ContentElement extensions already using as JObject and cached Elements, this in place of using directly the dynamic Content.

So for now can you try the above suggestion and see if it fixes the issue.

[sebastienros]

does is { } mean != null. If so then != null is easier to comprehend.

[sarahelsaig]

It's a combined operation for pattern matching introduced in C# 8. It does != null check and assignment to the field variable. So it makes the code more compact and lowers the cognitive complexity (once you are used to it anyway). Back in C# 7 we'd use jField.ToObject<TField>() is var field for the same, but starting C# 8 this empty object pattern is the norm.

If I replaced this with != null then it would look like this:

        jField.ToObject<TField>() != null
            ? jField.ToObject<TField>()
            : null;

I think that's less efficient and visually noisier. Please comment if you want C# 8 style, C# 7 style, or the old fashioned null check and explicit assignment.

[sebastienros]

and lowers the cognitive complexity (once you are used to it anyway)

Like pain, after a while you don't realize it's there, so pain isn't real actually.
We had a meeting today and nobody could explain what it does. Everyone could explain != null though.

Please keep the old style, and add more lines if necessary for readability or prevent two calls to ToObject.

[sarahelsaig]

It's not like pain, more like learning a new technique. First it's weird and awkward, but it you give it a chance you start seeing the utility in practice and it grows on you. Change is good.
(also we use this pattern matching in Lombiq everywhere)

But it's you guys' repo, so I abide by your decision and have rewrote the method.

[ns8482e]

May be you can use jField is not null it’s more readable

[sarahelsaig]

Ok. I have already eliminated the null check here, but I applied your suggestion to the null check in the other method in this file.

However this is starting to get a bit confusing. Does the OC repo have a style guide? I couldn't find it. If not, do you have plans to make one?

[ns8482e]

https://github.com/OrchardCMS/OrchardCore/blob/main/CONTRIBUTING.md

[sarahelsaig]

That links to ASP.Net Core's engineering guidelines. If you can edit that, it would be good to add something about null checking conventions based on this discussion. If not, an OC-specific guide would still be valuable to keep a record of such decisions. Just a thought.

[Skrypt]

No need to add documentation about this. We are just giving comments here based on our coding preferences because it is easier to read. Basically, adding documentation will not prevent this from happening so we give our advice on every pull request. Getting code merged into source code can take more time than we think at first. You will get used to it over time. Everyone has comments and suggestions, that is why pull requests exist.

[sebastienros]

Apart from the new c# is { } usages this is fine

[sarahelsaig]

Do I need to do anything else or is this ready to be merged?

[jtkech]

As said I would have just replaced the (JObject) castings with as JObject to fix the issue.

var jPart = contentItem.Content[fieldDefinition.PartDefinition.Name] as JObject;

Also, @sebastienros why not making the ContentElement JObject Data getter public (only keep the setter internal), this in place of dealing with the dynamic Content, as we do in ContentExtensions.

var jPart = contentItem.Data[fieldDefinition.PartDefinition.Name] as JObject;

Or at least do a cast on the contentItem.Content itself before accessing its properties, less reflections.

var jPart = ((JObject)contentItem.Content)[fieldDefinition.PartDefinition.Name] as JObject;

But this PR LGTM so I will approve.

Hmm, as suggested above, maybe still add a cast on the contentItem.Content itself, e.g. for this line.

if (((JObject)contentItem.Content)[fieldDefinition.PartDefinition.Name] is not JObject ...)

Or directly use .Data if we make its getter public.

But yes all the above is not crucial as here we are updating items, but making the .Data getter public may be useful for less reflections in other places using .Content, maybe another PR if worth to do ;)

So as said I will still approve this PR ;)

[Skrypt]

I agree with the as JObject; comment.

[sarahelsaig]

As said I would have just replaced the (JObject) castings with as JObject to fix the issue.

If possible I'd prefer to keep using is not JObject because it allows the type checks to be chained with boolean operators. After all it's one big type checking operation to reach the JField anyway so sticking them together seems logical.

Hmm, as suggested above, maybe still add a cast on the contentItem.Content itself, e.g. for this line.

Good point, I have applied the suggested cast.

But yes all the above is not crucial as here we are updating items, but making the .Data getter public may be useful for less reflections in other places using .Content, maybe another PR if worth to do ;)

If this comment gets two thumbs ups I will do it :)
(I mean make the getter public and replace .Content with .Data where applicable in a separate PR.)

[jtkech]

@DAud-IcI Okay, all good then for me, thanks for the change.