FieldConfigBase::setPropertyConstraints() and addPropertyConstraints() are broken

Take a look at field_test_entity_bundle_field_info_alter() which is used to test setPropertyConstraints and addPropertyConstraints

I'm seeing similar behaviour. Perhaps this is just a documentation issue and we're missing something.

using addConstraint works, but using addPropertyConstraints seems to be ignored by the form validation.

At least for node forms which is what i'm working with.

Ackward way to reproduce (strange behavior with cache):

- new installation Drupla 8.3
- add test content type
- add text field 'field_test'
- add new node with 'field_testing' value as '12345' (submit; new node has been created)
- add new module with hook

/**
 * Implements hook_entity_bundle_field_info_alter().
 */
function testmodule_entity_bundle_field_info_alter(&$fields, \Drupal\Core\Entity\EntityTypeInterface $entity_type, $bundle) {
    if (isset($fields['field_testing'])) {
       $fields['field_testing']->addConstraint('UniqueField', []);
   }
}

- clear cache (rebuild it totally: drush cr)
- try to add new node with 'field_testing' value as '12345'
- after submit you will get proper error message: 'value already exists'

now 'magic' begins:
- add line $fields['field_test']->addPropertyConstraints('value', ['Length' => ['max' => 3]]); after addConstraint

   if (isset($fields['field_test'])) {
       $fields['field_test']->addConstraint('UniqueField', []);
       $fields['field_test']->addPropertyConstraints('value', ['Length' => ['max' => 3]]);
   }

- drush cr (cache rebuild)
- try to add new node with 'field_test' value as '12345'
- after submit you will get proper 2 error messages: 'value already exists' and 'string to long'
- click submit again - only one error is present: 'value already exists'
- drush cr (cache rebuild)
- click submit once again - proper 2 error messages: 'value already exists' and 'string to long'
- click submit again - only one error is present: 'value already exists'
- --loop as you wish--

Error message caused by addPropertyConstraints will be shown (indicating proper behavior) only in situation when cache is 'fresh' (if other error is already present).

@pawel_r is correct in his investigation, this is a very tricky issue hidden behind the two cache layers used by \Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions.

In order to understand the problem space, we need to follow @Berdir's logic from #2247085-10: Constraints cannot be added to configurable fields:

So what you would do is alter hook_entity_bundle_field_info_alter(), then the constraints exist just in those runtime objects (which are cached).

Now, keeping in mind that the constraints only exist in the persistent cache of the field definition objects, the problem is that the patch from #2247085: Constraints cannot be added to configurable fields only stores the field item list constraints in the persistent cache, via the protected \Drupal\Core\Field\FieldConfigBase::$constraints property. This is why \Drupal\Core\Field\FieldConfigBase::setConstraints() and \Drupal\Core\Field\FieldConfigBase::addConstraint behave correctly.

However, the cached field definition objects do not have any clue about field item constraints because they are not stored/persisted anywhere. This is why we're seeing the strange behavior explained in #11:

- in the first request after a cache clear, \Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions() will return "fresh" field definition objects, those that have gone through the altering process of hook_entity_bundle_field_info_alter() and which have their itemDefinition property populated with the correct item-level constraints

- in subsequent requests, \Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions() will return the field definitions from the persistent cache, which will have an empty itemDefinition property so it will be populated by the first call to \Drupal\Core\Field\FieldConfigBase::getItemDefinition() which doesn't add any custom constraint

Here's a test-only patch with minimal changes to HEAD, just to show how we need to test this in a real-world scenario (i.e. using the persistent cache, not only the static one).

For the full patch I decided to rewrite it completely so it's easier to follow exactly what is being tested.

+++ b/core/modules/field/tests/src/Kernel/FieldCrudTest.php
@@ -134,6 +121,69 @@ public function testCreateField() {
+
+    $reflection_class = new \ReflectionClass($entity_field_manager);
+    $reflection_property = $reflection_class->getProperty('fieldDefinitions');
+    $reflection_property->setAccessible(TRUE);
+    $reflection_property->setValue($entity_field_manager, []);
+

what about just removing and re-initializing the service on the container, that should have the same effect and "simulates" a different request?

Should we do the tests both before and after, to make sure it works both on the first request and later requests with a fresh static cache?

@Berdir, that's an excellent suggestion!

I think this is good to go then. For reviewers, #12 has a great explanation of what exactly is happening and why it's not working.

Also, thanks for fixing bugs that I introduced :)

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Opened #2905890: FieldConfigBase does not store constraints, which is closely related.