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.
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff.txt | 3.06 KB | amateescu |
#16 | 2541228-16.patch | 11.13 KB | amateescu |
#16 | 2541228-16-test-only.patch | 8.23 KB | amateescu |
#12 | 2541228.patch | 10.72 KB | amateescu |
#12 | 2541228-test-only.patch | 7.82 KB | amateescu |
Comments
Comment #1
g089h515r806 CreditAttribution: g089h515r806 commentedComment #2
g089h515r806 CreditAttribution: g089h515r806 commentedComment #3
g089h515r806 CreditAttribution: g089h515r806 commentedTo make my question clear.
Comment #4
larowlanTake a look at field_test_entity_bundle_field_info_alter() which is used to test setPropertyConstraints and addPropertyConstraints
Comment #5
g089h515r806 CreditAttribution: g089h515r806 commentedMy code is copy from the document, it does not work.
it is the same with field_test_entity_bundle_field_info_alter.
Comment #6
g089h515r806 CreditAttribution: g089h515r806 commentedThe custom module I use. delete the test message.
Comment #7
thtas CreditAttribution: thtas commentedI'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.
Comment #11
pawel_r CreditAttribution: pawel_r commentedAckward 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
- 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]]);
afteraddConstraint
- 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).Comment #12
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@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:
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 ofhook_entity_bundle_field_info_alter()
and which have theiritemDefinition
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 emptyitemDefinition
property so it will be populated by the first call to\Drupal\Core\Field\FieldConfigBase::getItemDefinition()
which doesn't add any custom constraintHere'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.
Comment #15
Berdirwhat 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?
Comment #16
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Berdir, that's an excellent suggestion!
Comment #17
BerdirI 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 :)
Comment #21
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!
Comment #23
Wim LeersOpened #2905890: FieldConfigBase does not store constraints, which is closely related.