Problem/Motivation

In the parent issue NoEntitiesExistYetWithHigherCardinality was implemented. But it needs tests. Lets implement this contraint here. While implementing joachim had a good point about the name, its about field items not entitites. Oops. New name NoFieldItemsExistWithHigherCardinality.

Steps to reproduce

Proposed resolution

The contraint should check if there is no field item of a higher cardinality than the new field settings applied. This prevents changes to field settings that would result in eather to many items in the field.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#48 image.png1021.96 KBbbrala
#40 3513035-nr-bot_d3co_yjp.txt1.5 KBneeds-review-queue-bot
#35 3513035-nr-bot_2aj2881h.txt3.72 KBneeds-review-queue-bot
#20 3513035-nr-bot.txt2.48 KBneeds-review-queue-bot
#6 3513035-nr-bot.txt3.07 KBneeds-review-queue-bot

Issue fork drupal-3513035

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bbrala created an issue. See original summary.

bbrala opened merge request !11481

bbrala’s picture

bbrala
Location Netherlands
commented
Issue tags: +Needs tests

Added the changes from the parent, lets get some working tests.

anjaliprasannan made their first commit to this issue’s fork.

anjaliprasannan’s picture

anjaliprasannan commented
Status: Active » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3513035-nr-bot.txt3.07 KB

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.

bbrala’s picture

bbrala
Location Netherlands
commented

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.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I have some small documentation remarks, but this looks great.

bbrala’s picture

bbrala
Location Netherlands
commented

Updated the comments. :)

bbrala’s picture

bbrala
Location Netherlands
commented
Issue tags: -Needs tests
smustgrave’s picture

smustgrave commented

This should probably have a CR right?

bbrala’s picture

bbrala
Location Netherlands
commented
Issue tags: +Needs change record, +Needs issue summary update

Yeah cr, is, and title updtae. Since this basically is the implementation of this constraint.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work
bbrala’s picture

bbrala
Location Netherlands
commented
Title: Implement NoEntitiesExistYetWithHigherCardinality tests » New NoEntitiesExistYetWithHigherCardinality constraint
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record, -Needs issue summary update

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.

smustgrave’s picture

smustgrave commented

Right but maybe we should.

Just to announce the new validation type that contrib modules can now use.

bbrala’s picture

bbrala
Location Netherlands
commented

Ok, added a cr

smustgrave’s picture

smustgrave commented

Added to the top of my list for tomorrow!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

I reviewed the CR. It has all the information needed.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3513035-nr-bot.txt2.48 KB

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.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Reviewed & tested by the community
quietone’s picture

quietone commented

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.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

I'm confused about the scope here - the parent issues says

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.

bbrala’s picture

bbrala
Location Netherlands
commented

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.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I don't see any tests for the validator class, only for the constraint

bbrala’s picture

bbrala
Location Netherlands
commented

Confused, the validation runs against the constraint, i try to make cases for the constraint that touch the different paths throuugh the constraint.

smustgrave’s picture

smustgrave commented

@larowlan thoughts on the last comment?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Needs work

I don't see any tests that call ::validate and ensure the constraint validator works

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review

Wow just wow, i think i mixed code because there was indeed no test.

Created a test that validates, and also updated the logic.

delta == count -1, which was not added, so it worked, but only after having 2 extra for the cardinality!

Also rebased.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs review » Needs work

Needs some small fixes in test names and such ot seems.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review

Directory name Contraint vs Constraint. I keep making that typo... ;x

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Appears feedback around the ::validate is covered in core/modules/field/tests/src/Kernel/Plugin/Validation/Constraint/NoEntitiesExistYetWithHigherCardinalityTest.php now which checks the exception message.

Believe all feedback has been addressed

dcam made their first commit to this issue’s fork.

dcam’s picture

dcam commented

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3513035-nr-bot_2aj2881h.txt3.72 KB

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.

dcam’s picture

dcam commented

Tests are failing after the rebase with two deprecation errors coming from Symfony:

Since symfony/validator 7.4: Support for evaluating options in the base Constraint class is deprecated. Initialize properties in the constructor of Drupal\field\Plugin\Validation\Constraint\NoEntitiesExistYetWithHigherCardinality instead.

Since symfony/validator 7.4: The Symfony\Component\Validator\Constraint::getDefaultOption() method is deprecated.

There is an open issue about the first one: #3555534: Since symfony/validator 7.4: Support for evaluating options in the base Constraint class is deprecated. Initialize properties in the constructor instead..

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review

The test is green locally and is unrealted.

I've refactored to the correct pattern since symfony deprecations, should be all good now. Changes were pretty minimal, but had to change tests since there were changes on how to make parameters required which made the test kinda useless.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Still LGTM

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3513035-nr-bot_d3co_yjp.txt1.5 KB

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.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

