-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Unique should support objects fields #47714
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
base: 7.3
Are you sure you want to change the base?
Conversation
This looks like a new feature to me and thus should target 6.2. |
Also, this makes PropertyAccess a required dependency. Better not if possible. And let's throw a useful LogicException when the dep is missing, suggesting to install it (see existing ones in the codebase.) Thanks! |
08428b9
to
535c874
Compare
@nicolas-grekas it's done, not sure about the |
248840f
to
889cb8d
Compare
if (isset($element[$field])) { | ||
$output[$field] = $element[$field]; | ||
|
||
// For no BC, because PropertyAccessor require brackets for array keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For no BC, that simple case should be handled without requiring PropertyAccess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a first time this is what I've done: check if element is an array, then that the key do not contains any "[]." (PropertyAccessor syntax) and the key exists in the element.
In that case directly access to the array value via the key.
And add an Else statement (not array and/or PropertyPath syntax, that contains the if statement (because the actual implementation accept that the key can be missing).
At the end, find simpler to just transform the key in PrpertyPath compliant syntax.
May I ask if someone is still working on this? As I opened an issue recently and learnt about this pull request and I could work on it if no one does. |
@BastienRimbert I’m waiting for some feedbacks/discussions. |
Is this MR will be merged before the next century ? |
@mpiot @nicolas-grekas it's a great feature for Unique validator, it would be great to see it merged. Today even wanted to create PR for same changes. |
@@ -21,6 +25,13 @@ | ||
*/ | ||
class UniqueValidator extends ConstraintValidator | ||
{ | ||
private ?PropertyAccessorInterface $propertyAccessor; | ||
|
||
public function __construct(PropertyAccessorInterface $propertyAccessor = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd configure factor
for this method and onInvalid set null, otherwise it does not make any sense because PropertyAccess does not public constructor and only way to create Accessor is to call static method createPropertyAccessor
|
||
// For no BC, because PropertyAccessor require brackets for array keys | ||
// Previous implementation, only check in array | ||
if (\is_array($element) && !str_contains($field, '[')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do this:
if (is_object($element)) {
if (!class_exists(PropertyAccess::class)) {
throw new LogicException('Using UniqueValidator on object requires "PropertyAccess" component. Try running "composer require symfony/property-access".');
}
$this->accessor ??= PropertyAccess::createPropertyAccessor();
if ($accessor->isReadable($element, $field)) {
$output[$field] = $accessor->getValue($element, $field);
}
}
|
||
// For no BC, because PropertyAccessor require brackets for array keys | ||
// Previous implementation, only check in array | ||
if (\is_array($element) && !str_contains($field, '[')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think validating deeper than first level should be allowed.
Otherwise we may end up with validating uniqueness of other object.
It looks like this PR has been superseded by #48951 |
The Unique validator have a
fields
options, to define whichfields
of an array should be compared (if not set, the entire object or array is compared).But the
fields
option do not work if we defined object attributes (not the original implementation behavior), but should be logic to work with array and object.