Entity + Field + Property validation constraints are processed in the incorrect order

Note that #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method introduced test coverage to show the current pain, and show the responses that should be sent.

Postponing on that landing.

#2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method landed!

The assertions introduced by #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method to verify that this is indeed broken on all PHP versions was itself broken, due to behavioral differences between PHP 5.5, 5.6 and 7.0. That resulted in failing tests on 5.6. Hence there's a critical issue to make the assertion of fatal failure work on 5.6: #2832013: CommentResourceTestBase::testPostDxWithoutCriticalBaseFields() always fails on PHP 5.6 & MySQL 5.5 (works fine on other PHP versions).

Now working on this fix, so we can get rid of that "fatal failure assertion" entirely.

So, this is the goal. But in order for this patch to be green, we'll need to fix the underlying bug.

That is the most nasty one that we need to fix (per #2832013: CommentResourceTestBase::testPostDxWithoutCriticalBaseFields() always fails on PHP 5.6 & MySQL 5.5 (works fine on other PHP versions)), but there's actually more to fix.

This is the complete goal.

The first of those (forgetting to specify entity_type) results in a failure at

$this->entityManager->getStorage($comment->getCommentedEntityTypeId())->resetCache(array($comment->getCommentedEntityId()));

Because $comment->values['entity_type']['x-default'] === [], which results in getCommentedEntityTypeId() === NULL, which then results in \Drupal\Core\Entity\EntityTypeManager::getDefinition() throwing a PluginNotFoundException.

But, note that that code is called by \Drupal::service('comment.statistics')->update($this);(), which is called from \Drupal\comment\Entity\Comment::postSave(), i.e. after the Comment entity was already saved! So, frighteningly, setting no entity_type value throws no exception whatsoever! Fortunately, \Drupal\Core\Entity\Sql\SqlContentEntityStorage::save() then rolls back the DB transaction. So, no harm is done. It throws an EntityStorageException with the message The "" entity type does not exist..

That EntityStorageException is caught by EntityResource::post(), which converts it to a 500 response:

    catch (EntityStorageException $e) {
      throw new HttpException(500, 'Internal Server Error', $e);
    }

Next: figuring out a solution for this.

You can test this locally by importing this config:

uuid: adde12d4-5598-41bc-8136-0e54a161110b
langcode: en
status: true
dependencies:
  module:
    - comment
id: entity.comment
plugin_id: 'entity:comment'
granularity: resource
configuration:
  methods:
    - GET
    - POST
    - PATCH
    - DELETE
  formats:
    - json
    - hal_json
  authentication:
    - basic_auth
    - cookie

and running this CURL command:

curl -X POST -u root:root -H "Content-Type: application/json" -d '{"comment_type":[{"target_id":"comment"}],"entity_id":[{"target_id":"1"}],"field_name":[{"value":"comment"}],"subject":[{"value":"Dramallama"}],"comment_body":[{"value":"Llamas are awesome.","format":"plain_text"}]}' http://d8/entity/comment?_format=json

which should result in this output:

{"message":"A fatal error occurred: Internal Server Error"}

Making the entity_type base field required already helps. The first assertion now works.

But… it's not enough. Making a field required does not ensure it contains a valid value yet. And unfortunately, that's exactly the problem: if we now pass a non-existing entity_type, it will still fail:

$ curl -X POST -u root:root -H "Content-Type: application/json" -d '{"comment_type":[{"target_id":"comment"}],"entity_id":[{"target_id":"1"}],"field_name":[{"value":"comment"}],"subject":[{"value":"Dramallama"}],"comment_body":[{"value":"Llamas are awesome.","format":"plain_text"}],"entity_type":[{"value":"foobar"}]}' http://d8/entity/comment?_format=json

{"message":"A fatal error occurred: Internal Server Error"}

Again because of a PluginNotFoundException (because the foobar entity type plugin does not exist), which is converted to a EntityStorageException et cetera.

Moving on to the next one: missing entity_id.

$ curl -X POST -u root:root -H "Content-Type: application/json" -d '{"comment_type":[{"target_id":"comment"}],"field_name":[{"value":"comment"}],"subject":[{"value":"Dramallama"}],"comment_body":[{"value":"Llamas are awesome.","format":"plain_text"}],"entity_type":[{"value":"node"}]}' http://d8/entity/comment?_format=json

We've got a serious infrastructure problem here…

1. entity_id is already marked as required. But, unfortunately, the CommentNameConstraint (introduced in #2002158: Convert form validation of comments to entity validation, with additional test coverage in CommentValidationTest) constraint runs first. And it uses entity_id (as well as field_name). But at this point, no validation has run yet to verify that entity_id is set! So, it's pretty much completely pointless that entity_id is required, because you could never ever get the corresponding validation error (entity_id: This value should not be null), since the CommentName validation error will always be shown instead, since its constraint validator runs first.
2. AFAICT, the entity validation system is specifically designed to validate from the top down. So, first it validates entity-level constraints, then field-level constraints, etc.
3. We can't "move down" the CommentName constraint, because it covers multiple fields: name and uid. So we can't move it to the field level instead of the entity level.

So… how to fix this? The only thing I can think of is to expand what CommentNameConstraintValidator validates: let it also validate the entity_id and field_name fields. But the right solution would be to let CommentNameConstraintValidator somehow indicate it requires the entity_id and field_name fields to already be validated before it can run, because those being valid is a hard requirement.

Unassigning, this needs a Symfony 2 Validation component expert.

I don't think we need to turn it around!

I think it needs to be possible to express dependencies. And in fact, we already have much of the necessary infrastructure!

\Drupal\comment\Plugin\Validation\Constraint\CommentNameConstraint contains this:

  public function coversFields() {
    return ['name', 'uid'];
  }

What we need on top of that is ConstraintDependencyInterface (better name TBD), which allows that class to also specify:

  public function dependencies() {
    return ['entity_id', 'field_name'];
  }

Or perhaps this belongs in the @Constraint annotation:

 * @Constraint(
 *   id = "CommentName",
 *   label = @Translation("Comment author name", context = "Validation"),
 *   type = "entity:comment",
 *   dependencies = {
 *     "entity_id",
 *     "field_name",
 *   },
 * )

That would ensure that when validating the CommentNameConstraint, we'd first run any constraints for those lower levels (fields). Which could absolutely be a pure API addition, without BC break.

@fago: thanks for the review!

Only 6 failures. That means this is causing few regressions, if any, and it's probably feasible.

Now let's see if can help us fix the terrible Comment POSTing DX. As I explained in #11, making entity_type required allows us to pass the first test in testPostDxWithoutCriticalBaseFields(). The second test is testing a missing entity_id. And that's what I pointed out in my analysis in #12 as being the one to suffer from the current validation order (top-down instead of bottom-up).

Unfortunately, that's not what happens. The patch in #17 does result in a "This value should not be NULL" (aka required field has no value) constraint violation message for entity_id. But … it still allows CommentNameConstraintValidator to continue, despite that needing entity_id to have a correct value. This makes me suspect that in this Symfony Validator component, there's no concept of "only execute this constraint validator if all prior/basic ones did not complain"?

But then if I go and look at the docs, they do support this: http://symfony.com/doc/2.8/validation/sequence_provider.html seems a perfect fit, or otherwise http://symfony.com/doc/2.8/validation/groups.html. Entity validation experts, back to you!

In this patch, I:

1. bring back the changes from my earlier patches to make entity_type a required base field; this allows me to make the first test in testPostDxWithoutCriticalBaseFields() to pass again. This does NOT exercise the changes proposed by @dawehner.
2. now also update the second test in testPostDxWithoutCriticalBaseFields() to its desired state. I made this pass by modifying CommentNameConstraintValidator to only run if both entity_id and field_name are not empty (note that this is not sufficient — they should be guaranteed to be valid at this point). The correct solution would be to use a "sequence provider" AFAICT.
3. The changes in CommentNameConstraintValidator should be reverted.
4. The patch should have just as many failures as #17 — addressing those failures is not of the utmost importance right now. What matters more, is making sure only the appropriate constraint validators run, and only when necessary.

This also causes the following change in error response. I'm not sure this is correct/desirable. But that's just a detail in the grand scheme of things here.

This is now also blocking #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX :(

Summoning @fago again.

#29: thanks :) But until we have a patch that actually works just for fresh installs, that is not something we need to think about yet! Hopefully we'll be able to write an update hook soon!

We just discussed this in an Entity/Field API API-First Initiative blocker call. The consensus is that this is major due to the incorrect order in which validation constraints are checked.

However, it's difficult to see what the original problem (wrt Comments) ends and the general problem begins. So it was determined that the first task is to have a separate issue for making the entity_type base field on the Comment entity type required. Did that: #2885809: The 'entity_type' and 'field_name' base fields on Comment are required.

Reading this issue now, that means doing a subset of the patch in #11. But it'll still retain the majority of the original problem. At least it will be slightly smaller.

Also retitling to reflect the intended scope.

IS updated.

Yes — if there's an error at the field-level constraints, the entity-level constraints should not be executed, because the entity-level constraints assume valid field values.

+++ b/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php
@@ -134,13 +134,18 @@ protected function validateNode(TypedDataInterface $data, $constraints = NULL, $
+    // Don't validate the current level if there has been any violations on the
+    // deeper levels, as the current level might require for example the
+    // existence of some values.

Great comment.

Might also be good to add a comment to the class-level docblock, explaining that this validates depth-first.

#2885809: The 'entity_type' and 'field_name' base fields on Comment are required landed!

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -311,7 +311,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->addConstraint('ValidEntityType')

+++ b/core/modules/comment/src/Plugin/Validation/Constraint/EntityTypeConstraint.php
@@ -0,0 +1,24 @@
+  public $message = 'This field requires a valid entity type.';

+++ b/core/modules/comment/tests/src/Kernel/CommentValidationTest.php
@@ -76,6 +76,19 @@ public function testValidation() {
+    // Provide an invalid entity type.
+    $comment = $this->entityManager->getStorage('comment')->create([
+      'entity_id' => $node->id(),
+      'entity_type' => 'invalid-entity_type',
+      'field_name' => 'comment',
+      'comment_type' => 'comment',
+      'comment_body' => $this->randomMachineName(),
+    ]);
+    $violations = $comment->validate();
+    $this->assertCount(1, $violations, 'Violation found on entity type');
+    $this->assertEquals('entity_type', $violations[0]->getPropertyPath());
+    $this->assertEquals(t('This field requires a valid entity type.'), $violations[0]->getMessage());

But I think we still want this?

To be fair, the most important part of this issue has now already been solved in #2885809: The 'entity_type' and 'field_name' base fields on Comment are required, meaning that I think we can downgrade this from major to normal.

#59: Excellent, I was hoping more people would comment here and report other cases where this causes problems!

I think that's the additional impact we needed to hear about to justify bumping this to Major.

It'd be wonderful if you could update the issue summary with your perspective, with the use cases you see. 🙏

Don't underestimate yourself! :)

I think this is plenty clear as-is. I'm just quoting the entirety of #61 into the issue summary 😀

Please add that to the issue summary! 👍

Came up again in \Drupal\ckeditor5\Plugin\Validation\Constraint\SourceEditingRedundantTagsConstraintValidator.

Agreed that we should continue to return *all* validation errors!

But wouldn't depth-first still be preferable? 🤔

I apologize for rigidly pushing for a certain direction ~5 years ago. Let's get this back on track, and let's retitle this issue as you ask! But how?

1. The example I gave in #23 is that of CommentNameConstraintValidator needing a valid entity ID. I realize that not every validator needs other things to have validated already. Maybe we can optionally allow a validator to indicate which nested values need to have been validated already?
   → title could be "Allow validation constraints to specify which nested values must be validated first"
2. Or … perhaps you're saying: we should not add more infrastructure for this in Drupal, the few scenarios that need it should just use https://symfony.com/doc/5.4/validation/sequence_provider.html? Or … perhaps the new-since-Symfony-5.1 https://symfony.com/doc/5.4/reference/constraints/Sequentially.html?
   → title could be "Update CommentNameConstraintValidator to specify a validation sequence"

User enters '34' for the day of the month. Validation fails on that property. You don't validate the whole date, because the data is meaningless.

+

The same applies to the comment problem. If the entity_type is invalid, there is nothing you can work with.

Exactly!

I wrote this in #73.2:

Or … perhaps you're saying: we should not add more infrastructure for this in Drupal, the few scenarios that need it should just use https://symfony.com/doc/5.4/validation/sequence_provider.html?

I investigated this today, to check if this is viable to use today 🤓

Conclusion: unfortunately not.

Because Drupal core does not use \Symfony\Component\Validator\Validator\RecursiveContextualValidator implements ContextualValidatorInterface but \Drupal\Core\TypedData\Validation\RecursiveContextualValidator implements ContextualValidatorInterface, and the Drupal implementation does:

1.     if (isset($groups)) {
         throw new \LogicException('Passing custom groups is not supported.');
       }
   

2.     if (isset($constraints) || !$this->context->isGroupValidated($cache_key, Constraint::DEFAULT_GROUP)) {
         if (!isset($constraints)) {
           $this->context->markGroupAsValidated($cache_key, Constraint::DEFAULT_GROUP);
           $constraints = $metadata->findConstraints(Constraint::DEFAULT_GROUP);
         }
         $this->validateConstraints($value, $cache_key, $constraints);
       }
   

   meaning: Drupal hardcodes the default group and doesn't auto-resolve it into the concrete set of groups specified for the class, which is what Symfony's implementation does:

               // Replace the "Default" group by the group sequence defined
               // for the class, if applicable.
               // This is done after checking the cache, so that
               // spl_object_hash() isn't called for this sequence and
               // "Default" is used instead in the cache. This is useful
               // if the getters below return different group sequences in
               // every call.
               if (Constraint::DEFAULT_GROUP === $group) {
                   if ($metadata->hasGroupSequence()) {
                       // The group sequence is statically defined for the class
                       $group = $metadata->getGroupSequence();
                       $defaultOverridden = true;
                   } elseif ($metadata->isGroupSequenceProvider()) {
                       // The group sequence is dynamically obtained from the validated
                       // object
                       /* @var \Symfony\Component\Validator\GroupSequenceProviderInterface $object */
                       $group = $object->getGroupSequence();
                       $defaultOverridden = true;
   
                       if (!$group instanceof GroupSequence) {
                           $group = new GroupSequence($group);
                       }
                   }
               }
   

This means that today, it is impossible to use GroupSequenceProviderInterface today. Even though that would AFAICT be possible without changing any other API. If we'd port the if (isGroupSequenceProvider) branch into Drupal's recursive validator, it would be possible. (EDIT: that's not possible 1:1 because Drupal uses class TypedDataMetadata implements MetadataInterface, and MetadataInterface does not have "group sequence" methods, only ClassMetadataInterface does.)

Why? Quoting @fago from 2015 in #2343035-155: Upgrade validator integration for Symfony versions 2.5+:

The implemenation does not cover groups at all - as we do not need them and did not implement them before in our Metadata implementations either. This allows our RecurisveValidator to save quite some code over the symfony one. If we decide we want to add this feature later on, we'd have to add a respective implementation also.

I think this issue proves there is a reason to support it. Which @dawehner also said at #2343035-159: Upgrade validator integration for Symfony versions 2.5+.2:

As probably said personally, I think not implementing groups at the moment is totally fine. Its a feature which we don't use really and could add later, if it really would be needed.

Note that that

Considering #77, I think we need to move this to the typed data system component.

I think you're right 😔 The "group" functionality only works for executing multiple validation constraints on one node in a specific order. But in this case, it's child nodes that would need to be validated first. 😬

The problem we're trying to solve here is two-fold:

1. all of Drupal core, for both content and config, lumps all validation constraints together in one bucket
2. all of Drupal core, for both content and config, pays no attention to the validation sequence: it always uses the same top-down (breadth-first, see \Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validateNode()) recursive validation strategy, which results in the entity-level validation constraints not being able to assume valid field values and field-level validation constraints not being able to assume valid property values

I suspect

• https://symfony.com/doc/current/validation/groups.html
• https://symfony.com/doc/current/validation/sequence_provider.html
• https://symfony.com/doc/current/reference/constraints/Sequentially.html is the solution.

are part of the solution.

\Drupal\Tests\jsonapi\Functional\ResourceTestBase has thorough test coverage, but it does NOT assert that the validation error messages in case of a nonsensical value (for example, the string hello world being specified for a boolean "status" field or an integer "weight" field).
That test (and the REST equivalents) are specifically testing access control and basic shape of the representation, not detailed validation.

I have recently started encountering this problem again, but now in the world of Config, because I've been working on config validation. There, it's likely that you'll see multiple errors for the same config property path, for example:

[
  0 => "The '' plugin does not exist.",
  1 => "This value should not be null.",
]

It goes without saying that the latter (generated by the NotNull constraint) should result in none of the other validators running, because there literally is no point in them running. The consequence: noisy, confusing validation errors.