Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Implement some entity validation + a unit test case for the test entity.
Comment | File | Size | Author |
---|---|---|---|
#40 | d8_validate.patch | 18.78 KB | fago |
#40 | d8_validate.interdiff.txt | 1.19 KB | fago |
#37 | d8_validate_interdiff.txt | 2.87 KB | fago |
#37 | d8_validate.patch | 18.97 KB | fago |
#31 | d8_validate_entity_test.patch | 18.29 KB | fago |
Comments
Comment #1
BerdirTagging
Comment #2
fagoStarting with this one.
Comment #3
fagook, here is a first working patch which already implements property Entity::validate() support for EntityNG. We'll need to improve FieldItemBase::getConstraints() to add in property_constraints for improved DX. Then complete tests for all constraints.
I made sure to have constraints for all the max-length in hook_schema(). For checking allowed values we'll need #1758622: Provide the options list of an entity field.
Comment #4
fagoun-assigning while not working on it.
Comment #6
Berdir#3: d8_validate_entity_test.patch queued for re-testing.
Comment #7
BerdirI know it's a pattern we started elsewhere but I would suggest to not use a method name that starts with assert because these are by default ignored in the backtrace when reporting the line number of the assertion, so they will all be reported for the line that calls $this->assertValidation().
Just an idea, not to be implemented here: a bundle_field with a setting for the entity type that implements the allowed values stuff when we have it and automatically knows which bundle names are valid.
Comment #8
fagoYes, a good idea!
I see - fixed.
Also completed the patch and migrated to the property_constraints key for improved DX.
Comment #9
BerdirThis should be a moved to the language_field I think, then we also get test coverage for that.
Comment #10
fagoimplemented that. We cannot use property_constraints on the type definition level though (as this is not field specific). I guess that would make sense to support with field_type plugins then.
Comment #11
fagoAnother question is whether 12 is actually feasible? I just took that over from the schema.
Comment #12
BerdirAs discussed, this looks strange ([/]), should be "."
Missing a description.
As discussed, we need some more validation here:
- Correct violation
- Correct property path
- Maybe also the message or so, at least for one of them?
Also, some boundary value tests might make sense, e.g. the exact amount and one above?
And required is also not covered.
Comment #13
klausiYou should not get the constraint manager from the global context, it should be injected to the plugin. But I guess that can be a followup issue.
Comment #14
fagoYes, we definitly should. Let's open a follow-up for allowing constraints to have dependencies injected.
Comment #15
fagoYep, and it did not work as expected as it has been applied to the field. I've added a temporary fix to have it applied to the field as a whole, but we really need to do #1928938: Improve typed data definitions of lists to clean this up.
Comment #16
BerdirOk, this looks good to go.
The important part here is being able to call validate() an $entity and it validating it based on the defined field definitions, for which this adds the necessary definitions, code and tests.
I think we might need a follow-up or two to add better validation to certain things like language (should implement the allowed values interface once it's in) and entity references should check that the target exists. And I guess we'll have to find a way to deal with auto-create references there. And @yched is working on validation stuff including a backwards compatibility layer for hook_field_validate().
Comment #17
amateescu CreditAttribution: amateescu commentedMissing newline.
Which one is it, array or string? :)
No capital D.
Contains..
Extra space here.
We don't use t() in test assertions anymore. You can either use \Drupal\Component\Utility\String::format() or preferably just sprintf().
Other than that, this validation system looks rather complex at first sight, looking forward to see a grokable documentation for it, since #1845546: Implement validation for the TypedData API has no change notice :/
Comment #18
Berdir@amateescu. The t() is correct there, it's not an assertion message, it's an assertEqual(), and getMessage() uses t(), so it actual value and expected value should be consistent. It's basically the same as a assertText(t('Something')). The other things are correct, sorry for missing that.
Comment #19
amateescu CreditAttribution: amateescu commentedOk, no reason to hold up the patch on those minor issues then.
Comment #20
amateescu CreditAttribution: amateescu commentedComment #22
amateescu CreditAttribution: amateescu commentedDon't mind the testbot, #19 contains no functional changes.
Comment #23
fago#19: d8_validate_entity_test-19.patch queued for re-testing.
Comment #24
fago#19: d8_validate_entity_test-19.patch queued for re-testing.
Comment #25
fagoRaising priority to major as this issue unlocks getting this implemented for all other entity types, thus is a blocker for moving on with #1696648: [META] Untie content entity validation from form validation.
Comment #27
das-peter CreditAttribution: das-peter commented#19: d8_validate_entity_test-19.patch queued for re-testing.
Comment #28
fagotests are green, so back to RTBC
Comment #29
fago#19: d8_validate_entity_test-19.patch queued for re-testing.
Comment #31
fagore-rolled patch, no changes.
Comment #32
fagoAll green, so back to rtbc
Comment #33
fagoImproving issue title: This really lays the foundation for doing entity validation based on symfony validator as it improves how things are integrated. Then this adds tests for that, by impementing it for the test entity. So let's call it that way.
Comment #34
fago#31: d8_validate_entity_test.patch queued for re-testing.
Comment #35
alexpottAfter some discussion in IRC.. fago said...
Comment #36
fagoalso created #2012776: [META] Improve validation constraint test coverage based upon our discussion
Comment #37
fagoUpdated patch with improved test-coverage to assert the message as well.
Comment #38
BerdirUnrelated but that looks a german-speaking person defined that message, should be "must not" :)
The second validate() call seems unnecessary, it's the same message and violation here?
Unrelated but that looks a german-speaking person defined that message, should be "must not" :)
Comment #39
BerdirComment #40
fagoYep, that made more sense when we did not check the message for each constraint. Howsoever, removed the additional call.
Maybe, it's the one provided by symfony validator (and webmozart is German speaking). We probably want to work over those messages in general. Howsoever, that's not introduced as part of this patch.
Comment #41
BerdirYes, I think this looks good now, back to RTBC if bot agrees.
Comment #43
Berdir#40: d8_validate.patch queued for re-testing.
Comment #44
BerdirTestbot file system was full, that's why it failed before. This is still RTBC.
Comment #45
alexpottCommitted 54c2e3d and pushed to 8.x. Thanks!
Comment #46
BerdirAwesome. I have no clue if we have any change notices for symfony validation already and we certainly need to document the changes that we made here. This will also help with conversions of the other entity types.
@fago: Can you start something here? Do we have a change notice already somewhere that we can extend or should we create a new one anyway?
Comment #47
yched CreditAttribution: yched commentedWoah, I was not following this issue :-)
Great work ! I'm adapting #1992138: Helper issue for "field types as TypedData Plugins" to work on top of the new ComplexDataConstraint.
I'm still seeing the bug described in #2012682: Constraints in getPropertyDefinitions generate violations without a delta, though.
(FWIW, I also opened those for the bugs I'm seeing in the "field types as TypedData" patch:
#2012662: Constraints on 'target_id' / 'tid' properties break autocomplete if applied
#2012690: [Meta] Add useful information to 'type' constraints error messages)
One thing I'm not too fond of is the fact that we standardize placing constraints in getFieldDefinitions(). This data is persistently and statically cached, this bloats memory with validation info, that is not relevant for 95% requests.
Also, not sure how it will play when we try to move those "field definitions" to the generic FieldDefinitionInterface worked on in #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets ?
Comment #48
fagoWe don't have something yet, I'll start with one.
As discussed on IRC
- that's only the case for base-fields, for configurable fields we can generate constraints out of the field settings
- there won't be many constraints, as the patch has shown it's usual just one per base field. Some of those we might be able to replace with type-defaults anyway, e.g. for uuids (#1777956: Provide a way to define default values for entity fields adds a uuid entity field)
- having just one constraint for e.g. Length in the definition is not really different to the already common practice of having one field API setting for max-length in the definition
Given that all, I don't think it will be problematic and after all that's the most sane place to put them (DX wise). If it turns out to be problematic we could still work out caching strategies that cache validation constraints separately then.
Comment #49
fagoCreated a change notice and some basic documentation:
https://drupal.org/node/2015617
https://drupal.org/node/2015613
Please review.
Comment #50
BerdirLooks like a good start to me. Let's improve the documentation as we convert more things and start using it for forms.
Wondering if we want to start a list of default validators, what they do and how to configure them, possibly also what types result in what kind of validators (e.g. the language field and the uuid field in the default values issue) ? The linked Symfony documentation is a start, but I assume we'll add more validators and the examples there aren't in the way we specify it for Typed data...
Maybe just create a stub page with a single example and then create a documentation issue for it?
Comment #51
fagoSounds good. I've done so and created #2015739: Complete validation API documentation.
Also, I realized we missed documenation for the property_constraint key - so I've added a paragraph about that.
Comment #53
xjmUntagging. Please remove the tag when the change notification task is completed.