Simplify validation constraint implementations for fields

Yep, this makes a lot of sense :)

1. +++ b/core/lib/Drupal/Core/Validation/FieldItemAwareConstraintValidator.php
   @@ -0,0 +1,40 @@
   + * Validates the EntityChanged constraint.
   + */
   +abstract class FieldItemAwareConstraintValidator extends ConstraintValidator {
   

   The one line description must be updated.

2. +++ b/core/lib/Drupal/Core/Validation/FieldItemAwareConstraintValidator.php
   @@ -0,0 +1,40 @@
   +      throw new \UnexpectedValueException('FieldItemAwareConstraintValidator::getFieldItem() called for data that is neither a field item nor a field item property.');
   

   I'm not sure about this one, should exceptions include the fully qualified class name?

Hiding details is good and this patch should continue on. But I don't consider this a complete fix to the problem. This code is a turd, and a turd hiding class like this one isn't a full resolution.

+1 subscribe.

It currently does this with the following nasty code:
$field = $this->context->getMetadata()->getTypedData()->getParent();

True, but I think the DX problem is more caused by the validation constraints being not implement for field items, but field item properties. When the constraint actually deals with the field, it should be implemented on field level. If it deals just with a single value without any additional context, i.e. to check a regex, it fits well on the individual field property value.

Attached patch shows how the user constraint would be implemented on field item level - imho it's not that bad. You just get the field item object passed as value, what should be just fine. The only gotcha I see is that the field item might be NULL if it's empty, but that's the case for all constraint plugins and doing a

if (!isset($value)) {
  return;
}

in the beginning is what 90% of the symfony constraint validators already do also. (It's mostly NULL, Blank constraints that deal with the NULL value then - others should ignore them.)

In regard to the EntityChangedConstraint, I'm not sure we even need that. I'd see EntityChanged to be its own entity field type (not configurable), which
a) can care about setting/update the value as appropriately
b) can implement getConstraint() to return a constraint on the field, comparing it with last changed that of the entity. Thus, we could configure a regular compare-date constraint here and do not even need our own plugin.
Howsoever, that would totally deserve its own issue.

That patch makes a lot of sense, I totally agree that we should operate on the field item level for such context-sensitive constraints.

I stiill see a + $field = $field_item->getParent(); which is undoubtly better but you still have to deal with typed data internals. Or getParent isn't internals?

getParent() is a TypedDataInterface yes, but there will be some more context specific method after #2002138: Use an adapter for supporting typed data on ContentEntities I guess.

That said, $field (which is actually a FieldItemList class) is only used to get the name of the field, wouldn't it be easier to understand if we get that from the field definition? $field_item->getFieldDefinition()->getFieldName() ?

Thanks, I think that is easier to understand now.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserUniqueValidator.php
@@ -22,13 +22,13 @@
+    $field_name = $field_item->getFieldDefinition()->getFieldName(); ¶

Additional whitespace at the end of the line - fixed that.

created #2145103: Provide non-configurable field types for entity created, changed and timestamp fields

Tagging for reroll.

re-rolled

13: d8_field_validation.patch queued for re-testing.

This needs some re-roll / test fixes

Rerolled with @covers / @coversDefaultClass annotations.

Oops, wrong window, sorry, ignore patch above.

Rerolled. For some reason, PHP fatals on my machine leaving no information behind. Let's see how it fares on the bot.

Rerolled without the typo.

I have rerolled the patch, and fixed it. Now should not break the tests during the install.

The main problem with #24 was, that the user name validation wasn't worked with a field instance as it works for email, but this wasn't the only problem which broke the installation: I'm also found an other bug here: #2226811: FieldItemBase type hints DataDefinitionInterface but requires ComplexDataDefinitionInterface

related: #2226821: Combine Drupal Constraint and ConstraintValidation classes into one

This looks great, thanks! Imo, this is RTBC - but we'll need someone else to sign it off as I worked on it.

