Problem/Motivation

Problem #1: \Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase::testRequiredPropertyValuesMissing() attempts to blindly set all required properties to NULL and assert a validation error. But if the property being set is typed (i.e enforced at a language level) then setting it to NULL makes the test fail with a TypeError and no way to reuse the base class without overriding the whole method.

Problem #2: Attempting to fix Problem #1 uncovered an issue where ConfigEntityValidationTestBase::testRequiredPropertyValuesMissing() and ConfigEntityValidationTestBase::getPropertiesWithOptionalValues() assume that all label properties have the required_label type. If it validates a property with the optional label type, it will issue a "This value should not be blank" validation error. This was found when trying to implement the suggestion in #6 of validating the config_test entity which has an optional label.

Steps to reproduce

Create a config entity with a typed property e.g.

protected string $something

Attempt to add validation tests using \Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase
see testRequiredPropertyValuesMissing blow up with a TypeError

Proposed resolution

  • Catch TypeError and move on, validation is provided at the language level.
  • Check the schema definition of an entity's label, whether it is an optional label or required_label, instead of treating all labels as required.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3526908

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

larowlan created an issue. See original summary.

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

annmarysruthy opened merge request !12298

annmarysruthy’s picture

annmarysruthy commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Nice one @annmarysruthy - I think it in terms of adding a test, making one of the properties like weight on \Drupal\config_test\Entity\ConfigTest typed and adding a config validation test for that entity-type would demonstrate the issue.

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

dcam’s picture

dcam commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Writing the test was WAY more of a pain than it should have been.

First off, I uncovered two additional bugs in this test! The class contains two mentions of the required_label data type, including where it says:

    // If a config entity type is not fully validatable, all properties are
    // optional, with the exception of `type: langcode` and
    // `type: required_label`.

Except it doesn't actually check for required_label data. In both places it just assumes that all labels are required. So if you're validating a config entity that isn't required, like config_test, then the test fails. I couldn't simply change the config_test label's data type to required_label either. When I did a bunch of tests started failing.

To solve this problem I updated ::testRequiredPropertyValuesMissing() and ::getPropertiesWithOptionalValues() to verify that a label is required before treating it that way.

Secondly, the weight couldn't simply be given an int type (see #6). Tests started failing when I did that. I tried other solutions like assigning types to other untyped properties and adding another property. You can see the commit log above. Everything I tried broke tests. It turned out that adding the type to weight broke the least amount of tests, just one. It was the ConfigEntityAdapterTest which tried to assign a string value to the weight to trigger validation errors. Of course, with the new type this can't be done at the PHP level. I changed the adapter test trigger a violation on the untyped style property instead. I don't think this is important to the test, but please double-check.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can we update the summary some. Proposed solution means just catching the exception and moving on. But seems we are doing more then that.

dcam’s picture

dcam commented
Title: \Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase::testRequiredPropertyValuesMissing isn't compatible with typed properties » Fix issues with ConfigEntityValidationTestBase
Issue summary: View changes
Status: Needs work » Needs review

Sure thing. I didn't think about it the other day. I was ready to move on to something else after hours of frustrating debugging.

smustgrave’s picture

smustgrave commented

Why is the typehint needed in core/modules/config/tests/config_test/src/Entity/ConfigTest.php ?

dcam’s picture

dcam commented

Why is the typehint needed in core/modules/config/tests/config_test/src/Entity/ConfigTest.php ?

This was done at the suggestion of @larowlan in comment #6. And it's at the core of what this issue was originally created about. From the issue summary:

[The test] attempts to blindly set all required properties to NULL and assert a validation error. But if the property being set is typed (i.e enforced at a language level) then setting it to NULL makes the test fail with a TypeError...

In order to test this problem we need to have a typed property.

smustgrave’s picture

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

Thanks for clarifying that!

Believe all feedback here has been addressed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

I've been annoyed by the original issue before where typed properties can't be used, and nice find on all these additional edge cases!

Backported to 11.3.x as an eligible tests only bug fix.

Committed and pushed fac47b829ca to 11.x and e764a353476 to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed e764a353 on 11.3.x 
    fix: #3526908 Fix issues with ConfigEntityValidationTestBase

By:...

  • longwave committed fac47b82 on 11.x 
    fix: #3526908 Fix issues with ConfigEntityValidationTestBase

By:...

longwave closed merge request !12298

Status: Fixed » Closed (fixed)

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