Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[DoctrineBridge] In Profiler, Show all fields and values for validation constraints#57963
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Conversation
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.
As it touches the Profiler, would it be possible to have a screenshot showing the before/after? 🙂
src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| @@ -207,16 +207,25 @@ public function validate(mixed $value, Constraint $constraint): void | |||
| } | |||
| } | |||
| $errorPath = $constraint->errorPath ?? current($fields); | |||
| $invalidValue = $criteria[$errorPath] ?? $criteria[current($fields)]; | |||
| $errorPath = $constraint->errorPath ?? implode(', ', array_keys($criteria)); | |||
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.
This change will break the integration into the Form component as the error now can no longer be mapped back to the corresponding form type.
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.
if one field, error stay on field, but if there are many fields, error go on top form instead one arbitrary 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.
it'd be great to have a test case that covers the issue that@xabbuh spotted here
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.
Thinking about this PR again I'd say if we were to change anything here the constraint could accept theerrorPath option to be configured as an array which would lead to the violation being added to all these property paths. But doing the suggested change is a no go to me as it breaks the Form integration as explained.
eltharinAug 17, 2024 • edited by OskarStark
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by OskarStark
Uh oh!
There was an error while loading.Please reload this page.
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.
add to all property can be OK but can create backward compatibility breaks,
but actually we have violation on only one field is not good, it's not A field whitch is bad but a group,
if you set firstname and lastname unique, form will respond to you "firstname is incorrect Christian is already used".
That's why put this to the parent form instead one field is preferable.
I think.
I'm trying to do tests and I'll look for an array (or an object stringable) for errorPath
d485469 to8a7e593Compare8924462 to4d08d19Compare
Uh oh!
There was an error while loading.Please reload this page.
When using UniqueEntity with multiple fields, show all fields and all values in profiler for debug quickly.
Before only one value was shown :
in form errors :
After we have all fields :
and in the form, error is not attached to a field but to the top form :