Allow adding entity level constraints

Last item in the propsed resolution:

Add entity constraints via defining type constraints at the EntityDeriver or in EntityDataDefinition::getConstraints()

I don't think I got that one :-)

Sounds good, I have no better idea. We could also use the convention to add entity constraints to the ID field of the entity in baseFieldDefinitions(), but an explicit method is probably better.

A lot of static entity-type definitions are handled in the annotation - is there scope to allow definition of entity-level constraints there too?

Also, willing to help here - assume you're working on it because of the assigned field, but count me in for helping.

Thanks, will keep an eye on the issue

fails were because of changes to default plugin manager.
reverted those and fixed the root of the issue, calling enableModules builds a new container, so the $this->entityManager refers to a different object from that point on (yay for simpletest).

Minor nitpick:

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php
@@ -159,6 +159,7 @@ public function getDefinitions() {
+

Accidental?

full revert (fixed #18)

Anything else need doing here?

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
   @@ -83,7 +84,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   -        'constraints' => array('EntityType' => $entity_type_id),
   +        'constraints' => $entity_type instanceof ContentEntityTypeInterface ? $entity_type->getConstraints() : [],
   

   Are we losing the 'EntityType' constraint here?

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
   @@ -91,10 +92,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   -            'constraints' => array(
   -              'EntityType' => $entity_type_id,
   -              'Bundle' => $bundle,
   -            ),
   +            'constraints' => $this->derivatives[$entity_type_id]['constraints']
   

   Same - are we losing the Bundle here?

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
@@ -22,7 +24,9 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
+      // @todo: Once the EntityChanged constraint is completely moved to the
+      // entity level, remove the if clause.

Do we have an issue for this?

Cool, otherwise this looks good, I'm glad larowlan had the idea to have this in the entity annotation :-)

Perfect let's wait for bot.

+++ b/core/modules/quickedit/src/Form/QuickEditFieldForm.php
@@ -148,14 +160,17 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+    // Run entity-level validation as well, while skipping validation of all
+    // fields. We can do so by fetching and validating the entity-level
+    // constraints manually.
+    // @todo: Improve this in https://www.drupal.org/node/2395831.
+    $typed_entity = $entity->getTypedData();
+    $violations = $this->typedDataManager
+      ->getValidator()
+      ->validateValue($typed_entity, $typed_entity->getConstraints());

So the only purpose of $this->typedDataManger in this class is to spit out the validator? Then you should inject the validator in the first place. Or do you assume that we will use the typed data manager for more stuff later?

And why do you pass $typed_entity to validateValue()? In that method you then get the entity again, so this looks like useless typed data switching back and forth? Please add a comment.

Otherwise looks good.

But we can just inject the validator instead of the TypedDataManager in QuickEditFieldForm.php, correct? We would do

$container->get('typed_data_manager')->getValidator()

in the create() method.

I didn't realize that we always have to pass typed data objects to the validator, so I think that's fine and #2346373: Data reference validation constraints are applied wrong can take care of the receiving end in the constraint classes, we don't have to do that here (not critical).

working on #42

Fixes #42

doh, two ValidatorInterfaces

The basic approach here looks good to me.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
   @@ -83,7 +84,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   -        'constraints' => array('EntityType' => $entity_type_id),
   ...
   -            'constraints' => array(
   -              'EntityType' => $entity_type_id,
   -              'Bundle' => $bundle,
   -            ),
   ...
   +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -400,10 +400,6 @@ public function getDefaultConstraints(DataDefinitionInterface $definition) {
   -    // Add any constraints about referenced data.
   -    if ($definition instanceof DataReferenceDefinitionInterface) {
   -      $constraints += $definition->getTargetDefinition()->getConstraints();
   -    }
   

   Maybe I missed something, but I don't see where these are moved to. If they're being removed entirely, can the reason for that be added to the issue summary?

2. +++ b/core/modules/quickedit/src/Form/QuickEditFieldForm.php
   @@ -148,14 +160,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   -    if ($changed_field_name = $this->getChangedFieldName($entity)) {
   

   Let's delete the getChangedFieldName() method while we're at it.

Fixes #48.2
Added comments to IS about 48.1, I'd asked the same thing earlier - the answer is they're already added by EntityDataDefinition::{setEntityTypeId|setBundle}.

Cool. Thanks. What about the last part of #48.1?

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -400,10 +400,6 @@ public function getDefaultConstraints(DataDefinitionInterface $definition) {
-    // Add any constraints about referenced data.
-    if ($definition instanceof DataReferenceDefinitionInterface) {
-      $constraints += $definition->getTargetDefinition()->getConstraints();
-    }

Looks like this patch changes EntityReferenceItem to add explicit constraints for entity type and bundle, but this code was applying all target constraints (which could have included more than that) to a reference item. What's the reason for us no longer wanting that? I guess it's because something like EntityChanged only makes sense on the target and not on the reference? And that by similar logic we think the same will be true for all other entity-level constraints? But I'm a bit fuzzy on that, so having it spelled out in the issue summary would be helpful.

RE #50 please see #34 and #2346373: Data reference validation constraints are applied wrong (relevant parts are in this issue).

Updated IS

It feels right to not automatically reflect the constraints of the referenced entity higher up in the reference field, in the case of referenced entities that already exist.
However, not sure how an autocreated entity ever gets validated ?

I think we should move all entity reference discussion to #2346373: Data reference validation constraints are applied wrong.

The code looks RTBC to me now, thanks!

I think we need a change notice for this, explaining the differences how it was done in D7 and now in D8. The EntityChanged use case for nodes seems like a good example.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -89,12 +89,17 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
-      ->setTargetDefinition(EntityDataDefinition::create($settings['target_type']));
+      ->setTargetDefinition(EntityDataDefinition::create($settings['target_type']))
+      ->addConstraint('EntityType', $settings['target_type']);
...
-      $properties['entity']->getTargetDefinition()->addConstraint('Bundle', $settings['target_bundle']);
+      $properties['entity']->addConstraint('Bundle', $settings['target_bundle']);
+      // Set any further bundle constraints on the target definition as well,
+      // such that it can derive more special data types if possible. For
+      // example, "entity:node:page" instead of "entity:node".
+      $properties['entity']->getTargetDefinition()
+        ->addConstraint('Bundle', $settings['target_bundle']);

Would anyone like to help me in fixing this for DER?
Thanks.

RTBC as per #53

Tagging for triage as to whether it's critical, because I'm not completely clear on whether this is a hard-blocker for #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay. In the case of the EntityChanged constraint, why is it better for that to be an entity-level constraint vs. allowing field-level constraints to specify whether they should be applied always vs. only-when-changed? There's probably reasons that the former is better than the latter, just wanting clarity on what they are.

This both blocks #2395831: Entity forms skip validation of fields that are not in the EntityFormDisplay and is required for secure web services, so adding a bunch of tags. Also, triaged based on same reasoning as #2105797-43: Add CompositeConstraintBase so that constraints involving multiple fields, such as CommentNameConstraint, can be discovered.

@jibran yes, since #2346373: Data reference validation constraints are applied wrong went in, this issue is not postponed anymore, and I think work here can start again.

Back to RTBC

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityChangedConstraintValidator.php
@@ -19,14 +18,13 @@ class EntityChangedConstraintValidator extends ConstraintValidator {
-  public function validate($value, Constraint $constraint) {
-    if (isset($value)) {
-      /** @var $entity \Drupal\Core\Entity\EntityInterface */
-      $entity = $this->context->getMetadata()->getTypedData()->getEntity();
+  public function validate($entity, Constraint $constraint) {
+    if (isset($entity)) {
+      /** @var \Drupal\Core\Entity\EntityInterface $entity */

Love this change magic++

For #55 we have #2405467: Add EntityType and Bundle constraint on entity property of DynamicEntityReferenceItem in DER issue queue. @fago it would be great if you could share some thoughts over there. Thanks for all the help in IRC and the work on this patch.

The CR needs to be updated now that #2346373: Data reference validation constraints are applied wrong has landed.

Re #54 - did the non-critical followup for auto created entities get created?

diff --git a/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
index 7cea3c8..76decd9 100644
--- a/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\Core\Entity\Plugin\DataType\Deriver;
 
-use Drupal\Core\Entity\ContentEntityTypeInterface;
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Plugin\Discovery\ContainerDeriverInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
diff --git a/core/modules/quickedit/src/Form/QuickEditFieldForm.php b/core/modules/quickedit/src/Form/QuickEditFieldForm.php
index bdf058f..2db50c4 100644
--- a/core/modules/quickedit/src/Form/QuickEditFieldForm.php
+++ b/core/modules/quickedit/src/Form/QuickEditFieldForm.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\quickedit\Form;
 
-use Drupal\Core\Entity\FieldableEntityInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityStorageInterface;
 use Drupal\Core\Entity\EntityChangedInterface;

The removed use statements above can be removed due to this patch.

Re #54 - did the non-critical followup for auto created entities get created?

Yes it was, see #2438017: Entity reference does not validate auto-created entities

- Re-rolled with #74 addressed. Fortunately alexpott is really great in catching those deprecated use statements :-)

I'm still envy for the fabpot being able to automatically detect those problems, as well as CS problems.

Back to RTBC

@fago there seems to be a lot of overlap between this issue and #2142993: Test for EntityChangedConstraintValidator, should we close the other issue as a duplicate?

As EntityTypeConstraintsTest in this patch does essentially all the things EntityChangedConstraintValidatorTest does in the other issue...

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

just a quick follow-up question re #79: it seems we cannot do EntityChanged constraint validation on the changed field itself now (Ie $entity->changed->validate()). This is how we want it?

This caused a regression in Quick Edit: #2475483: Cannot quickedit an image or date field

Help welcome in #2064191-22: Fix the 'target_bundle' field setting - 'target_bundles' are not validated when entity_reference is enabled :-)

Basically, ER field 'target_bundles' setting really lives on the FieldDefinition level, and the one still present as a FieldStorage-level setting is just a decoy at this point.

That issue over there is trying to clean that up, and remove the dummy 'target_bundle' that still exists on the storage.

This means that the code that assigns the 'Bundle' constraint :

ERItem::propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    if (isset($settings['target_bundle'])) {
      $properties['entity']->addConstraint('Bundle', $settings['target_bundle']);
      // Set any further bundle constraints on the target definition as well,
      // such that it can derive more special data types if possible. For
      // example, "entity:node:page" instead of "entity:node".
      $properties['entity']->getTargetDefinition()
        ->addConstraint('Bundle', $settings['target_bundle']);
    }

cannot live there anymore (we don't know the field-level settings in ERItem::propertyDefinitions()), but needs to go in the runtime ERItem::getConstraints(). But there I have no clue how to set the *second* constraint (the one on $properties['entity']->getTargetDefinition()). It's also not clear to me why that second constraint is needed (as opposed to just the one directly on $properties['entity']) :-)