Skip to content

Conversation

bjarnef
Copy link
Contributor

@bjarnef bjarnef commented Sep 26, 2025

Prerequisites

  • I have added steps to test this contribution in the description below

Description

Fixes part of discussion #20280

Currently the validate method assumes the configuration field is items, but not necessary if re-using the validators across property editors in core and in custom property editors.

I guess we possible can obsolete the original Validate method not taking ConfigurationField as parameter. I considered if field could be a property on PropertyValidationContext, but it seems it is used for other things at content node properties.

chrome_s4zGY1mpxL

I have checked in for ValueListUniqueValueValidator.

devenv_bIgBA5m9xe

In BlockEditorMinMaxValidatorBase class I am not sure if it is the right approach - perhaps not needed here as use in BlockList and BlockGrid property editors, while the others are used in datatype configuration editor.

@Copilot Copilot AI review requested due to automatic review settings September 26, 2025 22:13
Copy link

Hi there @bjarnef, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

  • It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
  • The automated tests all pass (see "Checks" tab on this PR)
  • The level of security for this contribution is the same or improved
  • The level of performance for this contribution is the same or improved
  • Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
  • If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
  • 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR extends the validation system for datatype configuration fields by adding an overloaded Validate method that includes the configuration field as a parameter. The primary purpose is to allow validators to access field metadata during validation, which enables more context-aware validation error messages.

Key changes:

  • Added a new validation method signature that includes the configuration field parameter
  • Updated the validation call site to pass the field to validators
  • Implemented the new method in the existing validator to provide field-specific error context

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
IValueValidator.cs Adds new validation method signature with default implementation
ConfigurationEditor.cs Updates validation call to pass field parameter
ValueListUniqueValueValidator.cs Implements new validation method with field-aware error messages

Comment on lines 86 to 91
var duplicateValues = items
.Select(item => item)
.GroupBy(v => v)
.Where(group => group.Count() > 1)
.Select(group => group.First())
.ToArray();
Copy link
Preview

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .Select(item => item) on line 87 is redundant and serves no purpose. This line should be removed to improve code clarity.

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndyButland it seems the .Select(item => item) is redundant. In a similar for a custom property editor I have .Select(item => item.Value) though, as the one can only be used for string array, not array of objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant