Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered

This blocks criticals

Hm, can you clarify? #2278523: Avoid dynamically defining constraints (in related issues) is only a major. And the only other issue it references is #2023465: Allow validation constraints to add in message replacements which is also a major.

#2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay I think.

#2403485: Complete conversion of comment form validation to entity validation it blocks

optimistically taking this on, if you don't see anything in a week or so, send a search party, I'll be trapped in typed data hell

From what I can see - this might already be done - the ConstraintValidator classes are passes a FieldItemList as arguments, which means the entity ($items->getEntity()) is available, meaning although a validator is defined for a single field, it has access to all field values.
I don't think this blocks #2403485: Complete conversion of comment form validation to entity validation anymore, so going to demote this and unpostpone that. See you over there.

It turns out that you want to validate more than the stuff defined by your entity type, but you want to be able to apply business logic on your validation as well.
For that usecase we need a way for this, see #2405503: How to validate a REST submitted node?

Which would make #2346329: hook_entity_base_field_info_alter() and hook_entity_bundle_field_info_alter() are documented to get a parameter that doesn't implement an interface with setter methods critical again?

@larowlan
Note: I don't think that its the best idea to pin that kind of validation onto base fields directly. We are talking about things you normally just do for some bundles in a custom module so having an api which scales horizontal would be nice.

why not

So I think we can use something in a similar fashion to http://symfony.com/doc/current/reference/constraints/Collection.html here, but we need #2429037: Allow adding entity level constraints first to be able to add the composite constraint to the comment entity and replace CommentNameConstraintValidator with something similar - so I think that then makes this postponed as well - but will work on a patch that also includes #2429037: Allow adding entity level constraints as it stands.

my brain can't process this at the moment

I'm curious how horrible wrong this is.

and now my brain can process it @dawehner++ code > text

Let's see whether the testbot likes us

This time with more manual testing.

And some documentation / test coverage.

I have two questions about how you would work with a FieldItem instead of the scalar values in case you need access to more than just the main property. And what happens if you want to validate the second or third item in the field item list (or access the whole FieldItemList) ?

I personally think that passing the field name in fields should get you the field item list. The validator should deal with whether it wants the first/second/each value or the required properties - also if you know you want a specific property, we could also support [fields => [field_foo.0.value]] style format.

Eg in summary

• ['fields' = ['field_foo']]; // Gets you the FieldItemList.
• ['fields' = ['field_foo.0']]; // Gets you the FieldItem at delta 0.
• ['fields' = ['field_foo.0.title']]; // Gets you the scalar value of the title property of delta 0.

1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/CompositeConstraintValidatorBase.php
   @@ -0,0 +1,66 @@
   +      $split = explode('.', $field);
   

   We should add the third parameter here, but as mentioned above we should allow up to three (name, delta, property)

2. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/CompositeConstraintValidatorBase.php
   @@ -0,0 +1,66 @@
   +      $values[$field] = $entity->{$field}->first()->{$property};
   

   I think wiring this to first() is wrong

3. +++ b/core/tests/Drupal/Tests/Core/Validation/Plugin/Validation/Constraint/CompositeConstraintValidatorBaseTest.php
   @@ -0,0 +1,83 @@
   +  public function testValidate() {
   

   can we get a docblock and a @covers here?

well, the value I see is that it helps to ensure people are only using the fields they are supposed to. But for that, having an array of field item objects would suffice as well, e.g.:

If you don't specify the fields you want from outside, what exactly is the major gain of the base class? Can't you pull all the values you want
already from HEAD? I try to figure out where exactly the value would be, if you still have to do it manually?

Can't you pull all the values you want
already from HEAD? I try to figure out where exactly the value would be, if you still have to do it manually?

Yes, this issue made no sense to me, see #15, #16, #17 - until I saw it in code from @dawehner. If you remove the getFields, then I'm back at the 'makes no sense to me' station

dawehner: is this just about defining the properties in the annotation so that form validation can show errors online?

larowlan: dawehner: oh, you might be right, so entityformdisplay will scan the constraints and see oh, constraint x needs field a and b, but a isn't in this form, so no need to validate

larowlan: dawehner: in which case I think it makes sense to keep getFields, but just make them return the item list?

dawehner: larowlan: right
dawehner: larowlan: maybe this explains a bit what fago was saying

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/CompositeConstraintBase.php
@@ -0,0 +1,43 @@
+abstract class CompositeConstraintBase extends Constraint {

I was thinking about how this applies to the blocked issue on entity-form displays and how the code might look if this were in. In my opinion if we had an interface for composite constraints the code would be nicer because we'd get if ($constraint instanceof CompositeConstraintInterface) instead of using is_subclass_of()

Also, if what @dawehner said via irc in #30 is correct, do we need to be using 'name' in 'fields' in the conversion of CommentNameAuthorConstraint (sp?) if name is the field it is attached to already? Should we just be tracking fields that the constraint depends on in addition to the field it is attached to?

I honestly still don't see how the entity display validation should be aware, of which fields are covered. Does that mean its code has to special case the new base constraint?

So thinking ahead to #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay we're going to collect entity-level constraints in the form display, and filter out those that don't apply, then apply the remaining.
As I understand it, the validate method on the entity doesn't accept an array of constraints, how are we going to pass that context to the validator?
I can't fault the code as it stands, but I'm not sure it gets us towards the end-solution just yet.

We can filter the violations once they are created, based upon their constraint, property path (if not composite) and field access

Perfect, that makes total sense now, thank you

• Removed the validator base, its not worth it, IMHO
• Tried to integrate it into EntityFormDisplay, but it seems to be impossible to get the constraint from a given violation.
  \Symfony\Component\Validator\ExecutionContext::addViolation does not add the constraint, \Symfony\Component\Validator\Context\ExecutionContext::addViolationdoesn't do it, so we maybe have to wait for #2343035: Upgrade validator integration for Symfony versions 2.5+

@fago
So should we better postpone onto #2343035: Upgrade validator integration for Symfony versions 2.5+ and then finish this issue?

Discussed with the branch maintainers and they agreed this is critical, given that core has such a constraint, making this a hard blocker for #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay. Tagging, retitling, and see also #2395831-54: Entity forms skip validation of fields that are not in the EntityFormDisplay.

Also adding the blocker tag, since another issue is postponed on this.

Also tagging for security and WSCCI, as it is required for secure web services.

Per #46.

There's a massive amount of context here that I don't have... what exactly is needed from the WSCCI side?

I don't think there's anything needed here from the WSCCI team, but I tagged it WSCCI in #45 due to there being potential security problems (data validations not executed) in non-form REST submissions until this is fixed.

hm, I was thinking to add the violation filtering logic in #2395831: [PP-2] Entity forms skip validation of fields that are not in the EntityFormDisplay and just make sure the implementation of composite contraints done here provides the metadata necessary for it. #2395831: [PP-2] Entity forms skip validation of fields that are not in the EntityFormDisplay already prepares a patch for it, so could we work on it there? I'd be happy about reviews about the approach taken there!

Oh sure, let's do that there . This is for now just a reroll.

This test doesn't make sense anymore now that we don't longer have the base class ...

Given that fago also wanted to introduce the actual form integration in #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay it doesn't really make sense to test something related to this here.
I think its fine to keep EntityTestCompositeConstraint as it can be used in the future.
Beside from that there is nothing to do here, honestly.

Yep, we just needed the 2.5 API so we could get the constraint from the violation. Which we have.

In my books, this is RTBC except for:

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/CompositeConstraintBase.php
@@ -0,0 +1,32 @@
+ * type. See ... @todo. fix this.

This would have referenced #2429037: Allow adding entity level constraints but that is since fixed. So the See ... @todo can instead be @see \Drupal\Core\Entity\EntityType::addConstraint or a reference to the annotation.

There we go.

unless bot disagrees, but possibly even if bot disagrees (bots have been misbehaving today)

The patch looks good to me as well, I just found two small nitpicks:

1. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -46,40 +47,35 @@ public static function create(ContainerInterface $container) {
   +    $owner_id = (int) $entity->uid->target_id;
   

   Is this really necessary? Wouldn't it prevent someone from overriding the User entity class and use "machine names" for the id? Sure, they would need to keep '0' as the machine name for the anonymous user but other than that it should be possible :)

2. +++ b/core/modules/system/src/Tests/Entity/FieldWidgetConstraintValidatorTest.php
   @@ -55,6 +55,7 @@ public function testValidation() {
   +    $this->assertEqual($errors['type'], 'Multiple fields are validated', 'Constraint violation is generated correctly');
   

   If you're going to address the point above (i.e. if you roll a new patch), how about also changing the message here to "Composite constraint violation is generated correctly"?

(int) $entity->uid->target_id
This is consistent with EntityOwnerInterface

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/CompositeConstraintBase.php
@@ -0,0 +1,34 @@
+ * With that the $values in CompositeConstraintValidatorBase::doValidate() will
+ * return an array keyed with 'name', 'uid' and 'entity', with 'entity' being
+ * the actual full entity.

The "name, uid, entity" part looks specific to the CommentNameConstraint concrete class ?

Also, is that really up to date ? Implementations in the patch seem to be doing $entity = $value->getEntity() ?

1. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -21,6 +20,13 @@
   +   * Validator 2.5 and upwards compatible execution context.
   +   *
   

   I don't think we have to explicit document that its compatible with 2.5 and above ... the interface already has that kind of information.

2. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -62,21 +62,27 @@ public function validate($value, Constraint $constraint) {
   -        $this->context->addViolation($constraint->messageNameTaken, array('%name' => $author_name));
   +        $this->context->buildViolation($constraint->messageNameTaken, array('%name' => $author_name))
   +          ->atPath('name')
   +          ->addViolation();
   ...
   -        $this->context->addViolation($constraint->messageMatch);
   +        $this->context->buildViolation($constraint->messageMatch)
   +          ->atPath('name')
   +          ->addViolation();
   

   Urgs, so we now have to use that new API which is a little bit more verbose?

This is an api addition so API change section needs an update in IS and we need a change notice as well.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/CompositeConstraintBase.php
   @@ -0,0 +1,34 @@
   + * With that the $values in CompositeConstraintValidatorBase::doValidate() will
   + * return an array keyed with 'name', 'uid' and 'entity', with 'entity' being
   + * the actual full entity.
   
   +++ b/core/modules/comment/src/Entity/Comment.php
   +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -54,6 +54,9 @@
   

   This doesn't make sense. There is no class name CompositeConstraintValidatorBase neither in this patch nor in core.

2. +++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php
   @@ -46,41 +53,36 @@ public static function create(ContainerInterface $container) {
   -        $this->context->addViolation($constraint->messageNameTaken, array('%name' => $author_name));
   +        $this->context->buildViolation($constraint->messageNameTaken, array('%name' => $author_name))
   +          ->atPath('name')
   +          ->addViolation();
   

   I know we need a change notice overall but we need to mention this in it as well.

Added an IS/CR

This doesn't make sense. There is no class name CompositeConstraintValidatorBase neither in this patch nor in core.

Good catch! This is now entirely not needed anymore.

I hope the fix for the CommentAnoymousTest is now the proper way to do, before we have the custom violation list introduced in #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay

There we go, this time with the right bug fix.

Thanks for the fixes.

+++ b/core/modules/comment/src/CommentForm.php
@@ -323,9 +324,12 @@ public function validate(array $form, FormStateInterface $form_state) {
+      if ($violation->getPropertyPath() === 'name') {

Why not ditch this and use $violation->getPropertyPath() as a first param of the setErrorByName Method?

Why not ditch this and use $violation->getPropertyPath() as a first param of the setErrorByName Method?

Well, we just want to take into account violations against the name field here, other violations, like for fields itself should be ignored here.
They are handled in the entity display form component.

Yes this means that at the moment, we run most of the validations twice for comments.

Good points, addressed them quickly.

The change is only required if you pass any additional arguments beside message + message params. But that's easy missed when addViolation() is used else - thus I'm wondering whether it would make sense to promote using buildViolation() always, or at least always for composite constraints? It's a little bit more verbose, but less error-prone. In particular composite constraints will have to add sub-paths.

I think only in composite constraints is fine.

+++ b/core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraint.php
@@ -7,17 +7,18 @@
-use Symfony\Component\Validator\Constraint;
+use Drupal\Core\Entity\Plugin\Validation\Constraint\CompositeConstraintBase;

Nice.

I think everything is done here.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 69ce355 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/comment/src/CommentForm.php b/core/modules/comment/src/CommentForm.php
index 1436f7c..12f5bb8 100644
--- a/core/modules/comment/src/CommentForm.php
+++ b/core/modules/comment/src/CommentForm.php
@@ -19,7 +19,6 @@
 use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\Session\AccountInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
-use Symfony\Component\Validator\ConstraintViolation;
 
 /**
  * Base for controller for comment forms.
diff --git a/core/modules/system/src/Tests/Entity/EntityValidationTest.php b/core/modules/system/src/Tests/Entity/EntityValidationTest.php
index 1cce032..d4358fc 100644
--- a/core/modules/system/src/Tests/Entity/EntityValidationTest.php
+++ b/core/modules/system/src/Tests/Entity/EntityValidationTest.php
@@ -188,7 +188,7 @@ public function testCompositeConstraintValidation() {
 
     // Make sure we can determine this is composite constraint.
     $constraint = $violations[0]->getConstraint();
-    $this->assertTrue($constraint instanceof CompositeConstraintBase, 'Constraint is composite contraint.');
+    $this->assertTrue($constraint instanceof CompositeConstraintBase, 'Constraint is composite constraint.');
     $this->assertEqual('type', $violations[0]->getPropertyPath());
 
     /** @var CompositeConstraintBase $constraint */

Fixed on commit.