CreateModelMaybeAsync encourages bad practice (OSOE-894)

[MikeAlhayek]

Not that we have SiteDisplayDriver<> I suggest removing the CreateModelMaybeAsync

Helpful-Libraries/Lombiq.HelpfulLibraries.OrchardCore/Contents/BuildEditorContextExtensions.cs

Line 8 in 2111aa6

public static async Task<T> CreateModelMaybeAsync<T>(

for two reason,

1. The SiteDisplayDriver checks for the groupId already, so now this is a duplicate check.
2. The recommended approach is OC is to update the model even when the model validation fails. This methods encourages to not update the model if the model state fails.

Jira issue

[Piedone]

Yeah, that makes sense, thank you.

[sarahelsaig]

I disagree with removing the method altogether.

The recommended approach is OC is to update the model even when the model validation fails. This methods encourages to not update the model if the model state fails.

This has already been addressed in branch task/system-text-json-migration by returning the view-model even if TryUpdateModelAsync returns false. The method also does optional authorization with the authorizeAsync parameter which remains useful. The new version only returns null if there is authorizeAsync was provided and it fails. If the authorization fails then there is no point updating the model anyway.

The SiteDisplayDriver checks for the groupId already, so now this is a duplicate check.

Okay, but that doesn't help with already existing drivers that directly inherit from SectionDisplayDriver<TModel, TSection> instead of SiteDisplayDriver<TSettings>. The double check is a tiny overhead which is no great cost for making it foolproof and backwards compatible with existing code.

Also CreateModelMaybeAsync returns a view-model instance, you don't have to separately instantiate an empty view-model variable in the previous line. So it's aesthetically superior to using TryUpdateModelAsync.

What we could do instead, is splitting the method into two separate overloads with or without the string groupId parameter. Then mark the version with groupId as [Obsolete("Please inherit your driver from SiteDisplayDriver which already checks the GroupId")].

[MikeAlhayek]

Where do you use it outside of SectionDisplayDriver<ISite,TSection> aka SiteDisplayDriver?

I also think it also hides too much logic and makes the code a bit more harder to read and understand.

Writing the logic directly into the UpdateAsync is much cleaner and readable form my point event of view if that means writing little more "readable" code.

[sarahelsaig]

Where do you use it outside of SectionDisplayDriver<ISite,TSection> aka SiteDisplayDriver?

Nowhere, I think you misunderstood. If the check is in SiteDisplayDriver then it won't be applicable to existing drivers (which inherit from SectionDisplayDriver directly) until the developer takes the initiative to update their code manually. Removing the check outright would break the code for existing legacy drivers without warning.

I also think it also hides too much logic and makes the code a bit more harder to read and understand.
Writing the logic directly into the UpdateAsync is much cleaner and readable form my point event of view if that means writing little more "readable" code.

I strongly disagree, it only hides boilerplate logic. That's the purpose of shortcut methods. Also I don't think repeating simple scaffolding logic makes the code more readable, it just makes the reader's eyes glaze over with repetition, making it harder to spot meaningful change. Also makes the code base harder to search and more tedious to maintain too.

[Piedone]

Sári did what she described above under #281.