- Issue created by @bbrala
- Merge request !11481Add NoEntitiesExistYetWithHigherCardinality valdator from parent issue → (Open) created by bbrala
- 🇳🇱Netherlands bbrala Netherlands
Added the changes from the parent, lets get some working tests.
- First commit to issue fork.
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇳🇱Netherlands bbrala Netherlands
I did a bugfix for the validator in the parent issue: https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs?co...
That change is needed here also.
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
I have some small documentation remarks, but this looks great.
- 🇳🇱Netherlands bbrala Netherlands
Yeah cr, is, and title updtae. Since this basically is the implementation of this constraint.
- 🇳🇱Netherlands bbrala Netherlands
Title updated.
I looked at other issues regarding validation, and when there is no change in the interface (like '' becoming NULL) it seems it is not needed to have a CR as far as i can see.
- 🇺🇸United States smustgrave
Right but maybe we should.
Just to announce the new validation type that contrib modules can now use.
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
I reviewed the CR. It has all the information needed.
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇳🇿New Zealand quietone
There are no unanswered questions. I read the MR where I found the comments helpful and clear. And the CR is good to go as well. I updated credit.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I'm confused about the scope here - the parent issues says 📌 Convert field_storage_config and field_config's form validation logic to validation constraints Needs work
I've split two issues for the missing tests.
And links to this issue.
But this issue adds the constraint and no tests?
The needs tests tag was added in #3 but then removed later without any discussion I can see (apologies if I've missed something obvious).
We're adding the constraint to the schema too, so should we be adding some tests? Should we be removing something else?
I'm curious about the use of
NULL
in the call to->aggregate
- it would be good to see some tests confirming that this indeed checks all langcodes for instances where the existing delta is higher. - 🇳🇱Netherlands bbrala Netherlands
In the parent issue there were indeed 2 constraints that needed tests, and since these validation issues have been broken down a lot, i opted to create 2 issues for those contraints. But to test, you need the contraint also so it is implemented here.
Now sure what test you are missing, isnt
NoEntitiesExistYetWithHigherCardinalityTest
a test for this contraint? Seems reasonable to test like that. If you recon that is not enough coverage please let me know what you want more. - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I don't see any tests for the validator class, only for the constraint
- 🇳🇱Netherlands bbrala Netherlands
Confused, the validation runs against the constraint, i try to make cases for the constraint that touch the different paths throuugh the constraint.