Post-bot-rebellion rebase

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

Some comments on the MR.

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review

Rebased, upgrades to new symfony and did some cleanup.

All comments are handled.

bbrala’s picture

bbrala
Location Netherlands
commented

One failure is unrealted. Other failure seems weird, database table doesn't exist. I cannot rerun pipelines right now, so that is fun.

https://git.drupalcode.org/project/drupal/-/pipelines/762176/test_report...

core/tests/Drupal/KernelTests/Core/Field/FieldStorageCreateCheckDeprecationTest.php


/**
   * Tests the field storage create check subscriber.
   */
  public function testFieldStorageCreateCheck(): void {
    $this->expectUserDeprecationMessage('Creating the "entity_test.field_test" field storage definition without the entity schema "entity_test" being installed is deprecated in drupal:11.2.0 and will be replaced by a LogicException in drupal:12.0.0. See https://www.drupal.org/node/3493981');

    FieldStorageConfig::create([
      'field_name' => 'field_test',
      'entity_type' => 'entity_test',
      'type' => 'integer',
    ])->save();
  }

So i guess the exception is real, since it is missing storage. Not sure what is best here, bring back the exception handling? So if we cant query or the query fails have the validation fail? That would end up in this code as a validation error.

bbrala’s picture

bbrala
Location Netherlands
commented
Title: New NoEntitiesExistYetWithHigherCardinality constraint » New NoFieldItemsExistWithHigherCardinality constraint
Issue summary: View changes
bbrala’s picture

bbrala
Location Netherlands
commented

We actually need the catch for the deprecation test in https://www.drupal.org/node/3475719. I also renamed to NoFieldItemsExistWithHigherCardinality since that is actually true (thanks @joachim in Slack). All is green again.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thanks for rebasing seems like OpenTelemetryNodePagePerformance is now failing :(

bbrala’s picture

bbrala
Location Netherlands
commented
StatusFileSize
new image.png1021.96 KB

Failure is becaude the orde of some id's appearantly changed... Now what? Change the order in the expected?

screenshot

godotislate’s picture

godotislate
he/him
commented

I re ran the test. Looks like the failure was intermittent. Is this ready then?

bbrala’s picture

bbrala
Location Netherlands
commented
Status: Needs work » Needs review

I do think it is to be honest. But I'm not allowed to do that ;)

(wish we would get rerun priviledges again as subsystem maintainers).

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Relooked and see new pipeline is green. So weird.. but believe may be good to go.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs review

One question on the MR.

dcam’s picture

dcam commented

I removed that exception handling per #52. The tests are passing. This is ready for another review.

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the changes @dcam. Ideally we don't want to have @todos without issues to act on them, so it's good that it was resolved.

This is good for RTBC. I think this last change is minor enough for me to commit, but I'll give it a while to see if another committer gets to it first.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I reviewed this again as well, this has decent test coverage and the validator is looking simple enough to understand what is happening.

rtbc++

godotislate’s picture

godotislate
he/him
commented
Title: New NoFieldItemsExistWithHigherCardinality constraint » Add constraint to check that the highest delta for fields items do not exceed their cardinality
godotislate’s picture

godotislate
he/him
commented
Title: Add constraint to check that the highest delta for fields items do not exceed their cardinality » Add constraint to check that the max delta for a field item list does not exceed its cardinality

  • godotislate committed 20ac0fa9 on main 
    feat: #3513035 Add constraint to check that the max delta for a field...

godotislate closed merge request !11481

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 20ac0fa and pushed to main. Thanks!

I think there are two things this will need separate 11.x MR for:

  1. Add #[HasNamedArguments] Attribute to the NoFieldItemsExistWithHigherCardinality constructor
  2. Exception handling possibly in the Validator?
godotislate’s picture

godotislate
he/him
commented

I published the CR with 12 as the versions. That will need to be updated with a backport as well.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Wow, a big step forward, and this AFAICT unblocked #3324140: #3324140-64: Convert field_storage_config and field_config's form validation logic to validation constraints 🥳

smustgrave opened merge request !16316

smustgrave’s picture

smustgrave commented

Did 60.1 but for 60.2 what exception should we throw?

smustgrave’s picture

smustgrave commented

Wanted to follow up if we still wanted to backport or fine with just main?

smustgrave’s picture

smustgrave commented

With D12 alpha out should we just leave there?

godotislate’s picture

godotislate
he/him
commented

I think we can still try to get this in.

Re #64, the exception handling I was referring to was the one removed from the main commit, see comment: https://git.drupalcode.org/project/drupal/-/merge_requests/11481#note_82.... If tests aren't failing on 11.x, then we probably don't need it.