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.
Comment | File | Size | Author |
---|---|---|---|
#65 | field_validation.zip | 1.34 KB | g089h515r806 |
#56 | field-config-constraints-2247085.56.patch | 23.16 KB | larowlan |
#56 | interdiff.txt | 4.68 KB | larowlan |
#54 | field-config-constraints-2247085.54.patch | 21.29 KB | larowlan |
#54 | interdiff.txt | 11.29 KB | larowlan |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #2
svendecabooterI 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()
Comment #3
fagoYes. 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.
Comment #4
fagoAs 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.
Comment #5
BerdirStoring it should it should not be too costly, we also support custom indexes, so why not? But either is fine for me.
Comment #6
yched CreditAttribution: yched commentedStoring 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 :-/
Comment #7
BerdirOuch, schema. Ok, you win. If someone wants to store something, they can use arbitrary third party settings and build the constraint from that.
Comment #8
larowlanpoking along
Comment #9
larowlanHere'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.
Comment #10
BerdirI 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.
Comment #11
larowlanI think I can make that fly, so we'll soon see.
Comment #12
larowlantagging for re-roll and today's office hours
Comment #13
larowlanFixes #10 tests pass, so figure it's working
Comment #14
yched CreditAttribution: yched commentedYeah, 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 ?
Comment #15
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedComment #16
BerdirNot 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?
Comment #17
larowlanswitching it to just use $items
Comment #18
larowlanalso, we have the ComplexData plugin to allow validation of properties on a map
Comment #19
larowlanmoved it to $items but added example of ComplexData to verify we can still validate properties.
This is getting close
Comment #20
BerdirOk, 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.
Comment #21
BerdirAlso, 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).
Comment #22
BerdirAlso, the empty part might not be necessary, because we also have a NotNull validator on it as well.
Comment #23
yched CreditAttribution: yched commentedCan't say I'm following closely, but +1 to interdiff #19 (adding the constraint to the itemDefinition globally rather than to specific properties)
Comment #24
larowlanI 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.
Comment #25
fagoThanks, 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:
Why are those methods added to DataDefinitionInterface? It doesn't have setters else, so why would we make an exception for constraints?
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.
Should add the data type it's validation (although that's not used anywhere right now), e.g. field_item:taxonomy_term_reference
Comment #26
fagoalso, 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.
Comment #27
larowlan1.
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.
Please see question at #24 - how do I get the 'list/map' object from the FieldConfigBase object?
Comment #28
Berdir1. 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())
Comment #29
larowlanFieldConfigInterface 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?
Comment #30
BerdirFieldConfigInterface 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.
Comment #31
BerdirFieldConfigInterface 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 :)
Comment #32
larowlanKeen to work on this but cannot until someone can answer #24 - anyone got any idea?
Comment #33
larowlanThanks immensely to @Berdir who set me straight on the intent here
Comment #34
andypostLooks really good!
newline nit
Comment #35
Berdirthe forum validation should now fail as it needs to be updated to loop over $items.
Comment #36
larowlanSo 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.
Comment #37
BerdirTrue. 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.
Comment #38
larowlanThe 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.
Comment #39
larowlanSo 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.
Comment #42
jibranWe should add it to FieldConfigInterface.
Can we explain the constraint here?
Can we add tests for this?
Comment #43
larowlanThanks
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().
Comment #44
jibranGreat work.
Comment #45
yched CreditAttribution: yched commentedMinor remarks / questions, doesn't necessarily mean bumping back from RTBC :
Not too clear what those IDs are ?
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 ?
Nitpick on existing doc moved around, but it seems "overrides" actually means "overwrites" ?
Not needed ?
Comment #46
Berdir2. 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.
Comment #47
andypostComment #48
fagoI think this needs some more work, see #45,#46.
Comment #49
alexpottLet's do the clean up and documentation improvements @yched suggests in #45
Comment #50
yched CreditAttribution: yched commentedLast item in my #45.2 :
@Berdir's #46 works for me, fine with "only support constraints on item list and properties".
Comment #51
fagoFor 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.
Comment #52
larowlanI'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)
Comment #53
larowlan#2411853: Consider adding an api for easily adding constraints to FieldItem is follow up
Comment #54
larowlanPatch:
Comment #55
fagoThanks! Here a quick review:
typo, addConstraint.
Still there: This needs to go, DataDefinitionInterface is read only.
One NL too much.
Comment #56
larowlanFixes #55
Comment #59
yched CreditAttribution: yched commentedThanks @larowlan !
Looks ready to me ?
Comment #60
fagoYep, 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
Comment #61
alexpott@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!
Comment #64
g089h515r806 CreditAttribution: g089h515r806 commentedit does not work, is there any document of this feature, here is my code:
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.
works, but it work wrong.
does not work at all.
Comment #65
g089h515r806 CreditAttribution: g089h515r806 commentedIs 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.
Comment #66
jibranPlease open a new issue.
Comment #67
g089h515r806 CreditAttribution: g089h515r806 commentedWhy open a new issue?
I test the code of this issue, it does not work,
the status is fixed, so I open it.
Comment #68
jibranBecause, 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.
Comment #69
g089h515r806 CreditAttribution: g089h515r806 commentedI 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.
Comment #70
BerdirBecause 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.
Comment #71
g089h515r806 CreditAttribution: g089h515r806 commentedThe 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:
Or
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.
Comment #72
Anonymous (not verified) CreditAttribution: Anonymous commentedSo how is your issue behaving?
Comment #73
g089h515r806 CreditAttribution: g089h515r806 commentedThis code :
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.
Comment #74
sukanya.ramakrishnan CreditAttribution: sukanya.ramakrishnan commentedBerdir,
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
Comment #75
andypostComment #76
fgmIMO 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.
Comment #77
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIt's true that some parts of the code introduced by this patch are broken (
FieldConfigBase::setPropertyConstraints()
andaddPropertyConstraints()
) 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 :)