-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
Include attribute name in error messages when it's present #644
Conversation
dood-
commented
Jan 2, 2024
Q | A |
---|---|
Is bugfix? | ❌ |
New feature? | ✔️ |
Breaks BC? | ❌ |
Fixed issues | #630 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #644 +/- ##
============================================
+ Coverage 94.60% 94.72% +0.12%
- Complexity 796 801 +5
============================================
Files 93 93
Lines 2426 2483 +57
============================================
+ Hits 2295 2352 +57
Misses 131 131 ☔ View full report in Codecov by Sentry. |
PR Summary
Please note this is a high overview of the changes and not a granular breakdown. |
Co-authored-by: Alexander Makarov <[email protected]>
Since I added a new placeholder |
@vjik, @arogachev what do you think? |
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.
Hm. Is there a case when need attribute in message and at the same time, there is a label?
Seems, it's overhead. I suggest use placeholder {attribute}
only and return translated label, if it exist, or translated attribute otherwise.
@samdark, @vjik If we leave "The value" as a default value, then there is a problem with displaying the attribute names of arrays. Here is a test for an example: validator/tests/Rule/OneOfTest.php Lines 172 to 180 in 1e73321
We can not specify a Label attribute for the array or implement the LabelsProviderInterface, so the placeholder value will always be "The value" |
There will be no default value. Will be one attribute
For case with arrays always will be used translated attribute. |
If necessary, I can remove Attribute::TARGET_CLASS from the Label attribute. |
Yes, makes sense. |
Is it possible to disable mutants for a specific php version or just use @infection-ignore-all? |
I guess @vjik meant fixing tests so there are no mutants, not ignoring these. |
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.
Overall looks good 👍
There's just a little bit left to finish.
Thanks for you great work! I will review this as well. |
@@ -74,7 +74,7 @@ final class User | |||
|
|||
> **Note:** [readonly properties] are supported only starting from PHP 8.1. | |||
|
|||
Error messages may include an `{attribute}` placeholder that is replaced with the name of the property. If you would | |||
Error messages may include `{attribute}` or `{Attribute}` placeholder that is replaced with the name of the property. If you would |
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.
Need to cover the differences between the two.
src/Rule/AtLeastHandler.php
Outdated
'attribute' => $context->getTranslatedAttribute(), | ||
'Attribute' => StringHelper::uppercaseFirstCharacter($context->getTranslatedAttribute()), |
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.
These seem to be redundant in this case, because here there are multiple error attributes, not just a single one.
*/ | ||
public function getTranslatedAttribute(): ?string | ||
public function getTranslatedAttribute(): string |
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.
Maybe add a method like getCapitalizedTranslatedAttribute()
or additional argument in existing method with already applied StringHelper
? That would reduce the code a bit.
src/Rule/CompareHandler.php
Outdated
@@ -61,6 +63,7 @@ public function validate(mixed $value, object $rule, ValidationContext $context) | |||
|
|||
return (new Result())->addError($rule->getMessage(), [ | |||
'attribute' => $context->getTranslatedAttribute(), | |||
'Attribute' => StringHelper::uppercaseFirstCharacter($context->getTranslatedAttribute()), |
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.
Maybe group common parameters here and in other similar places?
'>=' => 'Value must be greater than or equal to "{targetValueOrAttribute}".', | ||
'<' => 'Value must be less than "{targetValueOrAttribute}".', | ||
'<=' => 'Value must be less than or equal to "{targetValueOrAttribute}".', | ||
'==', => '{Attribute} must be equal to "{targetValueOrAttribute}".', |
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.
targetValueOrAttribute
- support capitalization as well?
@@ -62,7 +62,7 @@ final class AtLeast implements DumpedRuleInterface, SkipOnErrorInterface, WhenIn | |||
public function __construct( | |||
private array $attributes, | |||
private int $min = 1, | |||
private string $incorrectInputMessage = 'The value must be an array or an object.', | |||
private string $incorrectInputMessage = '{Attribute} must be an array or an object.', | |||
private string $message = 'At least {min, number} {min, plural, one{attribute} other{attributes}} from this ' . |
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.
attributes
- support capitalization as well?
Co-authored-by: Alexander Makarov <[email protected]>
Thank you! |