Spin-off from #1992138: Helper issue for "field types as TypedData Plugins" / #1969728: Implement Field API "field types" as TypedData Plugins.
That issue adds actual validation on entity fields.
This means that implicit or explicit constraints contained in FieldItem::getPropertyDefinitions() (e.g 'type' => 'uri' in LinkItem, 'type' => 'integer' for 'tid' in TaxonomyTermReferenceItem...) that were just "dormant" so far in HEAD, are now fired.
When failed, the 'type' constraints generate messages like : "this value should be of type 3" :-)
That's because internally the 'types' that are used by the PrimitiveTypeConstraintValidator are integer-based class constants in the Primitive class (Primitive::INTEGER is 3, Primitive::URI is 7...)
Also, this bug aside, those messages are not ideal UX-wise.
- We typically prefix the validation errors reported in forms with the name of the field:
"My custom link: this value should be..."
- The unique common template for those error messages makes awkward formulations:
"... should be of type integer / ... should be of type uri / ..."
We tend to generate more nicely put feedback: "this should be an integer, this should be a URL", but that's difficult to capture in one single fits-all message template.
I guess each primitive type could define its own error message ? Possibly even overridable in the propertyDefinition itself ?
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 2012690-12-improve-type-constraint-error-for-string-data.patch | 1.52 KB | mlncn |
Comments
Comment #1
fagoComment #2
fagoThere are labels for the primitives in the type definitions, we could read them from there but we need to add information on which type is the "default type" for a primitive.
Comment #3
effulgentsia commentedDiscussed this with fago and yched in IRC. This will likely need us to create a DrupalExecutionContext that extends Symfony\Component\Validator\ExecutionContext, so that we can override addViolation() and friends to insert additional $params (e.g., name/title of field), that it could get from the Constraint instance definition.
fago suggested checking in to see if Symfony wants this capability upstream, but given our upcoming code freeze, I suggest doing whatever's fastest at this point. However, it probably makes sense to at least run a
composer updateon the Validator component, to make sure we're not coding against something already obsolete.Comment #4
fagoCurrently the Email field type has a dupcliated Length constraint in order to have a better message. So once we fix this we should remove that duplicated Length constraint there also - see https://drupal.org/node/2023563#comment-7943883.
Comment #4.0
fagoadd structure
Comment #5
clemens.tolboomThis issue is referenced in core/modules/comment/src/Tests/CommentValidationTest.php:128 and I ran into this through #2220381: "This value should be of the correct primitive type" error when field has no value
Comment #12
mlncn commentedThis is surely not what we hope for as a proper patch, but it's a start, so people who find this issue can at least find where this terrible error message is being produced, and try to produce a better one at least for their own purposes.
Feedback on the language for this particular error message i'm suggesting—"String data cannot be an object or an array unless it is a markup object."—could help the final correct form of this issue also.
Comment #15
ofrommel commentedI have also written a small patch (that failed some tests) https://www.drupal.org/project/drupal/issues/3056548. Your patch is probably better but you could probably also use $typed_data->getName() somewhere to deliver some more information.
Comment #19
quietone commentedTriaging for bugsmash.
@yched, Thanks for suggesting improving the error message.
This seems to be just about improving an error message. This will needs an Issue Summary update that explains the proposes resolution and a test. Adding tags for that. And steps to reproduce would help future reviewers. OH, and when there is a working patch screenshots before and after will be needed.
Comment #21
quietone commentedMaking this a meta to keep track of where the error messages need to be changed. And making #2925445: "this value should be of the correct primitive type" on empty integer fields a child issue.
Comment #22
clemens.tolboomAdded ref to issue mentioned by @ofrommel in #15