Add JsonStringEnumConverter to KnownConverters #16913
Conversation
|
@hieucd04 could you please try this PR at your end |
|
@hishamco I can only help testing nuget packages, pulling them into my code base on my end then run the code and check if they work or not. I've never built Orchard's code base from source. I'm sorry! |
|
I already tested the PR and works great |
| @@ -16,6 +16,7 @@ public static class JOptions | |||
| PathStringJsonConverter.Instance, | |||
| TimeSpanJsonConverter.Instance, | |||
| DateTimeJsonConverter.Instance, | |||
| new JsonStringEnumConverter() | |||
There was a problem hiding this comment.
First, you should create a static instance of this converter and the static instance. This will fix the issue. However, this will change the behavior of how we store enum moving forward from int to string.
But it could increase the size of data being transmitted from and to the server. Personally, I am okay with storing strings instead of int since this will ensure we don't have enum problem if someone adds a new enum value in position other than the end of the enum.
I would also document this change in the release notes to ensure people are aware of this change.
@sebastienros thought on this global behavior?
There was a problem hiding this comment.
If we don't own the converter it's fine to not have the Instance property.
Does it work with both int and string as input, to be sure it's not breaking any.
There was a problem hiding this comment.
We don't own the converter
Does it work with both int and string as input, to be sure it's not breaking any.
It should, but I will try another test
There was a problem hiding this comment.
@sebastienros yes it works with both int and string. However, it will write string moving forward and not int. It won't break anyone but will change the behavior.
Fixes #16879