Problem/Motivation

FieldConfigBase::setPropertyConstraints() and addPropertyConstraints() are only working in the first request after a cache clear. In every subsequent request, any item-level property constraints added through hook_entity_bundle_field_info_alter() are ignored.

Proposed resolution

Make \Drupal\Core\Field\FieldConfigBase store a list of property constraints in the persistent cache of \Drupal\Core\Entity\EntityFieldManager::getFieldDefinitions and use it when needed.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Original report by @g089h515r806:

This issue is following with https://www.drupal.org/node/2247085.There is a patch in #[2247085], commited , it provide a method to make following possible:Constraints can be added to configurable fields.

My question is :
1, does setPropertyConstraints support config field?
2, if it support, how to make it work in custom module, is there any limitation to use it for config field in custom module?
3, if it does not support config field, add this to the document.

I test setPropertyConstraints with config field following the document, https://www.drupal.org/node/2420295,
It does not work at all.

How to reproduce it:
1, install drupal8.0.X 12beta,
2,Add a field "field_test" which is a text field to "page" content type.
3,try to add a range constraints to this field through code.
It does not work at all.

Comments

g089h515r806’s picture

Issue summary: View changes
g089h515r806’s picture

Issue summary: View changes
g089h515r806’s picture

Title: Constraints cannot be added to configurable fields » does setPropertyConstraints support configurable fields?

To make my question clear.

larowlan’s picture

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

g089h515r806’s picture

My code is copy from the document, it does not work.

function field_validation_entity_bundle_field_info_alter(&$fields, \Drupal\Core\Entity\EntityTypeInterface $entity_type, $bundle) {
  if ($entity_type->id() == 'node' && $bundle == 'page') {
    $fields['field_test']->addPropertyConstraints('value', [
      'Range' => [
        'min' => 0,
        'max' => 32,
      ],
    ]);
  }
}

it is the same with field_test_entity_bundle_field_info_alter.

g089h515r806’s picture

FileSize
1.22 KB

The custom module I use. delete the test message.

thtas’s picture

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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pawel_r’s picture

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).

amateescu’s picture

Title: does setPropertyConstraints support configurable fields? » FieldConfigBase::setPropertyConstraints() and addPropertyConstraints() are broken
Version: 8.3.x-dev » 8.4.x-dev
Category: Support request » Bug report
Priority: Normal » Major
Issue summary: View changes
Status: Active » Needs review
Related issues: +#2247085: Constraints cannot be added to configurable fields
FileSize
1.58 KB
7.82 KB
10.72 KB

@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.

The last submitted patch, 12: 2541228-test-only-minimal-changes.patch, failed testing.

The last submitted patch, 12: 2541228-test-only.patch, failed testing.

Berdir’s picture

+++ 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?

amateescu’s picture

@Berdir, that's an excellent suggestion!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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 :)

The last submitted patch, 16: 2541228-16-test-only.patch, failed testing. View results

  • catch committed 75e86c9 on 8.4.x
    Issue #2541228 by amateescu, g089h515r806, Berdir, pawel_r:...

  • catch committed 50af90b on 8.3.x
    Issue #2541228 by amateescu, g089h515r806, Berdir, pawel_r:...
catch’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

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