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

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

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.

fago’s picture

Just to be clear: this applies to fields/properties that are part of the entity, right?

Exactly.

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"?

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.

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.

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.

Wim Leers’s picture

#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 :)

Anonymous’s picture

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 ?

pounard’s picture

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.

pounard’s picture

The Symfony\Validator component seems to be where to start: see https://github.com/symfony/Validator

fago’s picture

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 ?

Yes, entity validation would be incorporated in form validation though. That way we have a central place for validations.

I'm all in for such abstraction, and you should introspect what Symfony 2 offers in this area and extend it if possible.

Agreed.

Wim Leers’s picture

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.

pounard’s picture

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...).

Anonymous’s picture

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 ?

g089h515r806’s picture

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.

fago’s picture

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.

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.

Wim Leers’s picture

#12: Yes, it's something to just keep in mind, to make sure it remains possible. We shouldn't do this (yet) in core.

Anonymous’s picture

Analyzing current code at 8.x, and what to do (where to start, etc)

  1. Current validations done at the form level (on form elements). An invalid value, reaches the edited entity property at all?
  2. If it doesnt, then it will change if using validator: form API should just pass the values to the entity instance, and then we would call validator to check for errors.
  3. I dont know if using validator we can maintain the pointing of the offending elements after a validation fails.

Would be nice having somebody to help me with this? Maybe a sanbox to work coop on it.

Thanks!

pounard’s picture

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.

Anonymous’s picture

@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.

  1. Where do we should define the validator constraints on the entities? Are we going with the PHP constraint syntax, or YAML? Or...
fago’s picture

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)

Anonymous’s picture

@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! :)

fago’s picture

where will the constraints for the Validator be defined? will it be PHP ? will it be YAML?

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()).

Also, if FAPI will not change 'at all', i dont see the point on this issue at all! :)

The point is to decouple entity validation from entity-forms. Still, we need an API for validating forms that works independently from entities (=FAPI).

Anonymous’s picture

@fago: Where are currently those "property definitions" in 8.x ? Is this related to http://drupal.org/node/1346204 ?

plach’s picture

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() into EntityFormController::validate().

plach’s picture

Issue tags: +Entity forms

now tagging for real

sun’s picture

we might want to move form_state_values_clean() into EntityFormController::validate()

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 ;))

fago’s picture

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:

  • Violation messages need to be translated with Drupal's translation system. Should be solvable.
  • The API makes use of metadata-loaders that have to load metadata (=constraints) for a given class. This is a dealbreaker for us, as we cannot derive the metadata from the class - we derive the class from the metadata. Passing TypedData objects to the Validator would work, but the metadata loader would still receive the $class name only (so is the interface).

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.

fago’s picture

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

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.

attiks’s picture

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,
  ),
);

// 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


attiks’s picture

Assigned: Unassigned » attiks
attiks’s picture

FileSize
8.5 KB

working patch

attiks’s picture

Status: Active » Needs review
FileSize
9.19 KB

NR only for the bot

Status: Needs review » Needs work
Issue tags: -Entity forms

The last submitted patch, i1696648-29.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
Issue tags: +Entity forms

#29: i1696648-29.patch queued for re-testing.

attiks’s picture

bot failing on shortcut :/

attiks’s picture

FileSize
1.59 KB
9.14 KB

The plugins are moved from system to core/lib/Drupal/Core/Plugin/Validation/Constraint/

attiks’s picture

FileSize
1.19 KB
9.12 KB

Back to UnitTesting

nod_’s picture

+++ b/core/lib/Drupal/Core/Validation/Constraint/test.php

is that supposed to be in there? there are dpm in the file.

RobLoach’s picture

FileSize
8.57 KB

Removed test.php and fixed some whitespace. What follow up issues are associated with this?

attiks’s picture

FYI: I created #1831154: Allow the AnnotatedClassDiscovery to use custom locations to solve the plugin discovery

attiks’s picture

