Problem

Drupal 8 requires adding custom validation logic to be added, be defining custom constraints for fields and adding them via the respective field info alter hook. However, configuration fields do not allow adding custom hooks right now.

Proposed resolution

Add FieldConfigBase::addConstraint(), as existing for BaseFieldDefinition
Return added constraints from FieldConfigBase::getConstraints()

Remaining tasks

Do it + implement test coverage

User interface changes

-

API changes

-

This blocks #2406103: Remove hook_node_validate() and hook_node_submit() because they bypass the entity API.

Original report by @ svendecabooter

As far as I understand, it is possible to add field validation (constraints) to configurable fields upon definition of the field type.
However it does not seem to be possible to add constraints to a field that you do not define yourself.

For this to work, it would have to be possible to add constraints to configurable fields , e.g. through hook_entity_bundle_field_info_alter().

References:
http://drupal.stackexchange.com/questions/110763/d8-how-to-add-custom-va...
http://drupal.stackexchange.com/questions/107170/how-to-perform-entity-v...

I would need to read up some more before attempting to create a patch. Any feedback on the (high level) steps needed to allow for this functionality would be appreciated.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

I think it's possible globally via alteration but right now you cannot target a specific field like entity_type:entity_bundle_field.

See fago's comment http://drupal.stackexchange.com/a/107444/21641

BTW: I ended up doing the validation via the Entity::preSave method and if it won't pass it throws exception so the changes are not saved into storage.

svendecabooter’s picture

I would like to target a specific field though. Maybe that was not clear from the description above.
I'll look into the possibilities of preSave()

fago’s picture

Title: Allow to add custom constraints to configurable fields » Constraints cannot be added to configurable fields
Category: Feature request » Bug report
Issue summary: View changes
Priority: Normal » Critical

Yes. As discussed at #2406103: Remove hook_node_validate() and hook_node_submit() because they bypass the entity API this
- is required for solving #2406103: Remove hook_node_validate() and hook_node_submit() because they bypass the entity API
- is required for adding custom validation logic to the entity validation API for configurable fields

Thus, re-categorizing as critical bug.

fago’s picture

As solution, I think it's fine ok to just have a method for adding constraints, but not storing them in config. I.e., one can use the setter from the alter hook - if used earlier it won't be persisted and don't take affect. -> This needs to be documented at least.

Berdir’s picture

Storing it should it should not be too costly, we also support custom indexes, so why not? But either is fine for me.

yched’s picture

Storing means formalizing what gets saved ("settings" for each constraint) - not sure the Constraint plugins have the needed formalism ?
Also means config schema for all constraints :-/

Berdir’s picture

Ouch, schema. Ok, you win. If someone wants to store something, they can use arbitrary third party settings and build the constraint from that.

larowlan’s picture

Assigned: Unassigned » larowlan
Issue tags: +CriticalADay

poking along

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Active » Needs review
FileSize
8.35 KB

Here's a start, uses getMainPropertyName(), not totally happy with that.
But short of that we'd need to vary the interface to allow passing a property name.

Berdir’s picture

I think the suggestion above more or less was to *not* save those constraints, because if we do, then we need configuration schemas per plugin, they are no limited to simple key / (string) value pairs.

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

That is a bit fragile, not sure if that is actually going to work in all cases. But it might.

larowlan’s picture

I think I can make that fly, so we'll soon see.

larowlan’s picture

tagging for re-roll and today's office hours

larowlan’s picture

Fixes #10 tests pass, so figure it's working

yched’s picture

