Updated: Comment #98
This is a meta issue to track progress of the core validation API and its application to entities and fields.
Problem / Motivation
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.
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 a validation API based on Symfony validators: done in #2002152: Implement entity validation based on symfony validator.
- Implement the validation constraints for all core entity types and their base fields while leaving the form validation callbacks/methods as is.
- Replace form-specific validation code with calls to the validation API and map back violation messages to the corresponding form elements, see #2002180: Entity forms skip validation of fields that are edited without widgets.
User interface changes
None.
API changes
Minimal. We are only moving the logic in form validation callbacks/methods to constraint and validator classes that can be reused outside of forms.
Sub-issues
#2002152: Implement entity validation based on symfony validator
#2002156: Create validation constraints on 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
#1969728: Implement Field API "field types" as TypedData Plugins
#2002180: Entity forms skip validation of fields that are edited without widgets
#1758622: Provide the options list of an entity field (for validating allowed values)
#2403485: Complete conversion of comment form validation to entity validation
#2405943: User entity validation misses form validation logic
#2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay
#2403817: Feed entity validation misses form validation logic
#2403823: Menu link content entity validation misses form validation logic
#2403847: Shortcut content entity validation misses form validation logic
#2406103: Remove hook_node_validate() and hook_node_submit() because they bypass the entity API
#2418119: REST user updates bypass tightened user account change validation
Comment | File | Size | Author |
---|---|---|---|
#79 | i1696648-79.patch | 60.62 KB | attiks |
Comments
Comment #1
Wim LeersJust 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.
Comment #2
fagoExactly.
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.
Comment #3
Wim Leers#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 :)
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedTo 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 ?
Comment #5
pounardHaving 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.
Comment #6
pounardThe Symfony\Validator component seems to be where to start: see https://github.com/symfony/Validator
Comment #7
fagoYes, entity validation would be incorporated in form validation though. That way we have a central place for validations.
Agreed.
Comment #8
Wim Leerspounard++. 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.
Comment #9
pounardTranslation 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...).
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedValidator 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 ?
Comment #11
g089h515r806 CreditAttribution: g089h515r806 commentedMaybe 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.
Comment #12
fagoYes, 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.
Comment #13
Wim Leers#12: Yes, it's something to just keep in mind, to make sure it remains possible. We shouldn't do this (yet) in core.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedAnalyzing 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!
Comment #15
pounardFor 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.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commented@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.
Comment #17
fagoad #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)
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commented@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! :)
Comment #19
fagoWe'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).
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commented@fago: Where are currently those "property definitions" in 8.x ? Is this related to http://drupal.org/node/1346204 ?
Comment #21
plachAdded 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()
.Comment #22
plachnow tagging for real
Comment #23
sunYou 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 ;))
Comment #24
fagook, 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.
Comment #25
fagoSo 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
Then have classes like
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.
Comment #26
attiks CreditAttribution: attiks commentedmore discussion:
Comment #27
attiks CreditAttribution: attiks commentedComment #28
attiks CreditAttribution: attiks commentedworking patch
Comment #29
attiks CreditAttribution: attiks commentedNR only for the bot
Comment #31
attiks CreditAttribution: attiks commented#29: i1696648-29.patch queued for re-testing.
Comment #32
attiks CreditAttribution: attiks commentedbot failing on shortcut :/
Comment #33
attiks CreditAttribution: attiks commentedThe plugins are moved from system to core/lib/Drupal/Core/Plugin/Validation/Constraint/
Comment #34
attiks CreditAttribution: attiks commentedBack to UnitTesting
Comment #35
nod_+++ b/core/lib/Drupal/Core/Validation/Constraint/test.php
is that supposed to be in there? there are dpm in the file.
Comment #36
RobLoachRemoved test.php and fixed some whitespace. What follow up issues are associated with this?
Comment #37
attiks CreditAttribution: attiks commentedFYI: I created #1831154: Allow the AnnotatedClassDiscovery to use custom locations to solve the plugin discovery
Comment #38
attiks CreditAttribution: attiks commented#36 test has to go, but it is the only way to test the container for now.
About follow-ups (non created yet AFAIK):
Comment #39
attiks CreditAttribution: attiks commentednew patch, including new discovery so it can be tested.
This not a working version, if you can/want to help let me know
Comment #41
attiks CreditAttribution: attiks commentedone for the bot
Comment #43
attiks CreditAttribution: attiks commentedComment #44
attiks CreditAttribution: attiks commentedComment #46
attiks CreditAttribution: attiks commentedAdded mechanism to detect mode specific constraint based on TypedData->type
Strings aren't translated anymore
Comment #48
Lars Toomre CreditAttribution: Lars Toomre commentedI think $name in innermost loop needs to be reamed so as not to conflict with $name in outer most loop.
Comment #49
attiks CreditAttribution: attiks commented#48 thanks
Comment #50
attiks CreditAttribution: attiks commentedPatch 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)
Comment #51
attiks CreditAttribution: attiks commentedComment #53
attiks CreditAttribution: attiks commentedNew 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
Comment #54
Fabianx CreditAttribution: Fabianx commentedComment #56
Fabianx CreditAttribution: Fabianx commented#53: i1696648-53.patch queued for re-testing.
Comment #57
attiks CreditAttribution: attiks commented#56 no need to bother the bot, this will fail since i hijacked the entity_test form.
Comment #59
attiks CreditAttribution: attiks commentedform elements are altered if there are constraints
RequiredConstraint sets '#required'
IntegerMinValue set '#type' to number and adds '#min'
Comment #60
attiks CreditAttribution: attiks commentedRough technical overview of how it is implemented right now
Constraints can be defined on 3 levels:
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:
FormController->validate() calls $entity->validate
Constraints uses the Plugin system for it's definition
ex:
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:
The FormController calls uses getMessage, getMessageArguments, getObjectLabel to build the (translated) message.
ex:
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.
Follow up issues:
Comment #61
attiks CreditAttribution: attiks commentedConstraints are now recognized on entities (using the annotation)
Comment #63
attiks CreditAttribution: attiks commentedThis is how it might look on field settings forms
Comment #64
moshe weitzman CreditAttribution: moshe weitzman commentedattiks 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.
Comment #65
attiks CreditAttribution: attiks commentedOne for the testbot
Comment #66
Lars Toomre CreditAttribution: Lars Toomre commentedFYI... #65 includes lots of whitespace at end of lines as well as a number of missing blank lines at end of files.
Comment #67
attiks CreditAttribution: attiks commented#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.
Comment #68
Lars Toomre CreditAttribution: Lars Toomre commentedUnderstood @attkis. I just thought you might want to update your editor settings.
Comment #69
attiks CreditAttribution: attiks commented#68 I had it turned of for patch rerolls to not touch things by accidents.
Comment #70
attiks CreditAttribution: attiks commentedcode and comment cleanup
Comment #71
attiks CreditAttribution: attiks commentedmissed some parts
Comment #73
attiks CreditAttribution: attiks commentedDefault values added to plugin definition
Context added for translation
Readme added
Comment #75
attiks CreditAttribution: attiks commented#73: i1696648-73.patch queued for re-testing.
Comment #77
attiks CreditAttribution: attiks commented#73: i1696648-73.patch queued for re-testing.
Comment #79
attiks CreditAttribution: attiks commentedConstraint 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.
Comment #80
fagoWe 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.
Comment #81
pounard@fago Excellent news, I'm happy to read this!
Comment #82
attiks CreditAttribution: attiks commentedI 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.
Comment #83
effulgentsia CreditAttribution: effulgentsia commentedWe clearly can't ship core with web services, but no validation API, so I think this is a critical task.
Comment #84
attiks CreditAttribution: attiks commented#83 most of the work will happen in #1845546: Implement validation for the TypedData API, this issue is a subset of the overall problem.
Comment #85
YesCT CreditAttribution: YesCT commentedIs 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.
Comment #86
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #87
fago#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.
Comment #88
andypostIs there issue to use introduced validation for fields?
Comment #89
fagoNo, 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.
Comment #90
attiks CreditAttribution: attiks commentedComment #91
fagoOk, let's use this issue as META issue to track further conversion sub-issues. Tag: Entity validation
Comment #92
BerdirDon't forget the tags :)
Comment #93
fagoThanks!
Comment #93.0
fagoUpdated issue summary.
Comment #94
moshe weitzman CreditAttribution: moshe weitzman commentedI see that #2002152: Implement entity validation based on symfony validator is committed. I guess that the next step is from the Summary is for folks to tackle the "Convert form validation of X to entity validation" issues? Is anyone working on those?
Comment #94.0
moshe weitzman CreditAttribution: moshe weitzman commentedadded allowed values validation issue
Comment #95
fagoRight now the focus is on #1969728: Implement Field API "field types" as TypedData Plugins, which converts field validation over to the new validation API. Afterwards I think we should tackle #2002180: Entity forms skip validation of fields that are edited without widgets and #2002180: Entity forms skip validation of fields that are edited without widgets to everything needed in place + work on the conversion issues in parallel.
Comment #96
effulgentsia CreditAttribution: effulgentsia commented#95 lists the same issue twice. Was a different link intended there for one of them?
Comment #97
xjmSo, this is release blocking, but the API impact of the issues required for this is not clear. Let's update the issue summary here (and in child issues) with the specific API changes that are necessary to make this happen, so that core maintainers can review and sign off on them. @Berdir mentioned that @klausi might be able to help with this information.
Comment #97.0
xjmchanged field validation subissue per new plan
Comment #98
klausiUpdated the issue summary.
I'm not sure why this is release blocking - we have the validation API in place now and we even apply it to configurable fields (introduced in #1969728: Implement Field API "field types" as TypedData Plugins). So while it would be ugly and inconsistent to release D8 like this there is room for a contributed module to complete the validation API constraints and their (form) integration.
Do others agree that we could demote this to major?
Comment #99
g089h515r806 CreditAttribution: g089h515r806 commented@klausi,
How to programmatically add "property_constraints" to existing field instance before validate?
Comment #99.0
g089h515r806 CreditAttribution: g089h515r806 commentedupdate for remaining tasks
Comment #100
effulgentsia CreditAttribution: effulgentsia commentedOpened #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs …, so retitling this one.
Comment #101
effulgentsia CreditAttribution: effulgentsia commentedRemoved #2002174: Allow vocabularies to be validated via the API, not just during form submissions from the issue summary. Am about to reparent that one to the config issue meta.
Comment #102
catch#2002158: Convert form validation of comments to entity validation and #2002180: Entity forms skip validation of fields that are edited without widgets are both independently critical. Since that's all that's left here, closing this as duplicate. If I've got that wrong, please re-open with an updated issue summary.
Comment #103
fagoBy looking at all the overrides from ContentEntityForm::validate() I figured this is not fixed: We have some custom form validation logic left, which needs to be converted to entity validation. I opened issues for that and added them to the summary. As they are required to fix this critical, I categorized them as critical as well.
Comment #104
fagoComment #105
fagoFigured #2227381: Apply formatters and widgets to User base fields 'name' and 'email' does not cover user signature validation, so if we do that we still need to do #1548204: Remove user signature and move it to contrib as well, or just fix its validation in another issue. Thus, created one issue for addressing user validation and pointed out there that fixing the two others would be one possible way to fix it: #2405943: User entity validation misses form validation logic.
Comment #106
fagoFound another problem: hook_node_validate() -> Opened #2406103: Remove hook_node_validate() and hook_node_submit() because they bypass the entity API and the related but uncritical #2406107: hook_node_submit() is depcrecated and should be removed
Also opened an issue for improving docs: #2406113: Clarify how ContentEntityForm::validate() should be overridden
Comment #107
alexpottDiscussed with @xjm, @catch, @webchick and @effulgentsia. Without this issue being resolved validation of entities created or updated via REST will not have the same validation as entities created through forms - this is critical.
Comment #108
andypostAdded #2413769: Prevent the deletion of a bundle entity if there are existing content entities of that bundle
Config entities as bundles needs extend their permissions about delete
Comment #109
fagoAdded spin-off issue from user validation.
Comment #110
larowlanWe're down to three issues here, two of which are critical - if we resolve the third - we can close the meta right?
Comment #111
larowlanAdded #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay to IS
Comment #112
larowlanSo the only non critical sub-issue of this one is #2413769: Prevent the deletion of a bundle entity if there are existing content entities of that bundle - my comment from there was
So on that basis I don't think it is valid sub-issue of this one - and on that basis - I think we can close this as done - thoughts?
Comment #113
xjmI'd prefer to keep metas open while they have children for tracking the work, but if we're sure we've identified everything in the scope (i.e. all the sub-issues are filed) and the other two are critical on their own, we can demote this to major. So if anyone can confirm that once the two issues @larowlan mentions are done, this meta is complete?
Comment #114
fagoYep, I confirm this all that's left. I'd rather close it instead of demoting it, but I leave that to you :)
Agreed, removed from summary.
Comment #115
larowlanSounds like a plan, one more down :)
Comment #116
BerdirWe're done here.