ContentPartFieldDefinition Settings.ToObject deserializes to empty object

[davidpuplava]

Describe the bug

Content Part Fields Settings are not showing on the edit screen because they are NOT deserialized successfully.

Orchard Core version

Commit: f9fb26a

To Reproduce

Steps to reproduce the behavior:

1. Setup default Blog recipe site
2. Go to Content->Content Definition->Content Types
3. Click on Blog Post
4. Scroll down to Fields section
5. Click Edit button next to Subtitle
6. Enter a value for "Hint" such as "Test Hint"
7. Click Save
8. Scroll back down to Fields section
9. Click Edit button next to Subtitle
10. Observe that the Hint text field is empty

Expected behavior

Expected to see the "Test Hint" value in the "Hint" text field

Logs and screenshots

Here is a screen capture of the issue (this GIF also shows a potential solution).

Analysis

Looks to be related to the removal of Newtonsoft. The TextFieldSettingsDriver.cs file changed to the following block of code to deserialize the text field settings:

        return Initialize<TextFieldSettings>("TextFieldSettings_Edit", model =>
        {
            var settings = partFieldDefinition.Settings.ToObject<TextFieldSettings>();

            model.Hint = settings.Hint;
            model.Required = settings.Required;
            model.DefaultValue = settings.DefaultValue;
        }).Location("Content");

This particular line of code returns an empty TextFieldSettings object because the Settings property is an array of 2 JsonObjects rather than a TextFieldSettings object directly.

Possible Solutions

Inspecting the Settings property on the ContentDefinition type has a comment that says not to access directly and instead use the GetSettings method.

Changing to use GetSettings method does appear to correctly deserialize the settings object.

Other Considerations

This looks to affect other field settings such as BooleanFieldSettings. Searching the code base, I see 20 instances of ...Settings.ToObject<... which likely need to be tested and fixed as well.

Most importantly there are Migrations in OrchardCore.ContentManagement\Records that look to access the Settings in the same way. I'm not sure of the implications of changing that migration to use GetSettings.

[github-actions]

Thank you for submitting your first issue, awesome! 🚀 We're thrilled to receive your input. If you haven't completed the template yet, please take a moment to do so. This ensures that we fully understand your feature request or bug report. On what happens next, see the docs.

If you like Orchard Core, please star our repo and join our community channels.

[davidpuplava]

I can submit a PR to fix this for deserializing settings in the DisplayDriver classes. But I need guidance on if I should also change to use GetSettings in the Migration classes.

[MikeAlhayek]

@davidpuplava I think using .GetSettings<>() should be the way to go. Can you please confirm that .GetSettings<>() fixes the issue?

If that indeed fixes the issue, you may also want to fix it here

https://github.com/search?q=repo%3AOrchardCMS%2FOrchardCore%20%22.Settings.ToObject%3C%22&type=code

[davidpuplava]

Yes, at least in the TextFieldSettings display driver it fixes it. I'll confirm in the other places that there is a problem before I fix it. Thank you @MikeAlhayek.

[github-actions]

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

[gvkries]

Oh no, how did we miss this. It's breaking 2.0 and should be fixed asap.

[davidpuplava]

I should have the PR ready here shortly, in the next couple of hours. I wrote some unit tests to check each field type.

[hishamco]

Waiting for your PR

[davidpuplava]

PR submitted: #16774