+++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
@@ -448,6 +455,9 @@ public function getItemDefinition() {
+      foreach ($this->constraints as $constraint) {
+        $this->itemDefinition->getPropertyDefinition($this->itemDefinition->getMainPropertyName())->addConstraint($constraint['id'], $constraint['options']);
+      }

Yeah, just relying on getMainPropertyName() here is not great.

- Looks like we at least need to be able to set constraints on any of the field's properties ?
- Not sure if we also need to allow constraint on the FieldItem (i.e the TypedData/Map of properties) as a whole ?

Sivaji_Ganesh_Jojodae’s picture

Issue tags: -Needs reroll
Berdir’s picture

Not sure if we need that flexibility, I think we changed most field specific validators like UserUniqueValidator to work on $items.

Accessing a single property from there is as easy as $items->value, accessing the field definitions, or entity is just as easy, if you need need to validate multiple items then you need a foreach, but IMHO, that tradeoff is fine if we can limit the API here to one specific use case?

larowlan’s picture

switching it to just use $items

larowlan’s picture

also, we have the ComplexData plugin to allow validation of properties on a map

larowlan’s picture

moved it to $items but added example of ComplexData to verify we can still validate properties.
This is getting close

Berdir’s picture

Ok, tried to convert the forum container leaf validation thing to this.

It works, but the problem is apparently that I am getting $item, not $itemS. I don't know why UserNameConstraint gets items, and I'm not.

Berdir’s picture

Also, that entity query thing makes no sense at all, what the hell are we supposed to do with that? Query for a term that we have already loaded? Why?

I checked D7 and it is doing the same, and it makes just as much sense there (zero).

Berdir’s picture

Also, the empty part might not be necessary, because we also have a NotNull validator on it as well.

yched’s picture

Can't say I'm following closely, but +1 to interdiff #19 (adding the constraint to the itemDefinition globally rather than to specific properties)

larowlan’s picture

It works, but the problem is apparently that I am getting $item, not $itemS. I don't know why UserNameConstraint gets items, and I'm not.

I think the answer is FieldConfigBase->getItemDefinition() returns an $item, not the list.
Can't see a method on $item (FieldItemDataDefinition) to get the list though - any clues?

Note that eg EmailItem constraints are added on the $item, not the $items, so not sure it's a deal breaker.

fago’s picture

Status: Needs review » Needs work

Thanks, reviewed the patch. Adding a property constraint is nothing special but a DX improvment around adding the ComplexData constraint. Why not just replicate that helper method on config fields as well?

Here some more remarks:

  1. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -296,18 +284,7 @@ public function setConstraints(array $constraints) {
    diff --git a/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
    
    +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
    @@ -203,4 +203,37 @@ public function getConstraints();
    +  /**
    ...
    +  public function setConstraints(array $constraints);
    ...
    +  public function addConstraint($constraint_name, $options = NULL);
    

    Why are those methods added to DataDefinitionInterface? It doesn't have setters else, so why would we make an exception for constraints?

  2. +++ b/core/modules/field/tests/modules/field_test/field_test.module
    @@ -146,3 +146,19 @@ function field_test_entity_extra_field_info_alter(&$info) {
    +function field_test_entity_bundle_field_info_alter(&$fields, \Drupal\Core\Entity\EntityTypeInterface $entity_type, $bundle) {
    

    Minor minor: Does it make sense to add a separate module for that? I find it quite hard to maintain test modules containing stuff of multiple not-related test cases. Having one module per test case that contains added glue seems to simplify that.

  3. +++ b/core/modules/forum/src/Plugin/Validation/Constraint/ForumLeafConstraint.php
    @@ -0,0 +1,24 @@
    + *   label = @Translation("Forum leaf", context = "Validation"),
    

    Should add the data type it's validation (although that's not used anywhere right now), e.g. field_item:taxonomy_term_reference

fago’s picture

also, as discussed on IRC: We need to fix addConstraint() and the setter to work the same as for base fields - that's what would one except.

larowlan’s picture

1.

Why are those methods added to DataDefinitionInterface? It doesn't have setters else, so why would we make an exception for constraints?

Seemed like the best spot, the methods existed in DataDefinition but not the interface.

Where would you suggest putting them?

2. not fussed, if pressed would change but leaving it there for now

3. will fix

4.

also, as discussed on IRC: We need to fix addConstraint() and the setter to work the same as for base fields - that's what would one except.

Please see question at #24 - how do I get the 'list/map' object from the FieldConfigBase object?

Berdir’s picture

1. Yes, because the interface is a read only interface, we do not want to define which kind of data definitions can be altered and which not . I think we should add it to FieldConfigInterface and make it specific to that. That would also be a good place to document how it can be used (in hook_entity_bundle_field_info_alter())

larowlan’s picture

FieldConfigInterface isn't implemented by DataDefinition, I find it odd that DataDefinition has those public methods but they aren't in an interface.
I get you want DataDefinitionInterface to be read-only, so perhaps we need another interface that both FieldConfigBase and DataDefinition can extend from ConstraintValidatedInterface perhaps?

Berdir’s picture

FieldConfigInterface already has setters for things that are defined in DataDefinitionInterface as getters, this is IMHO consistent with that. It's the same for BaseFieldDefinition, those setters only exist there and not on an interface.

There is an issue where that is discussed I think and if we should have an interface for that, but I think that for now, that is consistent.

Berdir’s picture

FieldConfigInterface already has setters for things that are defined in DataDefinitionInterface as getters, this is IMHO consistent with that. It's the same for BaseFieldDefinition, those setters only exist there and not on an interface.

There is an issue where that is discussed I think and if we should have an interface for that, but I think that for now, my suggestion is consistent with what we have in HEAD :)

larowlan’s picture

Keen to work on this but cannot until someone can answer #24 - anyone got any idea?

larowlan’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
12.27 KB

Thanks immensely to @Berdir who set me straight on the intent here

andypost’s picture

Looks really good!

+++ b/core/modules/forum/src/Plugin/Validation/Constraint/ForumLeafConstraint.php
@@ -0,0 +1,24 @@
+  public $noLeafMessage = 'The item %forum is a forum container, not a forum. Select one of the forums below instead.';
+}

newline nit

Berdir’s picture

the forum validation should now fail as it needs to be updated to loop over $items.

larowlan’s picture

So it passed, and it looks like we have test coverage. It passes because the magic getters call through to first() - which is fine for a single cardinality field.

Berdir’s picture

True. can a forum post be assigned to multiple times? If not, why did the old code loop?

If we can have that, we should have a test for this with multiple assignemnts, and e.g. an invalid container assignment as the second item.

If we can not, then we should at least rename $item to $items in the constraint.

larowlan’s picture

The default field has cardinality one, but is not locked, so I guess that means it is possible. Not sure if we want to support it though. So I'll add the test and the loop, as well as renaming it tomorrow.

larowlan’s picture

So I wrote some tests for multiple values, but couldn't figure out why the widget kept using single-value.
Then found this in forum.module - we explicitly set #multiple to FALSE - so that answers that question - we don't support it.

So just renamed it to $items and made some changes around that logic.

Status: Needs review » Needs work

The last submitted patch, 39: field-config-constraints-2247085.39.patch, failed testing.

Status: Needs work » Needs review
jibran’s picture

Status: Needs review » Needs work
Issue tags: +Need tests
  1. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -482,4 +489,36 @@ public function getConfig($bundle) {
    +  public function setPropertyConstraints($name, array $constraints) {
    

    We should add it to FieldConfigInterface.

  2. +++ b/core/modules/field/src/Tests/FieldCrudTest.php
    @@ -88,6 +92,21 @@ function testCreateField() {
    +    // Test constraints are applied. A NotNull constraint is added dynamically.
    

    Can we explain the constraint here?

  3. +++ b/core/modules/forum/src/Plugin/Validation/Constraint/ForumLeafConstraintValidator.php
    @@ -0,0 +1,39 @@
    +class ForumLeafConstraintValidator extends ConstraintValidator {
    

    Can we add tests for this?

larowlan’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
15.89 KB

Thanks
Had to make some changed to EntityUnitTestBase because forum is peculiar about installation order, but its a good addition for future forum tests and saves c/p most of EntityUnitTestBase::setUp().

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Great work.

yched’s picture

Minor remarks / questions, doesn't necessarily mean bumping back from RTBC :

  1. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -191,6 +191,13 @@
    +   * Array of constraint options keyed by ID.
    

    Not too clear what those IDs are ?

  2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
    @@ -482,4 +489,28 @@ public function getConfig($bundle) {
    +  public function setPropertyConstraints($name, array $constraints) {
    +    $item_constraints = $this->getItemDefinition()->getConstraints();
    +    $item_constraints['ComplexData'][$name] = $constraints;
    +    $this->getItemDefinition()->setConstraints($item_constraints);
    +    return $this;
    +  }
    

    Nice to have that as a wrapper around 'ComplexData' constraints.

    I was a bit surprised that the constraints added that way go in a different pool than $this->constraints. That's because the 'ComplexData' constraint applies to an Item, while the FieldConfig::addConstraint()/setConstraints() API is about the ItemList, right ?

    Is there a way we could :
    - add a code comment about that in setPropertyConstraints() ?
    - make that clearer in the docs for FieldConfig::addConstraint()/setConstraints() : this is about constraints on the ItemList ?

    Also, just like we have setConstraints() and addConstraint() for ItemList constraints, shouldn't we have an addPropertyConstraint($name, $constraint) that doesn't overwrite the existing constraints on the property ?

    Last, this adds an API for:
    - constraints on the ItemList
    - constraints on a property within an Item
    We leave out the intermediate level, constraints on an Item ?

  3. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
    @@ -203,4 +203,37 @@ public function getConstraints();
    +   * NOTE: This will override any previously set constraints. In most cases
    

    Nitpick on existing doc moved around, but it seems "overrides" actually means "overwrites" ?

  4. +++ b/core/modules/field/src/Entity/FieldConfig.php
    @@ -11,6 +11,7 @@
    +use Drupal\Core\TypedData\DataDefinitionInterface;
    

    Not needed ?

Berdir’s picture

2. Yes, ComplexData constraints are on the item. +1 to try and explain that better, I also think we should explicitly document that those constraints are *not* persisted in the config entity, and must be applied in e.g. hook_entity_bundle_field_info_alter().

We have the same (only support constraints on item list and properties) for base fields, I think that is fine. for custom constraints for specific fields that only have a single value, the implementation is identical, if it is multi-value, you need a simple foreach loop.

andypost’s picture

fago’s picture

Status: Reviewed & tested by the community » Needs work

I think this needs some more work, see #45,#46.

alexpott’s picture

Let's do the clean up and documentation improvements @yched suggests in #45

yched’s picture

Last item in my #45.2 :

We leave out the intermediate level, constraints on an Item ?

@Berdir's #46 works for me, fine with "only support constraints on item list and properties".

fago’s picture

for custom constraints for specific fields that only have a single value, the implementation is identical, if it is multi-value, you need a simple foreach loop.

For single-valued fields, it doesn't matter - but the constraint is better re-usable if it's written per-item. For multi-valued things, it would make more sense to put the constraint on the item instead of looping imho, that way the message would automatically be assigned to the right level.
Given that, I think we should ease adding item constraint - but that's off-topic for this issue as it applies for the existing base field validation also.

larowlan’s picture

I'll document FieldConfigBase->getItemDefinition()->addConstraint() in setPropertyConstraints and add a follow up to explore making it simpler (although I'd argue that FieldConfigBase->getItemDefinition()->addConstraint() is an API already)

larowlan’s picture

larowlan’s picture

Status: Needs work » Needs review
FileSize
11.29 KB
21.29 KB

Patch:

  • Adds FieldConfigInterface::addPropertyConstraints to compliment setPropertyConstraints
  • Adds test for addPropertyConstraints
  • Fixes docs issues requested above
  • Moves methods to the right FieldConfigInterface (the core one, not the field.module one)
  • Adds the missing methods to the interface
  • Fixes the unneeded use statement
fago’s picture

Status: Needs review » Needs work

Thanks! Here a quick review:

  1. addPropertyConstraints() makes sense, but if added it should be added to BaseFieldDefinition too. Else, leave it out for another issue (e.g. which adds a writeable-interface also...).
  2. +++ b/core/lib/Drupal/Core/Field/FieldConfigInterface.php
    @@ -65,4 +65,131 @@ public function allowBundleRename();
    +   * \Drupal\Core\Field\FieldItemList, use FieldConfigInterface::addContraint()
    

    typo, addConstraint.

  3. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
    @@ -203,4 +203,37 @@ public function getConstraints();
    +  public function setConstraints(array $constraints);
    

    Still there: This needs to go, DataDefinitionInterface is read only.

  4. +++ b/core/modules/field/src/Tests/FieldCrudTest.php
    @@ -219,4 +234,25 @@ function testDeleteFieldCrossDeletion() {
    +
    

    One NL too much.

larowlan’s picture

Status: Needs work » Needs review
FileSize
4.68 KB
23.16 KB

Fixes #55

Status: Needs review » Needs work

The last submitted patch, 56: field-config-constraints-2247085.56.patch, failed testing.

Status: Needs work » Needs review
yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @larowlan !
Looks ready to me ?

fago’s picture

Yep, thanks - changes look good. This only misses test coverage for the newly added setters, however it looks we don't have it for the others either (only implicit I'd assume). I think we can fix this in a non-critical follow-up: #2412489: Add unit tests for DataDefinition

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Need tests

@fago thanks for creating the test followup.

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

  • alexpott committed 979a252 on 8.0.x
    Issue #2247085 by larowlan, Berdir: Constraints cannot be added to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

g089h515r806’s picture

it does not work, is there any document of this feature, here is my code:

function field_validation_entity_bundle_field_info_alter(&$fields, \Drupal\Core\Entity\EntityTypeInterface $entity_type, $bundle) {
  drupal_set_message('123456');
  //123
  //print debug($fields);
  if ($entity_type->id() == 'node' && $bundle == 'page') {
     drupal_set_message('123456567');
	// drupal_set_message(var_export($bundle, true));
     //debug($fields);
	 //drupal_set_message(var_export($fields, true));
	/* $fields['field_test']->getItemDefinition()->addConstraint('Range', ['min' => 0, 'max' => 32,]); 
	*/
	//$fields['field_test']->getItemDefinition()->addConstraint('Range', ['min' => 0, 'max' => 32,]); 
	//$fields['field_test']->addConstraint('Range', ['min' => 0, 'max' => 32,]);
	/*
	$fields['field_test']->addConstraint('value', [
      'Range' => [
        'min' => 0,
        'max' => 32,
      ],
    ]); 
	*/
	
    $fields['field_test']->addPropertyConstraints('value', [
      'Range' => [
        'min' => 0,
        'max' => 32,
      ],
    ]);
	
	
  }
}

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.