#36 test has to go, but it is the only way to test the container for now.

About follow-ups (non created yet AFAIK):

  1. Write Constraints for required, length, min/max, file extension, range, ....
  2. Write the validator implementatio for all typedData, fields and entities
  3. Write a validator usable by EntityFormController
  4. Remove all custom validation from hook_form_validate that can be handled by entity validation
  5. Implement clientside validation, the idea is to allow constraints to define a javascript definition as well, so we only need a system that collects all constraint and tie them to the right form element
  6. Implement validation for the form api, so it can be used without an entity
  7. And probably more
attiks’s picture

FileSize
14.3 KB

new patch, including new discovery so it can be tested.

This not a working version, if you can/want to help let me know

Status: Needs review » Needs work

The last submitted patch, i1696648-39.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
22.48 KB

one for the bot

Status: Needs review » Needs work

The last submitted patch, i1696648-41.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
22.47 KB
attiks’s picture

FileSize
22.61 KB

Status: Needs review » Needs work

The last submitted patch, i1696648-44.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
32.02 KB

Added mechanism to detect mode specific constraint based on TypedData->type
Strings aren't translated anymore

Status: Needs review » Needs work

The last submitted patch, i1696648-46.patch, failed testing.

Lars Toomre’s picture

+++ 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.

attiks’s picture

#48 thanks

attiks’s picture

FileSize
40.32 KB

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)

attiks’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, i1696648-50.patch, failed testing.

attiks’s picture

FileSize
45.68 KB

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

Fabianx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Entity forms

The last submitted patch, i1696648-53.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
Issue tags: +Entity forms

#53: i1696648-53.patch queued for re-testing.

attiks’s picture

#56 no need to bother the bot, this will fail since i hijacked the entity_test form.

Status: Needs review » Needs work

The last submitted patch, i1696648-53.patch, failed testing.

attiks’s picture

FileSize
49.9 KB

form elements are altered if there are constraints
RequiredConstraint sets '#required'
IntegerMinValue set '#type' to number and adds '#min'

attiks’s picture

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

  • this loops over all fields and typedData, calling validate() on each
  • validate() will get the constraints and validates the value/field
  • all violations are collected and returned to the parent
  • FormController uses set_form_error for each violation

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:

$this->addMessageArguments('%min', current($this->settings));

The FormController calls uses getMessage, getMessageArguments, getObjectLabel to build the (translated) message.
ex:

form_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.php
  EntityTypeConstraint.php
  MinValueConstraint.php
    MinValueIntegerConstraint.php
  NotNullConstraint.php
  RequiredConstraint.php
    RequiredFieldConstraint.php

Follow up issues:

attiks’s picture

Status: Needs work » Needs review
FileSize
4.24 KB
53 KB

Constraints are now recognized on entities (using the annotation)

Status: Needs review » Needs work

The last submitted patch, i1696648-61.patch, failed testing.

attiks’s picture

This is how it might look on field settings forms
i1696648-field_instance_settings.png

moshe weitzman’s picture

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.

attiks’s picture

Status: Needs work » Needs review
FileSize
54.61 KB

One for the testbot

Lars Toomre’s picture

FYI... #65 includes lots of whitespace at end of lines as well as a number of missing blank lines at end of files.

attiks’s picture

#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.

Lars Toomre’s picture

Understood @attkis. I just thought you might want to update your editor settings.

attiks’s picture

#68 I had it turned of for patch rerolls to not touch things by accidents.

attiks’s picture

FileSize
78.55 KB

code and comment cleanup

attiks’s picture

FileSize
54.99 KB

missed some parts

Status: Needs review » Needs work

The last submitted patch, i1696648-71.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
19.03 KB
64.09 KB

Default values added to plugin definition
Context added for translation
Readme added

Status: Needs review » Needs work
Issue tags: -Entity forms

The last submitted patch, i1696648-73.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review

#73: i1696648-73.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, i1696648-73.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review

#73: i1696648-73.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity forms

