Problem/Motivation

\Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase::assertValidationErrors() was introduced in #3324150: Add validation constraints to config_entity.dependencies and currently just asserts an array of actual violation messages/validation error messages.

It'd be better to instead assert them based on the property path at which that validation error occurred, to avoid false positives. That's also what \Drupal\Tests\ckeditor5\Kernel\CKEditor5ValidationTestTrait::validatePairToViolationsArray() does.

Discovered this while working on #3341682: New config schema data type: `required_label`.

Steps to reproduce

N/A

Proposed resolution

Improve it!

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#14 3349293-14.patch11.13 KBwim leers

Issue fork drupal-3349293

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

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Parent issue: » #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs …

Wim Leers opened merge request !3691

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Active » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I like this. Once tests pass I see no real reason not to RTBC it. IMHO it might also make sense for the base class to always assert that property paths are passed as the keys, just so it's not possible to write a test that doesn't include the property paths.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Glad you like it!

IMHO it might also make sense for the base class to always assert that property paths are passed as the keys, just so it's not possible to write a test that doesn't include the property paths.

True, but the assertSame() will make it crystal clear anyway, so I don't see the benefit of making test logic more complicated if it result in almost identical DX :)

smustgrave’s picture

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

Marking it but only question is what if contrib modules are using ConfigEntityValidationTestBase. Will they now fail?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

It has only existed in 10.1.x so hasn't shipped in any release at all yet — stable or otherwise! See #3324150: Add validation constraints to config_entity.dependencies :)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

I agree with @quietone, we should document what the keys and values mean in the array.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
Related issues: +#3358739: Update coder to 8.3.18

Good call! 😊 Done.

I took advantage of #3358739: Update coder to 8.3.18 having landed a few days ago. That got Drupal core on https://www.drupal.org/project/coder/releases/8.3.18, which includes #3253472: Support advanced PHPStan data types (general, arrays).

That means it's now far easier to clearly convey that key-value pairs are expected, and what the type should be of the keys vs the values.

borisson_’s picture

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

This documentation looks great!

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

A low stakes back to NW.

Left two comments on the MR (one on an existing thread) that are simple enough that it would be fine to self-rtbc once addressed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 3349293-14.patch11.13 KB

Great catch, @bnjmnm! 😄

Tried to create an 11.x branch, and failed: Failed to create branch '3349293-improve-ConfigEntityValidationTestBase-assertValidationErrors-11x': invalid reference name '11.x'.

So continuing to push to the 10.1.x MR and posting a patch to test against 11.x.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +blocker

Also, this literally just came up at #3364109-7: Configuration schema & required values: add test coverage for `nullable: true` validation support!

See the test output at https://www.drupal.org/pift-ci-job/2681594:

1) Drupal\KernelTests\Core\Entity\EntityFormDisplayValidationTest::testConfigDependenciesValidation with data set "valid dependency types" (array(array('system.site'), array('node:some-random-uuid'), array('system'), array('stark')), array())
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    0 => 'This value should not be null.'
+)

not very helpful, right? 😳 This fixes that:

1) Drupal\KernelTests\Core\Entity\EntityFormDisplayValidationTest::testConfigDependenciesValidation with data set "valid dependency types" (array(array('system.site'), array('node:some-random-uuid'), array('system'), array('stark')), array())
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    'status' => 'This value should not be null.'
+)

👍

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

The failure on 10.1.x is a random fail in Drupal\FunctionalJavascriptTests\Ajax\ThrobberTest.

#14 is green on 11.x 👍

  • bnjmnm committed 54fad06e on 11.x 
    Issue #3349293 by Wim Leers, phenaproxima, smustgrave, longwave, bnjmnm...

  • bnjmnm committed c5a3011f on 10.1.x 
    Issue #3349293 by Wim Leers, phenaproxima, smustgrave, longwave, bnjmnm...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Fixed

Good stuff here 😎.
Committed to 11.x and cherry-picked to 10.1.x.
🏄‍♀️

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Version: 11.x-dev » 10.1.x-dev

bnjmnm closed merge request !3691

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thank you! 🙏😊

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.