$fields['field_test']->addConstraint('Range', ['min' => 0, 'max' => 32,]);

works, but it work wrong.

    $fields['field_test']->addPropertyConstraints('value', [
      'Range' => [
        'min' => 0,
        'max' => 32,
      ],
    ]);

does not work at all.

g089h515r806’s picture

Status: Closed (fixed) » Active
FileSize
1.34 KB

Is there any workable module code which I could use to make my code work correctly as the document?
My purpose is to upgrade Field validatio to Drupal8, and want to use the method provide by this issue, but it does not work.

1,maybe there is some wrong in my code.
2,The document is not clear enough
3,maybe the patch has no been full tested.

jibran’s picture

Status: Active » Closed (fixed)

Please open a new issue.

g089h515r806’s picture

Why open a new issue?
I test the code of this issue, it does not work,
the status is fixed, so I open it.

jibran’s picture

Because, it would be easier to track the problem and its priority. This has been fixed for 6 months. There are a lot of changes added to Drupal 8 core since then so the root cause can be anything. You can always post the new link issue here so that people who are following this issue can join the discussion over there.

g089h515r806’s picture

I have created a new issue,
https://www.drupal.org/node/2541228
The code is the same with the document, but it does not work.
Maybe I made some mistake, but I did a lot of work, but it still not work.