The last submitted patch, i1696648-73.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
19.03 KB
60.62 KB
fago’s picture

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:

  • Symfony constraints miss discovery / a registry. We probably want to use our plugin system for that + just add in all provided symfony constraints by default (they are ale below a certain namespace)
  • This would allow us to stay with an array based constraint configuration, what we believe has better DX as it avoids having to know the class locations and importing all necessary namespaces....
  • For translating we'd have to map their replacement arguments from the {{ arg }} notation to ours + find a way to parse constraint messages. That could work via the annotationreader and piggy-backing on the convention that messages are defined in class properties with a 'message' suffix.

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.

pounard’s picture

@fago Excellent news, I'm happy to read this!

attiks’s picture

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.

effulgentsia’s picture

Priority: Normal » Critical
Issue tags: +WSCCI

We clearly can't ship core with web services, but no validation API, so I think this is a critical task.

attiks’s picture

Issue tags: -WSCCI

#83 most of the work will happen in #1845546: Implement validation for the TypedData API, this issue is a subset of the overall problem.

YesCT’s picture

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.

effulgentsia’s picture

Status: Needs review » Postponed

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.

fago’s picture

Status: Postponed » Needs work

#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.

andypost’s picture

Is there issue to use introduced validation for fields?

fago’s picture

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.

attiks’s picture

Assigned: attiks » Unassigned
fago’s picture

Title: Untie entity validation from form validation » [META] Untie entity validation from form validation

Ok, let's use this issue as META issue to track further conversion sub-issues. Tag: Entity validation

Berdir’s picture

Don't forget the tags :)

fago’s picture

Thanks!

fago’s picture

Issue summary: View changes

Updated issue summary.

moshe weitzman’s picture

I 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?

moshe weitzman’s picture

Issue summary: View changes

added allowed values validation issue

fago’s picture

Right 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.

effulgentsia’s picture

#95 lists the same issue twice. Was a different link intended there for one of them?

xjm’s picture

So, 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.

xjm’s picture

Issue summary: View changes

changed field validation subissue per new plan

klausi’s picture

Status: Needs work » Active
Issue tags: -Needs issue summary update

Updated 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?

g089h515r806’s picture

@klausi,
How to programmatically add "property_constraints" to existing field instance before validate?

g089h515r806’s picture

Issue summary: View changes

update for remaining tasks

effulgentsia’s picture

Title: [META] Untie entity validation from form validation » [META] Untie content entity validation from form validation
Issue summary: View changes
effulgentsia’s picture

Issue summary: View changes

Removed #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.

catch’s picture

Status: Active » Closed (duplicate)

#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.

fago’s picture

Issue summary: View changes

By 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.

fago’s picture

Status: Closed (duplicate) » Active
fago’s picture

Issue summary: View changes

Figured #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.

fago’s picture

alexpott’s picture

Issue tags: +Triaged D8 critical

Discussed 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.

andypost’s picture

fago’s picture

Issue summary: View changes

Added spin-off issue from user validation.

larowlan’s picture

We're down to three issues here, two of which are critical - if we resolve the third - we can close the meta right?

larowlan’s picture

larowlan’s picture

So 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

We are talking about config entities here.
How else can you delete them other than via the form?
We don't babysit bad use of the API and as far as I know - REST doesn't allow deleting config entities.
So we're really talking about CMI dependencies here, the config entities depend on the content ones.
On that basis I don't think this pertains to entity validation either.

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?

xjm’s picture

I'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?

fago’s picture

Issue summary: View changes

Yep, I confirm this all that's left. I'd rather close it instead of demoting it, but I leave that to you :)

So on that basis I don't think it is valid sub-issue of this one

Agreed, removed from summary.

larowlan’s picture

Priority: Critical » Major
Issue tags: -Triaged D8 critical

Sounds like a plan, one more down :)

Berdir’s picture

Category: Task » Plan
Status: Active » Fixed

We're done here.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.