Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Issue tags: +Entity Field API

Tagging

fago’s picture

Assigned: Unassigned » fago
Issue tags: +sprint

Starting with this one.

fago’s picture

Status: Active » Needs review
FileSize
13.81 KB

ok, here is a first working patch which already implements property Entity::validate() support for EntityNG. We'll need to improve FieldItemBase::getConstraints() to add in property_constraints for improved DX. Then complete tests for all constraints.

I made sure to have constraints for all the max-length in hook_schema(). For checking allowed values we'll need #1758622: Provide the options list of an entity field.

fago’s picture

Assigned: fago » Unassigned

un-assigning while not working on it.

Status: Needs review » Needs work
Issue tags: -sprint, -Entity Field API, -Entity validation

The last submitted patch, d8_validate_entity_test.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Entity Field API, +Entity validation

#3: d8_validate_entity_test.patch queued for re-testing.

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.phpundefined
@@ -0,0 +1,104 @@
+      $this->assertValidation($entity_type);
...
+  protected function assertValidation($entity_type) {

I know it's a pattern we started elsewhere but I would suggest to not use a method name that starts with assert because these are by default ignored in the backtrace when reporting the line number of the assertion, so they will all be reported for the line that calls $this->assertValidation().

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.phpundefined
@@ -41,22 +41,43 @@ public function baseFieldDefinitions() {
       'type' => 'string_field',

Just an idea, not to be implemented here: a bundle_field with a setting for the entity type that implements the allowed values stuff when we have it and automatically knows which bundle names are valid.

fago’s picture

Just an idea, not to be implemented here: a bundle_field with a setting for the entity type that implements the allowed values stuff when we have it and automatically knows which bundle names are valid.

Yes, a good idea!

I know it's a pattern we started elsewhere but I would suggest to not use a method name that starts with assert because these are by default ignored in the backtrace when reporting the line number of the assertion, so they will all be reported for the line that calls $this->assertValidation().

I see - fixed.

Also completed the patch and migrated to the property_constraints key for improved DX.

Berdir’s picture

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/EntityTestStorageController.phpundefined
@@ -41,22 +41,33 @@ public function baseFieldDefinitions() {
+      'property_constraints' => array(
+        'value' => array('Length' => array('max' => 12)),

This should be a moved to the language_field I think, then we also get test coverage for that.

fago’s picture

implemented that. We cannot use property_constraints on the type definition level though (as this is not field specific). I guess that would make sense to support with field_type plugins then.

fago’s picture

Another question is whether 12 is actually feasible? I just took that over from the schema.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraintValidator.phpundefined
@@ -0,0 +1,47 @@
+      $this->context->validateValue($property, $constraints, '[' . $name . ']', $group);

As discussed, this looks strange ([/]), should be "."

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.phpundefined
@@ -0,0 +1,112 @@
+   * @return \Drupal\Core\Entity\EntityInterface

Missing a description.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.phpundefined
@@ -0,0 +1,112 @@
+    $test_entity->uuid->value = $this->randomString(150);
+    $this->assertEqual($test_entity->validate()->count(), 1, 'Validation failed.');
...
+    $test_entity->langcode->value = $this->randomString(40);
+    $this->assertEqual($test_entity->validate()->count(), 1, 'Validation failed.');
...
+    $test_entity->name->value = $this->randomString(40);
+    $this->assertEqual($test_entity->validate()->count(), 1, 'Validation failed.');

As discussed, we need some more validation here:

- Correct violation
- Correct property path
- Maybe also the message or so, at least for one of them?

Also, some boundary value tests might make sense, e.g. the exact amount and one above?

And required is also not covered.

klausi’s picture

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraint.php
@@ -0,0 +1,67 @@
+    }
+    parent::__construct($options);
+    $constraint_manager = \Drupal::service('validation.constraint');

You should not get the constraint manager from the global context, it should be injected to the plugin. But I guess that can be a followup issue.

fago’s picture

You should not get the constraint manager from the global context, it should be injected to the plugin. But I guess that can be a followup issue.

Yes, we definitly should. Let's open a follow-up for allowing constraints to have dependencies injected.

fago’s picture

And required is also not covered.

Yep, and it did not work as expected as it has been applied to the field. I've added a temporary fix to have it applied to the field as a whole, but we really need to do #1928938: Improve typed data definitions of lists to clean this up.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, this looks good to go.

The important part here is being able to call validate() an $entity and it validating it based on the defined field definitions, for which this adds the necessary definitions, code and tests.

I think we might need a follow-up or two to add better validation to certain things like language (should implement the allowed values interface once it's in) and entity references should check that the target exists. And I guess we'll have to find a way to deal with auto-create references there. And @yched is working on validation stuff including a backwards compatibility layer for hook_field_validate().

amateescu’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/Field/FieldItemBase.php
@@ -136,4 +137,18 @@ public function onChange($property_name) {
\ No newline at end of file

Missing newline.

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraint.php
@@ -0,0 +1,67 @@
+   * An array of constraints for contained properties, keyed by property name.
+   *
+   * @var string
+   */
+  public $properties;

Which one is it, array or string? :)

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ComplexDataConstraintValidator.php
@@ -0,0 +1,47 @@
+   * {@inheritDoc}

No capital D.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.php
@@ -0,0 +1,126 @@
+ * Definition of Drupal\system\Tests\Entity\EntityValidationTest.

Contains..

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.php
@@ -0,0 +1,126 @@
+class EntityValidationTest extends EntityUnitTestBase  {

Extra space here.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.php
@@ -0,0 +1,126 @@
+    $this->assertEqual($violation->getMessage(), t('This value is too long. It should have %limit characters or less.', array('%limit' => '32')));

We don't use t() in test assertions anymore. You can either use \Drupal\Component\Utility\String::format() or preferably just sprintf().

Other than that, this validation system looks rather complex at first sight, looking forward to see a grokable documentation for it, since #1845546: Implement validation for the TypedData API has no change notice :/

Berdir’s picture

@amateescu. The t() is correct there, it's not an assertion message, it's an assertEqual(), and getMessage() uses t(), so it actual value and expected value should be consistent. It's basically the same as a assertText(t('Something')). The other things are correct, sorry for missing that.

amateescu’s picture

Ok, no reason to hold up the patch on those minor issues then.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, d8_validate_entity_test-19.patch, failed testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Don't mind the testbot, #19 contains no functional changes.

fago’s picture

fago’s picture

fago’s picture

Priority: Normal » Major

Raising priority to major as this issue unlocks getting this implemented for all other entity types, thus is a blocker for moving on with #1696648: [META] Untie content entity validation from form validation.

Status: Reviewed & tested by the community » Needs work
Issue tags: -sprint, -Entity Field API, -Entity validation

The last submitted patch, d8_validate_entity_test-19.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Entity Field API, +Entity validation

#19: d8_validate_entity_test-19.patch queued for re-testing.

fago’s picture

Status: Needs review » Reviewed & tested by the community

tests are green, so back to RTBC

fago’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +sprint, +Entity Field API, +Entity validation

The last submitted patch, d8_validate_entity_test-19.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
18.29 KB

re-rolled patch, no changes.

fago’s picture

Status: Needs review » Reviewed & tested by the community

All green, so back to rtbc

fago’s picture

Title: Implement entity validation for the test entity » Implement entity validation based on symfony validator

Improving issue title: This really lays the foundation for doing entity validation based on symfony validator as it improves how things are integrated. Then this adds tests for that, by impementing it for the test entity. So let's call it that way.

fago’s picture

#31: d8_validate_entity_test.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.phpundefined
@@ -0,0 +1,132 @@
+  protected function checkValidation($entity_type) {
+    $entity = $this->createTestEntity($entity_type);
+    $violations = $entity->validate();
+    $this->assertEqual($violations->count(), 0, 'Validation passes.');
+
+    // Test triggering a fail for each of the constraints specified.
+    $test_entity = clone $entity;
+    $test_entity->uuid->value = $this->randomString(129);
+    $this->assertEqual($test_entity->validate()->count(), 1, 'Validation failed.');
+
+    $test_entity = clone $entity;
+    $test_entity->langcode->value = $this->randomString(13);
+    $this->assertEqual($test_entity->validate()->count(), 1, 'Validation failed.');
+
+    $test_entity = clone $entity;
+    $test_entity->type->value = NULL;
+    $this->assertEqual($test_entity->validate()->count(), 1, 'Validation failed.');
+
+    $test_entity = clone $entity;
+    $test_entity->name->value = $this->randomString(33);
+    $this->assertEqual($test_entity->validate()->count(), 1, 'Validation failed.');
+
+    // Make sure the violation is correct.
+    $violations = $test_entity->validate();
+    $violation = $violations->get(0);
+    $this->assertEqual($violation->getRoot(), $test_entity, 'Violation root is entity.');
+    $this->assertEqual($violation->getPropertyPath(), 'name.0.value', 'Violation property path is correct.');
+
+    $this->assertEqual($violation->getInvalidValue(), $test_entity->name->value, 'Violation contains invalid value.');
+    $this->assertEqual($violation->getMessage(), t('This value is too long. It should have %limit characters or less.', array('%limit' => '32')));
+  }

After some discussion in IRC.. fago said...

ok, so let's be bullet-proof and verify the message for each constraint as well in this issue - so we know for sure the right constraint failed validation

fago’s picture

fago’s picture

Status: Needs work » Needs review
FileSize
18.97 KB
2.87 KB

Updated patch with improved test-coverage to assert the message as well.

Berdir’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.phpundefined
@@ -105,26 +105,34 @@ protected function checkValidation($entity_type) {
+    $this->assertEqual($violations[0]->getMessage(), t('This value should not be null.'));

Unrelated but that looks a german-speaking person defined that message, should be "must not" :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.phpundefined
@@ -0,0 +1,140 @@
+    $violations = $test_entity->validate();
+    $this->assertEqual($violations->count(), 1, 'Validation failed.');
+    $this->assertEqual($violations[0]->getMessage(), t('This value is too long. It should have %limit characters or less.', array('%limit' => '32')));
+
+    // Make sure the information provided by a violation is correct.
+    $violations = $test_entity->validate();
+    $violation = $violations[0];
+    $this->assertEqual($violation->getRoot(), $test_entity, 'Violation root is entity.');
+    $this->assertEqual($violation->getPropertyPath(), 'name.0.value', 'Violation property path is correct.');
+    $this->assertEqual($violation->getInvalidValue(), $test_entity->name->value, 'Violation contains invalid value.');
+    $this->assertEqual($violation->getMessage(), t('This value is too long. It should have %limit characters or less.', array('%limit' => '32')));

The second validate() call seems unnecessary, it's the same message and violation here?

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityValidationTest.phpundefined
@@ -105,26 +105,34 @@ protected function checkValidation($entity_type) {
+    $this->assertEqual($violations[0]->getMessage(), t('This value should not be null.'));

Unrelated but that looks a german-speaking person defined that message, should be "must not" :)

Berdir’s picture

Status: Needs review » Needs work
fago’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
18.78 KB

The second validate() call seems unnecessary, it's the same message and violation here?

Yep, that made more sense when we did not check the message for each constraint. Howsoever, removed the additional call.

Unrelated but that looks a german-speaking person defined that message, should be "must not" :)

Maybe, it's the one provided by symfony validator (and webmozart is German speaking). We probably want to work over those messages in general. Howsoever, that's not introduced as part of this patch.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, I think this looks good now, back to RTBC if bot agrees.

Status: Reviewed & tested by the community » Needs work
Issue tags: -sprint, -Entity Field API, -Entity validation

The last submitted patch, d8_validate.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Entity Field API, +Entity validation

#40: d8_validate.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Testbot file system was full, that's why it failed before. This is still RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 54c2e3d and pushed to 8.x. Thanks!

Berdir’s picture

Title: Implement entity validation based on symfony validator » Change notice: Implement entity validation based on symfony validator
Priority: Major » Critical
Status: Fixed » Active
Issue tags: -sprint +Needs change record

Awesome. I have no clue if we have any change notices for symfony validation already and we certainly need to document the changes that we made here. This will also help with conversions of the other entity types.

@fago: Can you start something here? Do we have a change notice already somewhere that we can extend or should we create a new one anyway?

yched’s picture

Woah, I was not following this issue :-)
Great work ! I'm adapting #1992138: Helper issue for "field types as TypedData Plugins" to work on top of the new ComplexDataConstraint.

I'm still seeing the bug described in #2012682: Constraints in getPropertyDefinitions generate violations without a delta, though.
(FWIW, I also opened those for the bugs I'm seeing in the "field types as TypedData" patch:
#2012662: Constraints on 'target_id' / 'tid' properties break autocomplete if applied
#2012690: [Meta] Add useful information to 'type' constraints error messages)

One thing I'm not too fond of is the fact that we standardize placing constraints in getFieldDefinitions(). This data is persistently and statically cached, this bloats memory with validation info, that is not relevant for 95% requests.
Also, not sure how it will play when we try to move those "field definitions" to the generic FieldDefinitionInterface worked on in #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets ?

fago’s picture

@fago: Can you start something here? Do we have a change notice already somewhere that we can extend or should we create a new one anyway?

We don't have something yet, I'll start with one.

One thing I'm not too fond of is the fact that we standardize placing constraints in getFieldDefinitions(). This data is persistently and statically cached, this bloats memory with validation info, that is not relevant for 95% requests.

As discussed on IRC
- that's only the case for base-fields, for configurable fields we can generate constraints out of the field settings
- there won't be many constraints, as the patch has shown it's usual just one per base field. Some of those we might be able to replace with type-defaults anyway, e.g. for uuids (#1777956: Provide a way to define default values for entity fields adds a uuid entity field)
- having just one constraint for e.g. Length in the definition is not really different to the already common practice of having one field API setting for max-length in the definition

Given that all, I don't think it will be problematic and after all that's the most sane place to put them (DX wise). If it turns out to be problematic we could still work out caching strategies that cache validation constraints separately then.

fago’s picture

Status: Active » Needs review

Created a change notice and some basic documentation:

https://drupal.org/node/2015617
https://drupal.org/node/2015613

Please review.

Berdir’s picture

Title: Change notice: Implement entity validation based on symfony validator » Implement entity validation based on symfony validator
Priority: Critical » Major
Status: Needs review » Fixed

Looks like a good start to me. Let's improve the documentation as we convert more things and start using it for forms.

Wondering if we want to start a list of default validators, what they do and how to configure them, possibly also what types result in what kind of validators (e.g. the language field and the uuid field in the default values issue) ? The linked Symfony documentation is a start, but I assume we'll add more validators and the examples there aren't in the way we specify it for Typed data...

Maybe just create a stub page with a single example and then create a documentation issue for it?

fago’s picture

Sounds good. I've done so and created #2015739: Complete validation API documentation.

Also, I realized we missed documenation for the property_constraint key - so I've added a paragraph about that.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the tag when the change notification task is completed.