Berdir’s picture

Because this *is* fixed, it was implemented and several use cases in core use it, for example forum.module, I use it as well in custom projects. You are asking a support question, so that should be a new issue. You can post a comment to link to your issue so that people who worked on this can find it.

g089h515r806’s picture

The forum module's code is:
"
$fields['taxonomy_forums']->addConstraint('ForumLeaf', []);
"
I know how to do it in this way.

It will be convinience if we could do it in following way:

The code in the document is:

$fields['field_test']->addPropertyConstraints('value', [
      'Range' => [
        'min' => 0,
        'max' => 32,
      ],
    ]);

Or

$fields['field_test']->setPropertyConstraints('value', [
      'Range' => [
        'min' => 0,
        'max' => 32,
      ],
    ]);

Does your custom project use the sencond way, and it works? If you have, could you upload some of your code, i could following you.

Anonymous’s picture

So how is your issue behaving?

g089h515r806’s picture

This code :

$fields['field_test']->setPropertyConstraints('value', [
      'Range' => [
        'min' => 0,
        'max' => 32,
      ],
    ]);

support base field. it should works correctly.

In this issue, core developers make it works for config field.

It should work both for base field and config field now.

I test it with config field, but it does not work.
it only works with:
"
$fields['taxonomy_forums']->addConstraint('ForumLeaf', []);
"

