Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V14: Add JsonPropertyName to label configuration #17435

Open
wants to merge 1 commit into
base: v14/dev
Choose a base branch
from

Conversation

nikolajlauridsen
Copy link
Contributor

@nikolajlauridsen nikolajlauridsen commented Nov 6, 2024

Fixes: #17192 and #16613

The issue arose after using concrete configuration objects. specifically from #13605.

The issue is that in the database, and in what the frontend sends the configuration key is umbracoDataValueType, however, the property in the configuration object is called ValueType, this means that when we serialize/deserialize in ConfigurationEditorOfTConfiguration.ToConfigurationObject the value isn't deserialized into the property properly, and it always defaults to "STRING".

To fix this I've added the JsonPropertyName to ensure proper serialization, however, this fix doesn't feel quite right, but I'm having a hard time finding a better solution given the whole serialize/deserialize situation. It seems like in the past this is what the ConfigurationField was used for, which can also be seen in DiscoverFields .

The "most correct" approach would probably be to re-implement ToConfigurationObject without using JSON and taking LabelConfiguration into account; however, this is a bit beyond the scope of this bugfix, I think.

Testing

  1. Run the site with source code models generator
  2. Generate models
  3. Enure the Image models UmbracoHeight is of type integer.

This item has been added to our backlog AB#46951

@nielslyngsoe nielslyngsoe added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants