[Validator] Fixed string conversion in constraint violations #10687
Conversation
webmozart
commented
Apr 10, 2014
webmozart
commented
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #10675 |
| License | MIT |
| Doc PR | - |
| } | ||
|
|
||
| if (is_array($value)) { | ||
| return 'Array'; |
There was a problem hiding this comment.
maybe add the array size like Array(2) since Object and Resource also add further data
There was a problem hiding this comment.
I don't really like that writing, since Array(2) is the output of print_r when an array contains only the value 2 and might cause confusion.
There was a problem hiding this comment.
print_r returns Array ( [0] => 2 )
There was a problem hiding this comment.
I still don't see the usefulness of this return value. It should format the value, i.e. the contents, but it basically returns the same as formatTypeOf. It returns the type and not the contents of the array. So it does not add anything new. So ideally it should output a concise representation of the array like ['foo', 1, 'Object(Author)']. Or at least the array size as I suggested above.
Imagine the this is used in the CountValidator.
There was a problem hiding this comment.
I don't think this value needs to be very useful. In most cases, validation messages are displayed to the end user. Displaying something like ['foo', 1, 'Object(Author)'] to an average end users frightens them because they think something is broken.
Developers can always call $violation->getInvalidValue() to analyze the value in detail.
There was a problem hiding this comment.
Resource ID and object class are also implementation detail not intended for end users.
There was a problem hiding this comment.
I agree with both of you @webmozart and @Tobion!
If this is displayed to the end users, we should not be too precise; so the resource id is not needed, but I would even say that displaying array, object, or resource to the end user looks weird as he probably did not enter that in a form value anyway.
There was a problem hiding this comment.
Ok, I will change these outputs to just "array", "object" and "resource" then. I agree that this doesn't help end users very much. I'd even go further to say that messages should never include the value if that value is one of these types. If developers choose to include the value in the message anyway, the displayed information will be minimal.
|
Updated. |
| protected function valuesToString(array $values, $formatDates = false) | ||
| { | ||
| foreach ($values as $key => $value) { | ||
| $values[$key] = $this->valueToString($value, $formatDates); |
There was a problem hiding this comment.
indexing by $key should not be necessary
There was a problem hiding this comment.
Can you clarify what you mean?
There was a problem hiding this comment.
Nvm, didn't see you replace the interated array values. Maybe using another array like $convertedValues[] = ... would be cleaner.
|
I now introduced calls to |
| * | ||
| * @return string | ||
| */ | ||
| protected function formatValues(array $values, $formatDates = false) |
There was a problem hiding this comment.
in formatValue the parameter is named $prettyDateTime. Seems incosistent at the moment.
…ormatValue() calls
…e is an array, object or resource This was decided in the discussion of symfony#10687.
|
Updated. |
| } | ||
|
|
||
| /** | ||
| * Returns a string representation of the value. |
There was a problem hiding this comment.
Could you add some more explanations about when/why/how this is ever called? Something along the lines of:
"This method is used when a constraint error message contains the {{ value }} placeholder. By default, Symfony never includes it, but the developer can override those error messages. It's up to the developer to make sure it actually makes sense to include such string representation of submitted values in error messages."
|
👍 |
1 similar comment
|
👍 |
…e is an array, object or resource This was decided in the discussion of symfony#10687.
…ns (eagleoneraptor, webmozart) This PR was merged into the 2.3 branch. Discussion ---------- [Validator] Fixed string conversion in constraint violations | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #10675 | License | MIT | Doc PR | - Commits ------- 32ae95b [Validator] Added more detailed inline documentation 08ea6d3 [Validator] Removed information from the violation output if the value is an array, object or resource d6a783f [Validator] Renamed valueToString() to formatValue(); added missing formatValue() calls 71897d7 [Validator] Fixed CS cea4155 [Validator] Fixed date-to-string conversion tests to match ICU 51 5aa7e6d [Validator] Added "{{ value }}" parameters where they were missing f329552 [Validator] Simplified and explained the LuhnValidator bff09f2 [Validator] Simplified IssnValidator 224e70f [Validator] Fixed and simplified IsbnValidator fd58870 [Validator] Simplified IBAN validation algorithm 97243bc [Validator] Fixed value-to-string conversion in constraint violations 75e8815 [Validator] Fix constraint violation message parameterization