If setPropertyConstraints works both with base field and config field, a lot of code could be reused directly, and one validator could be used across base field and config field, otherwise we need write some wrapper code in custom module.it seems that it is fixed in this issue, I test setPropertyConstraints with config field, but it does not work as my expection.

@berdir said it is fixed, But I could not find an workable example for config field with setPropertyConstraints or addPropertyConstraints method. There is no example in core, no example in custom module.

There is a Range Constraint, if we could add it to base field, to config field, through one unify UI, it will be great, otherwise, we need write a Range Constraint for config field, one for base field.

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.

sukanya.ramakrishnan’s picture

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

Berdir,

any updates to the question asked by g089h515r806? I am facing the same issue, for some reason addpropertyconstraint in the entity_bundle_field_info_alter hook doesnt work for me for config fields?

Thanks
Sukanya

andypost’s picture

Version: 9.x-dev » 8.0.0
fgm’s picture

Status: Closed (fixed) » Needs work

IMO this was closed but not fixed: the fix appears to only cover base fields, not configurable fiels as described in the issue summary. So reopening the issue which should not have been closed. Donning armour.

amateescu’s picture

Version: 8.0.0 » 8.0.x-dev
Status: Needs work » Fixed

It's true that some parts of the code introduced by this patch are broken (FieldConfigBase::setPropertyConstraints() and addPropertyConstraints()) but this is a very old issue so we need to fix it in a followup.

I just posted a patch in #2541228-12: FieldConfigBase::setPropertyConstraints() and addPropertyConstraints() are broken, reviews are welcome :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.