Issue #2541228 by amateescu, g089h515r806, Berdir, pawel_r:...

Issue #2541228 by amateescu, g089h515r806, Berdir, pawel_r: FieldConfigBase::setPropertyConstraints() and addPropertyConstraints() are broken

core/lib/Drupal/Core/Field/FieldConfigBase.php

@@ -180,6 +180,14 @@ abstract class FieldConfigBase extends ConfigEntityBase implements FieldConfigIn
    */
   protected $constraints = [];
 
+  /**
+   * Array of property constraint options keyed by property ID. The values are
+   * associative array of constraint options keyed by constraint plugin ID.
+   *
+   * @var array[]
+   */
+  protected $propertyConstraints = [];
+
   /**
    * {@inheritdoc}
    */
@@ -515,7 +523,20 @@ public function getItemDefinition() {
     if (!isset($this->itemDefinition)) {
       $this->itemDefinition = FieldItemDataDefinition::create($this)
         ->setSettings($this->getSettings());
+
+      // Add any custom property constraints, overwriting as required.
+      $item_constraints = $this->itemDefinition->getConstraint('ComplexData') ?: [];
+      foreach ($this->propertyConstraints as $name => $constraints) {
+        if (isset($item_constraints[$name])) {
+          $item_constraints[$name] = $constraints + $item_constraints[$name];
+        }
+        else {
+          $item_constraints[$name] = $constraints;
+        }
+        $this->itemDefinition->addConstraint('ComplexData', $item_constraints);
       }
+    }
+
     return $this->itemDefinition;
   }
 
