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: [PP-2] 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

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

javier.alejandro.castro’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...).

javier.alejandro.castro’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.

javier.alejandro.castro’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.

javier.alejandro.castro’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)

    javier.alejandro.castro’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).

    javier.alejandro.castro’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

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

    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,
      ),
    );
    <?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
    ?>
    attiks’s picture

    Assigned:Unassigned» attiks
    attiks’s picture

    StatusFileSize
    new8.5 KB
    PASSED: [[SimpleTest]]: [MySQL] 46,834 pass(es).
    [ View ]

    working patch

    attiks’s picture

    Status:Active» Needs review
    StatusFileSize
    new9.19 KB
    PASSED: [[SimpleTest]]: [MySQL] 46,881 pass(es).
    [ View ]

    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

    StatusFileSize
    new1.59 KB
    new9.14 KB
    PASSED: [[SimpleTest]]: [MySQL] 46,875 pass(es).
    [ View ]

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

    attiks’s picture

    StatusFileSize
    new1.19 KB
    new9.12 KB
    PASSED: [[SimpleTest]]: [MySQL] 46,864 pass(es).
    [ View ]

    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

    StatusFileSize
    new8.57 KB
    PASSED: [[SimpleTest]]: [MySQL] 46,867 pass(es).
    [ View ]

    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

    StatusFileSize
    new14.3 KB
    FAILED: [[SimpleTest]]: [MySQL] 46,611 pass(es), 374 fail(s), and 111 exception(s).
    [ View ]

    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
    StatusFileSize
    new22.48 KB
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1696648-41.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]

    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
    StatusFileSize
    new22.47 KB
    FAILED: [[SimpleTest]]: [MySQL] 47,580 pass(es), 36 fail(s), and 27 exception(s).
    [ View ]
    attiks’s picture

    StatusFileSize
    new22.61 KB
    FAILED: [[SimpleTest]]: [MySQL] 47,613 pass(es), 24 fail(s), and 1 exception(s).
    [ View ]

    Status:Needs review» Needs work

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

    attiks’s picture

    Status:Needs work» Needs review
    StatusFileSize
    new32.02 KB
    FAILED: [[SimpleTest]]: [MySQL] 47,539 pass(es), 24 fail(s), and 1 exception(s).
    [ View ]

    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

    StatusFileSize
    new40.32 KB
    FAILED: [[SimpleTest]]: [MySQL] 47,517 pass(es), 43 fail(s), and 11 exception(s).
    [ View ]

    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

    StatusFileSize
    new45.68 KB
    FAILED: [[SimpleTest]]: [MySQL] 42,439 pass(es), 3,936 fail(s), and 2,806 exception(s).
    [ View ]

    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

    StatusFileSize
    new49.9 KB
    FAILED: [[SimpleTest]]: [MySQL] 42,574 pass(es), 4,016 fail(s), and 2,609 exception(s).
    [ View ]

    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:

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

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

    <?php
    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
    StatusFileSize
    new4.24 KB
    new53 KB
    FAILED: [[SimpleTest]]: [MySQL] 42,559 pass(es), 4,021 fail(s), and 2,609 exception(s).
    [ View ]

    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

    StatusFileSize
    new5.18 KB

    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
    StatusFileSize
    new54.61 KB
    PASSED: [[SimpleTest]]: [MySQL] 47,826 pass(es).
    [ View ]

    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

    StatusFileSize
    new78.55 KB
    FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1696648-70.patch. Unable to apply patch. See the log in the details link for more information.
    [ View ]

    code and comment cleanup

    attiks’s picture

    StatusFileSize
    new54.99 KB
    FAILED: [[SimpleTest]]: [MySQL] 47,865 pass(es), 0 fail(s), and 158 exception(s).
    [ View ]

    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
    StatusFileSize
    new19.03 KB
    new64.09 KB
    FAILED: [[SimpleTest]]: [MySQL] 48,024 pass(es), 0 fail(s), and 3 exception(s).
    [ View ]

    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
    StatusFileSize
    new19.03 KB
    new60.62 KB
    PASSED: [[SimpleTest]]: [MySQL] 48,070 pass(es).
    [ View ]
    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: Convert form validation of vocabularies to config validation 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

    Issue summary:View changes

    Added #2413769: Access to delete comment type should be at entity level
    Config entities as bundles needs extend their permissions about delete

    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: Access to delete comment type should be at entity level - 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 :)