If this is really close to commit, please include mr.york (https://drupal.org/user/52785) in the commit message, he helped me a lot in debugging.

+++ b/core/modules/user/lib/Drupal/user/Entity/User.php
@@ -473,8 +473,8 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      // No Length constraint here because the UserName constraint also covers

This needs more spaces

Add spaces.

@Désiré I've added commit message to summary

Just a few nitpicks

1. +++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserMailUnique.php
   @@ -14,7 +14,8 @@
   + *   type = { "email" }
   
   +++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserNameConstraint.php
   @@ -14,7 +14,8 @@
   + *   type = { "string" }
   

   is this change described somewhere?
   I mean that I found no docs about "context" in annotation

2. +++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserNameConstraintValidator.php
   @@ -18,11 +18,12 @@ class UserNameConstraintValidator extends ConstraintValidator {
   +  public function validate($field_item, Constraint $constraint) {
   
   +++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserUniqueValidator.php
   @@ -18,20 +18,22 @@ class UserUniqueValidator extends ConstraintValidator {
   +  public function validate($field_item, Constraint $constraint) {
   

   ConstraintValidatorInterface declares $value but $fieldItem makes more sense, but maybe it is a FieldItemList?

This looks ok, but I don't see that it actually accomplishes the change suggested in the summary.

Also, seems the last patch didn't get tested? maybe need to re-post.

I mean that I found no docs about "context" in annotation

I think not, how can I do it?

ConstraintValidatorInterface declares $value but $fieldItem makes more sense, but maybe it is a FieldItemList?

Yes, I'll change this.

@Désiré the only pages I found is https://drupal.org/node/2015613
Also please check the change notices

Renamed the $field_item values to $field_item_list in some validate function.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserNameConstraintValidator.php
@@ -18,11 +18,12 @@ class UserNameConstraintValidator extends ConstraintValidator {
+    $name = $field_item_list->value;

Oh, indeed the constraints are assigned to the item list level now. They could be assigned to the items by using
$field_definition->getItemDefinition()->setConstraints(). That does not seem so nice though, so staying at list level is fine to me.

Given that , we'd better use $list->first()->value then I guess.

Given that , we'd better use $list->first()->value then I guess.

I replaced the relevant code.

Rerolle #41.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserNameConstraint.php
@@ -14,7 +14,8 @@
+ *   type = { "string" }

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserNameUnique.php
@@ -14,7 +14,8 @@
+ *   type = { "string" }

This is wrong now, as you cannot put it on a string property, but only on a string field (the complete list). Unfortunately, we do not have data types per field type so there is no correct type to refer here except for the generic "list" :/

Maybe, we should do
type = {"list"} and
item_type = "field_item:string". I guess this needs more work/fixing, so let's better just keep it removed/missing for now.

Maybe, we should do
type = {"list"} and
item_type = "field_item:string". I guess this needs more work/fixing, so let's better just keep it removed/missing for now.

Removed the " * type = { "string" }" annotation.

46: 2110345-validation_constraints_for_fields-46.patch queued for re-testing.

Either the patch is incomplete or the issue summary is outdated, since a getFieldItem() method is not added

Updated the summary. Patch looks ready to me. Once, we've got the base class filter out NULL values for us we could even add type hinting to our $value parameter, what should help DX a lot. But we'll need #2226821: Combine Drupal Constraint and ConstraintValidation classes into one for that first.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
@@ -18,12 +18,12 @@ class CommentNameConstraintValidator extends ConstraintValidator {
-  public function validate($field_item, Constraint $constraint) {
-    $author_name = $field_item->value;
+  public function validate($field_item_list, Constraint $constraint) {
+    $author_name = $field_item_list->first()->value;

Throughout core, FieldItemList variables tend to be named $items, would be best if we stick to that ?

I've been thinking about that also, but I wasn't sure. But as you think the same way I guess we should then.

-> Let's use $items or $field_items as well.

Renamed the $field_item_list values to $items in relevant validate function.

52: 2110345-validation_constraints_for_fields-52.patch queued for re-testing.

52: 2110345-validation_constraints_for_fields-52.patch queued for re-testing.

52: 2110345-validation_constraints_for_fields-52.patch queued for re-testing.

52: 2110345-validation_constraints_for_fields-52.patch queued for re-testing.

Reroll.

Thanks, re-roll / patch looks good to me.

58: 2110345-validation_constraints_for_fields-58.patch queued for re-testing.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserMailUnique.php
@@ -14,7 +14,8 @@
+ *   type = { "email" }

oh, I just figured that should not be added is the type is wrong. Just keep leaving it out as for the others.

Updating patch with reroll. Please review.

Both the prior state of the UserNameUnique validator and this patched version don't let me check for a unique username in the following way (following the pattern used in User.module::user_validate_name()):

function user_validate_name($name) {
  $definition = FieldDefinition::create('string')
    ->setConstraints(array('UserNameUnique' => array()));
  $data = \Drupal::typedDataManager()->create($definition);
  $data->setValue($name);
  $violations = $data->validate();
  if (count($violations) > 0) {
    return $violations[0]->getMessage();
  }
}

In the case of the patched version I get the error Fatal error: Call to a member function id() on a non-object in /srv/www/drupal8/core/modules/user/lib/Drupal/user/Plugin/Validation/Constraint/UserUniqueValidator.php on line 29.

I'm not sure if this is because I'm using this validator incorrectly, or if this patch doesn't allow using these validators out of the context of a User entity.

62: drupal8-validation_constraints_for_fields-2110345-62.patch queued for re-testing.

62: drupal8-validation_constraints_for_fields-2110345-62.patch queued for re-testing.

Re-roll.

Straight reroll.

@@ -114,11 +112,11 @@ function testValidation() {
-    // Provoke a email collision with an exsiting user.
+    // Provoke a email collision with an existing user.

If this is touched then also fix "a email collision" to "an email collision".

For the rest this looks RTBC to me.

Fixed that little typo and cleaned up a few stray whitespace removals that crept in the patch. I didn't touch the code itself so if this comes back green it's RTBC.

Agreed - RTBC

+++ b/core/modules/user/user.module
@@ -17,7 +17,7 @@
+use Drupal\Core\Field\FieldDefinition;

@@ -332,7 +332,7 @@ function user_load_by_name($name) {
+  $definition = FieldDefinition::create('string')

This needs to be changed to BaseFieldDefinition

Indeed, that inderdiff looks strange. Anyway, changes are good - thanks, back to RTBC.

Needs a reroll.

+++ b/core/modules/user/src/Entity/User.php
@@ -468,7 +468,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
-      ->setPropertyConstraints('value', array(

@@ -483,7 +483,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
-      ->setPropertyConstraints('value', array('UserMailUnique' => array()));

We now have no usages of BaseFieldDefinition::setPropertyConstraints(). Shouldn't we get rid of it? Let's explore this in a followup.

back to RTBC

Committed e32a11e and pushed to 8.0.x. Thanks!

Filed follow-up to further simplify defining unique field constraints at #2483869: Make Unique Field constraints generic