@@ -546,9 +567,12 @@ public function addConstraint($constraint_name, $options = NULL) {
    * {@inheritdoc}
    */
   public function setPropertyConstraints($name, array $constraints) {
-    $item_constraints = $this->getItemDefinition()->getConstraints();
-    $item_constraints['ComplexData'][$name] = $constraints;
-    $this->getItemDefinition()->setConstraints($item_constraints);
+    $this->propertyConstraints[$name] = $constraints;
+
+    // Reset the field item definition so the next time it is instantiated it
+    // will receive the new constraints.
+    $this->itemDefinition = NULL;
+
     return $this;
   }
 
@@ -556,15 +580,14 @@ public function setPropertyConstraints($name, array $constraints) {
    * {@inheritdoc}
    */
   public function addPropertyConstraints($name, array $constraints) {
-    $item_constraints = $this->getItemDefinition()->getConstraint('ComplexData') ?: [];
-    if (isset($item_constraints[$name])) {
-      // Add the new property constraints, overwriting as required.
-      $item_constraints[$name] = $constraints + $item_constraints[$name];
+    foreach ($constraints as $constraint_name => $options) {
+      $this->propertyConstraints[$name][$constraint_name] = $options;
     }
-    else {
-      $item_constraints[$name] = $constraints;
-    }
-    $this->getItemDefinition()->addConstraint('ComplexData', $item_constraints);
+
+    // Reset the field item definition so the next time it is instantiated it
+    // will receive the new constraints.
+    $this->itemDefinition = NULL;
+
     return $this;
   }
 

core/modules/field/tests/modules/field_test/field_test.module

@@ -151,15 +151,18 @@ function field_test_entity_extra_field_info_alter(&$info) {
  * Implements hook_entity_bundle_field_info_alter().
  */
 function field_test_entity_bundle_field_info_alter(&$fields, EntityTypeInterface $entity_type, $bundle) {
-  if (($field_name = \Drupal::state()->get('field_test_set_constraint', FALSE)) && $entity_type->id() == 'entity_test' && $bundle == 'entity_test' && !empty($fields[$field_name])) {
+  if (($field_name = \Drupal::state()->get('field_test_constraint', FALSE)) && $entity_type->id() == 'entity_test' && $bundle == 'entity_test' && !empty($fields[$field_name])) {
+    // Set a property constraint using
+    // \Drupal\Core\Field\FieldConfigInterface::setPropertyConstraints().
     $fields[$field_name]->setPropertyConstraints('value', [
-      'Range' => [
-        'min' => 0,
-        'max' => 32,
+      'TestField' => [
+        'value' => -2,
+        'message' => t('%name does not accept the value @value.', ['%name' => $field_name, '@value' => -2]),
       ],
     ]);
-  }
-  if (($field_name = \Drupal::state()->get('field_test_add_constraint', FALSE)) && $entity_type->id() == 'entity_test' && $bundle == 'entity_test' && !empty($fields[$field_name])) {
+
+    // Add a property constraint using
+    // \Drupal\Core\Field\FieldConfigInterface::addPropertyConstraints().
     $fields[$field_name]->addPropertyConstraints('value', [
       'Range' => [
         'min' => 0,

core/modules/field/tests/src/Kernel/FieldCrudTest.php

@@ -12,6 +12,8 @@
 /**
  * Create field entities by attaching fields to entities.
  *
+ * @coversDefaultClass \Drupal\Core\Field\FieldConfigBase
+ *
  * @group field
  */
 class FieldCrudTest extends FieldKernelTestBase {
@@ -64,10 +66,6 @@ public function setUp() {
    * Test the creation of a field.
    */
   public function testCreateField() {
-    // Set a state flag so that field_test.module knows to add an in-memory
-    // constraint for this field.
-    \Drupal::state()->set('field_test_add_constraint', $this->fieldStorage->getName());
-    /** @var \Drupal\Core\Field\FieldConfigInterface $field */
     $field = FieldConfig::create($this->fieldDefinition);
     $field->save();
 
@@ -100,17 +98,6 @@ public function testCreateField() {
     // Check that the denormalized 'field_type' was properly written.
     $this->assertEqual($config['field_type'], $this->fieldStorageDefinition['type']);
 
-    // Test constraints are applied. A Range constraint is added dynamically to
-    // limit the field to values between 0 and 32.
-    // @see field_test_entity_bundle_field_info_alter()
-    $this->doFieldValidationTests();
-
-    // Test FieldConfigBase::setPropertyConstraints().
-    \Drupal::state()->set('field_test_set_constraint', $this->fieldStorage->getName());
-    \Drupal::state()->set('field_test_add_constraint', FALSE);
-    \Drupal::entityManager()->clearCachedFieldDefinitions();
-    $this->doFieldValidationTests();
-
     // Guarantee that the field/bundle combination is unique.
     try {
       FieldConfig::create($this->fieldDefinition)->save();
@@ -133,6 +120,81 @@ public function testCreateField() {
     // TODO: test other failures.
   }
 
+  /**
+   * Tests setting and adding property constraints to a configurable field.
+   *
+   * @covers ::setPropertyConstraints
+   * @covers ::addPropertyConstraints
+   */
+  public function testFieldPropertyConstraints() {
+    $field = FieldConfig::create($this->fieldDefinition);
+    $field->save();
+    $field_name = $this->fieldStorage->getName();
+
+    // Test that constraints are applied to configurable fields. A TestField and
+    // a Range constraint are added dynamically to limit the field to values
+    // between 0 and 32.
+    // @see field_test_entity_bundle_field_info_alter()
+    \Drupal::state()->set('field_test_constraint', $field_name);
+
+    // Clear the field definitions cache so the new constraints added by
+    // field_test_entity_bundle_field_info_alter() are taken into consideration.
+    \Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();
+
+    // Test the newly added property constraints in the same request as when the
+    // caches were cleared. This will test the field definitions that are stored
+    // in the static cache of
+    // \Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions().
+    $this->doFieldPropertyConstraintsTests();
+
+    // In order to test a real-world scenario where the property constraints are
+    // only stored in the persistent cache of
+    // \Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions(), we need to
+    // simulate a new request by removing the 'entity_field.manager' service,
+    // thus forcing it to be re-initialized without static caches.
+    \Drupal::getContainer()->set('entity_field.manager', NULL);
+
+    // This will test the field definitions that are stored in the persistent
+    // cache by \Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions().
+    $this->doFieldPropertyConstraintsTests();
+  }
+
+  /**
+   * Tests configurable field validation.
+   *
+   * @see field_test_entity_bundle_field_info_alter()
+   */
+  protected function doFieldPropertyConstraintsTests() {
+    $field_name = $this->fieldStorage->getName();
+
+    // Check that a valid value (not -2 and between 0 and 32) doesn't trigger
+    // any violation.
+    $entity = EntityTest::create();
+    $entity->set($field_name, 1);
+    $violations = $entity->validate();
+    $this->assertCount(0, $violations, 'No violations found when in-range value passed.');
+
+    // Check that a value that is specifically restricted triggers both
+    // violations.
+    $entity->set($field_name, -2);
+    $violations = $entity->validate();
+    $this->assertCount(2, $violations, 'Two violations found when using a null and outside the range value.');
+
+    $this->assertEquals($field_name . '.0.value', $violations[0]->getPropertyPath());
+    $this->assertEquals(t('%name does not accept the value @value.', ['%name' => $field_name, '@value' => -2]), $violations[0]->getMessage());
+
+    $this->assertEquals($field_name . '.0.value', $violations[1]->getPropertyPath());
+    $this->assertEquals(t('This value should be %limit or more.', ['%limit' => 0]), $violations[1]->getMessage());
+
+    // Check that a value that is not specifically restricted but outside the
+    // range triggers the expected violation.
+    $entity->set($field_name, 33);
+    $violations = $entity->validate();
+    $this->assertCount(1, $violations, 'Violations found when using value outside the range.');
+    $this->assertEquals($field_name . '.0.value', $violations[0]->getPropertyPath());
+    $this->assertEquals(t('This value should be %limit or less.', ['%limit' => 32]), $violations[0]->getMessage());
+  }
+
   /**
    * Test creating a field with custom storage set.
    */
@@ -280,24 +342,4 @@ public function testDeleteFieldCrossDeletion() {
     $this->assertFalse(FieldStorageConfig::loadByName('entity_test', $field_storage->getName()));
   }
 
-  /**
-   * Tests configurable field validation.
-   *
-   * @see field_test_entity_bundle_field_info_alter()
-   */
-  protected function doFieldValidationTests() {
-    $entity = EntityTest::create();
-    $entity->set($this->fieldStorage->getName(), 1);
-    $violations = $entity->validate();
-    $this->assertEqual(count($violations), 0, 'No violations found when in-range value passed.');
-
-    $entity->set($this->fieldStorage->getName(), 33);
-    $violations = $entity->validate();
-    $this->assertEqual(count($violations), 1, 'Violations found when using value outside the range.');
-    $this->assertEqual($violations[0]->getPropertyPath(), $this->fieldStorage->getName() . '.0.value');
-    $this->assertEqual($violations[0]->getMessage(), t('This value should be %limit or less.', [
-      '%limit' => 32,
-    ]));
-  }
-
 }