Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Let's convert the form validation logic of nodes to validation constraints + test that. We can keep the existing form validation in place until we leverage the entity validation for forms and remove it then.
This is part of #1696648: [META] Untie content entity validation from form validation.
Work is done in the entity API sandbox in the node-validation-2002156 branch.
Comment | File | Size | Author |
---|---|---|---|
#19 | node-validation-2002156-19.patch | 6.51 KB | klausi |
#19 | node-validation-2002156-19-interdiff.txt | 1.72 KB | klausi |
#15 | node-validation-2002156-15.patch | 7.65 KB | klausi |
#15 | node-validation-2002156-15-interdiff.txt | 1.56 KB | klausi |
#13 | node-validation-2002156-13.patch | 7.47 KB | klausi |
Comments
Comment #1
BerdirTagging
Comment #2
klausiFirst patch that adds a constraint for the node title length and a test.
Looking at NodeFormController there are some open questions:
* The title is required if the content type supports the title property. How do we implement that as validation constraint?
* The node changed date must not be lower than the changed date in the DB, otherwise the form submission gets rejected. Do we want to have that as generic constraint?
* The node author must be a valid user reference, do we have an entity reference legit value constraint already somewhere?
Comment #3
BerdirFor the user reference, I think the issue that converts the entity reference field to a plugin adds a validator to check that.
Comment #4
fagoSounds reasonable!
We can conditionally add in the constraint in the title's getConstraint() method.
this should use property_constraints
Comment #5
klausiNow using property_constraints.
Implemented a node_title data type to handle the content type specific title constraint. Does not work right now because the field item is never null (always contains an array value with one key). How is the NotNull constraint supposed to work on field items?
Comment #7
klausiOk, I think I know what went wrong: if the field is empty then the field item class never gets called in PropertyContainerMetadata::accept() to validate the constraints.
So the solution is to override on the field level, not the field item level.
This patch should pass the test now.
TODO:
* node author reference validation
* node updated date validation
* checking the node edit from array for #maxlength or #required or any other restrictions
Comment #9
Berdir#2015701: Convert field type to typed data plugin for entity reference module contains code for entity reference validation.
Comment #10
klausiFixed title constraint in case we cannot load the node type.
Added constraint validator for the node changed date.
TODO:
* node author reference validation
* checking the node edit from array for #maxlength or #required or any other restrictions
Comment #11
klausiI checked the form array in NodeFormController:form() and did not find any additional constraints, except for data type and option restrictions, but those should be covered elsewhere with the data type anyway.
So the node author reference validation remains, but that is also a general data type specific constraint, which is not node specific and should also be handled elsewhere.
Comment #12
fagoMaybe call it a custom constraint, because something extra can live in the field definition also?
Or a field generating custom constraints.
Wrong docs.
You can use array access here.
Maybe call it a custom constraint, because something extra can live in the field definition also?
Or a field generating custom constraints.
Wrong docs.
You can use array access here.
What is saved as node title if the title is not required?
I'd expect 'required' => TRUE to be on the node title usually?
Looks like we could really need a way to customize/override field definitions by bundle... :/
On the issue introducing validation we agreed on checking the violation message also - so we are sure the right constraint failed.
$this->assertEqual($violations->count(), 1, 'Validation failed.');
$this->assertEqual($violations[0]->getMessage(), t('This value should not be null.'));
Comment #13
klausiFixed issues from #12.
The node title is not required per se, so we cannot specify that on the entity base properties.
Comment #14
fagoThe node title is not required per se, so we cannot specify that on the entity base properties.
Yes. But I'm wondering whether it isn't still required. Looking at the schema I think it technically is required:
So let's follow that? Make it required + provide a default value of '' i.e. you may not unset it. Then the node type setting influences the form behaviour, while at storage level a titel is required?
Without that, I think you could unset it and trigger storage exceptions.
I realized this is in the wrong namespace - it's not a data type plugin.
Comment #15
klausiOk, fine with me wither way.
Comment #16
fagoSo why do we skip required validation here then? With '' it should pass?
Comment #17
klausiSo you are saying that we don't need a custom TtitleField at all, just always make it required and let the default required implementation handle this?
If a content type has no title configured then the field would get populated with the default empty string, so I guess that would be fine too.
Could you confirm that this is what you meant before I roll a patch?
Comment #18
fagoYes, exactly!
Comment #19
klausiHere we go.
Comment #21
klausi#19: node-validation-2002156-19.patch queued for re-testing.
Comment #22
moshe weitzman CreditAttribution: moshe weitzman commented#19: node-validation-2002156-19.patch queued for re-testing.
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedCode looks good and retest is green.
Comment #24
alexpottRetitled as we're not actually doing any conversion here as the conversions will occur when entity forms support validation.
Committed 2ea2cf6 and pushed to 8.x. Thanks!
Comment #25.0
(not verified) CreditAttribution: commentedadded meta issue, development branch.