Fixing FormatException when the login screen is posted with values other than true/false for RememberMe (Lombiq Technologies: OCORE-132)

[Piedone]

Fixes #14792

[hishamco]

Why we don't move the logic into the controller? The view should have a minimalistic logic that is related to the presentation

[Piedone]

This is a workaround solely for the <input asp-for="RememberMe" class="form-check-input" tabindex="3"> line below, that's where the exception is thrown, due to using the auto-generation of the editor with asp-for. With a different implementation of the field (like by having a checkbox implemented directly) the exception won't be thrown. So, for other implementations of this view the code wouldn't necessarily be appropriate.

[hishamco]

When I initially thought about this issue, I didn't think of creating a workaround like you did. If it's fine for all to accept this workaround, or I might propose another PR to fix it in a proper way

[Piedone]

What did you think about? Alternatives I tried but didn't like better:

• Nullable RememberMe but that would need a hand-crafted checkbox because the tag helper doesn't support nullable booleans.
• Hand-crafted checkbox by itself.
• Custom model binding but that would be just ugly.

[MikeAlhayek]

@Piedone what happens if the client sends a bad value to DateTime or any other types? Likely we'll get similar behavior. Do we really need to implement a workaround for that?

[Piedone]

I guess so. I'd expect this to be a simple validation issue that model binding should handle, but apparently, it doesn't, and I couldn't find any better options, see above. To be clear, the problem is only in the view, due to asp-for.

[MikeAlhayek]

@Piedone implementing this kinds of work around feels wrong to me. So I am going to suggest that we stay away doing this. However, if you think we should support this kind of binding in OC, we should be able to register a custom boolean binder that would be applied through the app. Try implementing IModelBinderProvider and register it using MvcIOptions with something like this

services.AddMvc(options =>
{
    options.ModelBinderProviders.Insert(0, new StringToBoolBinderProvider());
});

[Piedone]

I looked into that approach as well as mentioned, but I don't think it's better. Anyway, I changed it to that, please check.

[MikeAlhayek]

I looked into that approach as well as mentioned, but I don't think it's better. Anyway, I changed it to that, please check.

@Piedone what do you not like about this approach?

I think this is a much cleaner approach. However, I don't think this logic should be in the Users module. I think it should be available in Orchardcore.Mvc.Core project instead. Maybe is can be added here instead https://github.com/OrchardCMS/OrchardCore/tree/8f0c371e95bea724534280f2ee81801699ab14d2/src/OrchardCore/OrchardCore.Mvc.Core/ModelBinding

then register it here

OrchardCore/src/OrchardCore/OrchardCore.Mvc.Core/Startup.cs

Line 84 in 8f0c371

options.ModelBinderProviders.Insert(0, new CheckMarkModelBinderProvider());

[Piedone]

The #14792 issue is specific to what the Users module does in its view, thus I don't think it warrants a custom model binder for the whole app. But perhaps other modules also display a similar checkbox that can benefit from this, so sure, I moved it up.

[Piedone]

Thanks for the review! Anything you'd add @hishamco?

[hishamco]

This might be looks better than the old workaround :)

[hishamco]

@Piedone please merge after fixing the suggested format