| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | entity system |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Entity Field API, Entity forms, Entity validation |
Issue Summary
Entity validation needs to be untied from form validation, i.e. have a way to validate the properties (and fields) of an entity object. Currently, validation of an entity is mostly tied to the form system, only field API has a hardly used way to validate field data independently of a form.
Problem / Motivation
If implementing web services or other custom ways to update an entity, APIs for validating the data are necessary. However, in those scenarios no forms are involved, so those should not be required. Also see #1540656: [META] Entity Serialization API for web services (e.g. content staging)
Suggested solution
Add an entity validation API that works based upon a property-based validation API, see #1346214: [meta] Unified Entity Field API. First off property based validation would handle property value validation, while the remaining validation can happen on the entity-level (i.e. validate combinations of property values).
Sub-issues
#2002152: Implement entity validation for the test entity
#2002156: Convert form validation of nodes to entity validation
#2002158: Convert form validation of comments to entity validation
#2002162: Convert form validation of users to entity validation
#2002168: Convert form validation of terms to entity validation
#2002174: Convert form validation of vocabularies to entity validation
#2002176: Convert field validation to entity validation
#2002180: Embrace entity validation in entity forms
Comments
#1
Just to be clear: this applies to fields/properties that are part of the entity, right? I.e. will this also accomodate the use case of "the tags field of a node has a new value, we want to create a new revision of the node with the new value"?
This would then effectively allow us to no longer have to piggy back everything over Form API over at the Edit module. It would also enable us to use e.g. CreateJS, and really, any kind of client-side editing which would then sync changes back to Drupal.
#2
Exactly.
Yep - saving a new revision isn't handled very special from the API, so it's just like an average update which should involve validation.
Yep, having to use FormAPI for webservices is really odd. For Drupal 7 the entity API module's property info system implements some similar, very basic property-based validation, which may be useful for Edit module in d7? For that, validation is done best via the entity-metadata-wrappers. That is what RestWS leverages for validation without relying on form API.
#3
#2: Only for titles and "rich text" fields (i.e. those using a WYSIYWG) I would want to avoid using forms; for the rest, I still need forms. Once this is in, we could move towards e.g. CreateJS and that could change in the Edit module. For now, this issue is likely not important to the Edit module, but it is important to enable future improvements :)
#4
To be even clearer: having that new entity validation API in place, then current form validation logic would be somehow replaced by that entity validation, right ?
#5
Having abstracted validation code from forms is a good idea in general, Zend framework has this for ages (actual Zend_Validate in ZF1 and newest Zend\Validator namespace in ZF2). Forms should rely on such things too it would avoid the #element_validate development hell.
I'm all in for such abstraction, and you should introspect what Symfony 2 offers in this area and extend it if possible.
#6
The Symfony\Validator component seems to be where to start: see https://github.com/symfony/Validator
#7
Yes, entity validation would be incorporated in form validation though. That way we have a central place for validations.
Agreed.
#8
pounard++. I don't recall when & where, but I have been in #element_validate hell too.
It'll be interesting to see how form validation will leverage this new validation system. It would be very cool if the validation rules would be defined in such a way that we could also automatically translate them into JS. That can't work for validation of everything, but for some things it is possible.
#9
Translation to JS is another problem, and tied to FAPI, it may worth its own core issue (even more its own initiative!). Meanwhile the PHP side Validator component is supposed to be context agnostic and used in many contextes (WS, REST, forms, etc...).
#10
Validator is already on 8.x branch? If not there, and it is THE WAY to go, please confirm and i can start working on this ?
#11
Maybe we could create a validator plugin.
then webform_validator, form_validator, field_validator, property_validator inherit validator Class.
Real validators such as :
field_lenght_validator,field_unique_validator,field_regrex_validator,field_php_validator inherit field_validator.
webform_lenght_validator,webform_unique_validator,webform_regrex_validator,webform_php_validator inherit webform_validator.
property_lenght_validator,property_unique_validator,property_regrex_validator,property_php_validator inherit property_validator.
Then we could solve all validation in a unify way.
#12
Yes, I've been thinking about that as well. I think configurable validation plugins, that optionally also come with a JS-variant would be a good way to go. Let's keep it in mind now, but have a closer look on that as a follow-up.
#13
#12: Yes, it's something to just keep in mind, to make sure it remains possible. We shouldn't do this (yet) in core.
#14
Analyzing current code at 8.x, and what to do (where to start, etc)
Would be nice having somebody to help me with this? Maybe a sanbox to work coop on it.
Thanks!
#15
For backward compatibility reasons and in order to avoid the patch to be too invasive over FAPI, a simple FAPI elment #element_validate to Symfony\Validator instance bridge could be done and tested with simple widgets. Once this done, anything will be easy to port.
#16
@pounard: i think we are going farther, your solution will just work, but we are not solving the main issue: having the tools to validate an entity without FAPI. The propossal un #15 maintains the FAPI element, so entity validation are still coupled to FAPI.
#17
ad #15: yep, for the first step I don't think we've to change FAPI much or at all.
So far I've thought about
- mapping entity validation errors to form widgets (what field api already does) and building an interim, updated $entity for invoking validation on it during form validation.
- supporting per property-type validation routines that obey some per-type keys, like max,min for integers or a regex for strings.
- then for complex types the validation routine should live in a method of the provided class, optionally obey some special keys of the property definitions as well
- allow custom validation routines based on a hook, i.e. hook_entity_validate($entity) and/or hook_$ENTITY_TYPE_validate($entity)
#18
@fago: there is the need to respond: where will the constraints for the Validator be defined? will it be PHP ? will it be YAML ?
Also, if FAPI will not change 'at all', i dont see the point on this issue at all! :)
#19
We'll see with what we come up here ;) I think, partly it will be part of the property definitions (max int, ..) and defined as those (might be in modules, might be yaml for fields then..), but I guess we want to allow arbitrary custom PHP conditions as well (e.g. some custom checks in hook_entity_validate()).
The point is to decouple entity validation from entity-forms. Still, we need an API for validating forms that works independently from entities (=FAPI).
#20
@fago: Where are currently those "property definitions" in 8.x ? Is this related to http://drupal.org/node/1346204 ?
#21
Added as follow-up of #1499596: Introduce a basic entity form controller and tagged as such.
The plan we agreed on over there is to introduce a common validation workflow for entity forms exploiting the form-independent entity validation addressed here. Among the rest, as a minor clean-up, we might want to move
form_state_values_clean()intoEntityFormController::validate().#22
now tagging for real
#23
You cannot do that, unless you intend to work on a local copy, since form_state_values_clean() is destructive. form_state_values_clean() may only be invoked in form submission handlers (at which point internal Form API and button values no longer matter).
(That said, but kinda OT, I played with the idea of making Form API itself call into form_state_values_clean() before executing submit handlers. And because I never created an issue for that idea, I just did so:
#1729882: Always invoke form_state_values_clean() before invoking form submission handlers ;))
#24
ok, I started looking at this + discussed it with attiks at badcamp. First off, we explored the following implementation alternatives:
a) Use the symfony validator and call it from the typed data's validate() method.
b) Come up with our own solution
b1) Use some pre-defined supported keys in typed data definitions and check them in validate()
b2) Do a proper API for adding constraints and use those in validate()
a)
We had a close look at the symfony validator component as it would make a lot of sense if we could benefit from all the existing validators. However, we ran into some problems with that:
So we've figured out whether it's possible to validate a value with a single constraint, so we could still re-use the constraints and implement our own logic for traversing through the tree of data and applying constraints. However, it turned out the the contrainst validators require (via a chain of dependencies) the metadatafactory. We figured there is a black-hole-metadata-factory which makes it possible to validate a single constraint on a value, but still that would create a bunch of objects (graphwalker, executioncontext, globalexecution context, constraintvalidatorfactory,...) - and that just for validating a single constraint. So that's probably not really a feasible thing to do per constraint we have to validate.
Given all that, I don't think the symfony validator component is a good fit for us. :/ If we figure out our own solution, we could easily embrace typed-data metadata.. So let's explore that:
b1) We could just go with a pre-defined list of constaints that are defined by data-type, e.g. 'max', 'min' for integers or a 'regex' for strings. That would already account for a lot of basic validation use-cases, while further custom stuff can be added easily in the validate() method of the typed data class.
Further data types like 'email' could easily define their validation logc in the method and/or define other supported constraint keys.
b2) Inspired by the symfony component, we could do constraint validation classes that can be defined separately. Each of the constraint validation class can be applied by defining the constraint in the 'constraints' array - what would make it easy to re-use contraint validation logic across data types - think min or max-length. We do not re-code that for each of the data types - but moreover, this allows reusing the constraint validatiors separately also - e.g. for adding them as fapi element validators.
Finally, having contstraint validators makes it easier to do client side validation as those could be re-built per validator in JS client side.
#25
So here is the initial implementation brainstorming for b2)
- Implement constraint validators as plugins.
- Use a simple array-based notation for specifying constraints below the typed data definitions 'constraints' key, with the key being the plugin name and the value the settings.
Example given
$properties['nid'] = array('label' => t('Node ID'),
'description' => t('The ID of the node of which this comment is a reply.'),
'type' => 'entityreference_field',
'settings' => array('entity type' => 'node'),
'constraints' => array(
'min' => 0,
),
);
Then have classes like
<?php
class ConstraintValidatorExample {
public function validate(\Drupal\Core\TypedData\TypedDataInterface $data, $property_path, $label = NULL) {
$violations = array();
if ($fail) {
$violations[] = new Violation($property_path, 'This fails for %name.', array('%label' => $label));
}
return $violations;
}
}
class Violation {
public function __construct($property_path, $message, $message_args) {
...
}
public function getMessage() {
return t($this->message, $this->messageArgs);
}
public function getPropertyPath() {
...
}
}
?>
Then, the typed data object interface validate method would need to support the property path. This is necessary for doing property violation message and mapping them back to right property.
For having propery labels in the validation messages, we need to use the field label even when the failurs come from field components (e.g. field_text->value). For that, the field item validate() method can get the constraints from the individual value properties and run validate() on them itself in order to pass on a better suiting label.
Then, the property-path is built up so when you call $entity->validate() you'll get violations for e.g. field.value, if you call it on the field you'll get the violation for 'value'. With that information we should be able to properly map the violation to a widget.
#26
more discussion:
$properties['nid'] = array('label' => t('Node ID'),
'description' => t('The ID of the node of which this comment is a reply.'),
'type' => 'entityreference_field',
'settings' => array('entity type' => 'node'),
'constraints' => array(
'type' => 'integer',
'min' => 0,
'non null' => TRUE,
),
);
<?php
// implement validate in FieldItemBase.php and TypedData
// No, implement in $entity->validate()
class EntityValidator implements EntityValidatorInterface() {
}
// Validator, acts as the glue between constraints, violations and TypedData
// Maybe something similar can be done between constraints, violations and FAPI element
// Uses info from getPropertyDefinitions and actual data
class TypedDataValidator implements TypedDataValidatorInterface() {
protected $data = array();
protected $violations = array();
public function __construct(\Drupal\Core\TypedData\TypedDataInterface $data) {
$this->data = $data;
}
public function validate() {
// loop constraints
foreach ($this->data['constraints'] as $constraint => $info) {
// get from plugin
$check = drupal_container()->get('constraintFactory')($constraint, $configuration);
if (!check->validate($data)) {
// or use one generic class
$this->violations[] = new drupal_container()->get('violationFactory')($constraint, $this->data->value, $data, $property_path);
}
}
return $this->violations;
}
}
// Maybe add this to support the == NULL case
class IntegerConstraintTypedData extends IntegerConstraint {
public function validate() {
if (is_null($value)) {
return TRUE;
}
return parent::validate();
}
}
// Constraint - drupal agnostic
// Only returns true or false, so it can be used from custom code as well.
class IntegerConstraint implements ConstraintInterface {
protected $info;
protected $value;
public function __construct($value, $info) {
$this->info = $info;
$this->value = $value;
}
public function validate() {
if (is_null($value)) { // @see IntegerConstraintTypedData
return TRUE;
}
elseif (!is_numeric($value)) {
return FALSE;
}
return TRUE;
}
}
// Violation - drupal specific, can use t()
class TypeViolation implements ViolationInterface {
protected $message = '';
public function __construct($constraint, $check, $data) {
$message = t('Wrong data type for %label', array(
'%label' => $data->label;
))
}
public function getMessage() {
}
public function getConstraint() {
}
// ...
}
// For form api not tight to an entity, use #constraint
?>
#27
#28
working patch
#29
NR only for the bot
#30
The last submitted patch, i1696648-29.patch, failed testing.
#31
#29: i1696648-29.patch queued for re-testing.
#32
bot failing on shortcut :/
#33
The plugins are moved from system to core/lib/Drupal/Core/Plugin/Validation/Constraint/
#34
Back to UnitTesting
#35
+++ b/core/lib/Drupal/Core/Validation/Constraint/test.phpis that supposed to be in there? there are dpm in the file.
#36
Removed test.php and fixed some whitespace. What follow up issues are associated with this?
#37
FYI: I created #1831154: Allow the AnnotatedClassDiscovery to use custom locations to solve the plugin discovery
#38
#36 test has to go, but it is the only way to test the container for now.
About follow-ups (non created yet AFAIK):
#39
new patch, including new discovery so it can be tested.
This not a working version, if you can/want to help let me know
#40
The last submitted patch, i1696648-39.patch, failed testing.
#41
one for the bot
#42
The last submitted patch, i1696648-41.patch, failed testing.
#43
#44
#45
The last submitted patch, i1696648-44.patch, failed testing.
#46
Added mechanism to detect mode specific constraint based on TypedData->type
Strings aren't translated anymore
#47
The last submitted patch, i1696648-46.patch, failed testing.
#48
+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined@@ -413,4 +413,33 @@ public function __clone() {
+ // Loop over all properties in all languages.
+ foreach ($this->getProperties() as $name => $properties) {
+ foreach ($this->getTranslationLanguages(TRUE) as $langcode => $language) {
+ foreach ($this->getTranslation($langcode) as $name => $property) {
+ $violations += $property->validate();
+ }
+ }
I think $name in innermost loop needs to be reamed so as not to conflict with $name in outer most loop.
#49
#48 thanks
#50
Patch now reads constraints passed to field_create_instance
Lots of bug fixes
Patch is for bot, will probably break entity_test related tests
Architectural reviews appreciated, the idea is to change the formcontroller so field constraints are automatically applied (like RequiredConstraint => #required)
#51
#52
The last submitted patch, i1696648-50.patch, failed testing.
#53
New patch adds:
Field level constraints
Binding to form in the build phase, so if the field is marked required, we add '#required' to the form
#54
#55
The last submitted patch, i1696648-53.patch, failed testing.
#56
#53: i1696648-53.patch queued for re-testing.
#57
#56 no need to bother the bot, this will fail since i hijacked the entity_test form.
#58
The last submitted patch, i1696648-53.patch, failed testing.
#59
form elements are altered if there are constraints
RequiredConstraint sets '#required'
IntegerMinValue set '#type' to number and adds '#min'
#60
Rough technical overview of how it is implemented right now
Constraints can be defined on 3 levels:
entity- field [0..n]
- typedData [1..n]
- property [1..n]
- typedData [1..n]
FormController->build() calls entity->getConstraintsObjects()
- gets a list of constraints keyed by field/property name
Each constraint has a method convertToFormAPI() which returns an array containing keyed data compatibnle with the form API
ex:
return array('#type' => 'number',
'#min' => current($this->settings),
);
FormController->validate() calls $entity->validate
Constraints uses the Plugin system for it's definition
ex:
* @Plugin(* id = "min",
* label = @Translation("MinValue"),
* message = @Translation("The value of %label has to be greater than %min.")
The message is added to the plugin system so it gets detected by l.d.o.
The Constraint base class has a method addMessageArguments so it can prefill some placeholders
ex:
<?php$this->addMessageArguments('%min', current($this->settings));
?>
The FormController calls uses getMessage, getMessageArguments, getObjectLabel to build the (translated) message.
ex:
<?phpform_set_error('error',
t($violation->getMessage(),
$violation->getMessageArguments() + array('%label' => $violation->getObjectLabel()))
);
?>
Constraints:
Indented to show parent - child relationship.
The factory used to choose the most appropriate Constraint accepts an object as parameter, this is either a TypedData, a Field or an Entity. The factory looks at the type of the object ('integer, 'field', ...) and uses the most specific class it can find.
Constraint.phpEntityTypeConstraint.php
MinValueConstraint.php
MinValueIntegerConstraint.php
NotNullConstraint.php
RequiredConstraint.php
RequiredFieldConstraint.php
Follow up issues:
#61
Constraints are now recognized on entities (using the annotation)
#62
The last submitted patch, i1696648-61.patch, failed testing.
#63
This is how it might look on field settings forms
#64
attiks walked me through the Constraint system and it looks really solid so far. Can't wait to see the clientside validation when that is finished.
#65
One for the testbot
#66
FYI... #65 includes lots of whitespace at end of lines as well as a number of missing blank lines at end of files.
#67
#66 this is just to see how much is breaking, last test patch almost broke everything, cleanup, docs, test is for the next couple of days.
#68
Understood @attkis. I just thought you might want to update your editor settings.
#69
#68 I had it turned of for patch rerolls to not touch things by accidents.
#70
code and comment cleanup
#71
missed some parts
#72
The last submitted patch, i1696648-71.patch, failed testing.
#73
Default values added to plugin definition
Context added for translation
Readme added
#74
The last submitted patch, i1696648-73.patch, failed testing.
#75
#73: i1696648-73.patch queued for re-testing.
#76
The last submitted patch, i1696648-73.patch, failed testing.
#77
#73: i1696648-73.patch queued for re-testing.
#78
The last submitted patch, i1696648-73.patch, failed testing.
#79
Constraint plugins moved to core\lib\Drupal\Core\Plugin, #1831154: Allow the AnnotatedClassDiscovery to use custom locations and #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory for background discussion.
#80
We are actually back at considering symfony validator now. I talked to Bernhard Schussek (bschussek) recently, who was so nice to guide us through the validator component and even helps us solving our issues with it, such that we can use it.
We (bschussek, attiks and me) just had a skype call, where we discussed all the details. You can find some notes of our discussion here.
Right now, the biggest problem is figuring out how we can bypass the mentioned metadata problem as our metadata does not map to classes. bschussek is so kind to look into that for us right now. Thus, we'd have to go with a dev version for while, until this makes it into the next release.
Short summary of the things we figured out:
Advantages of going with symfony validation:
- Avoid re-inventing the wheel, but collaborate
- Most needed constraints already exist, so we can reuse them.
- Sophisticated features that are solved for us, e.g. like validation groups or the ability to define constraints that apply to the first field item only, or to the first field items date column.
#81
@fago Excellent news, I'm happy to read this!
#82
I created #1845546: Implement validation for the TypedData API, because this is no longer only for entities, since there are only 10 days left, we need to figure out what needs to be done before feature freeze and what can be done later.
#83
We clearly can't ship core with web services, but no validation API, so I think this is a critical task.
#84
#83 most of the work will happen in #1845546: Implement validation for the TypedData API, this issue is a subset of the overall problem.
#85
Is it worth rerolling #79 (which does not apply now)
sounds like a new approach is needed since #1845546: Implement validation for the TypedData API.
should this wait on that?
if not, what detail can be given about what needs to be done here?
Most likely the issue summary needs to be updated with the new plan.
#86
I think it makes sense to postpone this on #1845546: Implement validation for the TypedData API, and raise that one to critical. Doing so now. Please correct me if I'm wrong on that.
#87
#1845546: Implement validation for the TypedData API landed, so unpostponing.
I think we should do #1868004: Improve the TypedData API usage of EntityNG what gives us $entity->validate(). Then, we'd just need to integrate this with forms in the form controller here.
#88
Is there issue to use introduced validation for fields?
#89
No, not yet. Feel free to go ahead and create one. We need to have all fieldable entities converted over, then we go ahead and migrate existing validation routines over. The only remaining conversion is #1818570: Convert users to the new Entity Field API.
#90
#91
Ok, let's use this issue as META issue to track further conversion sub-issues. Tag: Entity validation
#92
Don't forget the tags :)
#93
Thanks!