Problem/Motivation

WebformSubmissionResultTest::normalizedElementIdTest() is named with a Test suffix instead of a test prefix, and carries no @test / #[Test] annotation. PHPUnit only collects methods whose name starts with test (or are annotated), so this method is never run.

It is meant to be the only coverage for element-ID normalization across addValidationError(), getOrCreateValidationError() and hasValidationError() (it carries the matching @covers tags). In practice it contributes nothing.

Reviving it also exposes a behavior bug: with the method renamed so it runs, it fails at the existence check that uses the original (un-normalized) element ID, because getOrCreateValidationError() does not store the error it creates. So the test has been hiding a real defect, not just sitting idle.

Steps to reproduce

Rename the method to a test-prefixed name and run the unit test:

ddev phpunit web/modules/custom/graphql_webform/tests/src/Unit/WebformSubmissionResultTest.php

It fails with:

The element ID was normalized when checking for a validation error.
Failed asserting that false is true.
tests/src/Unit/WebformSubmissionResultTest.php (hasValidationError assertion)

Proposed resolution

Rename normalizedElementIdTest() to testNormalizedElementId() so PHPUnit runs it, then make the test pass by resolving the underlying behavior.

The failure is that getOrCreateValidationError() returns a fresh error object without storing it in $this->validationErrors (only addValidationError() stores), so hasValidationError() reports false after a get-or-create. Pick one:

  • Option A (keep model behavior): register the error through addValidationError() before asserting existence, and drop the assumption that get-or-create persists.
  • Option B (match the "or create" naming): have getOrCreateValidationError() store the created error. Note this changes isValid(), which returns false whenever $this->validationErrors is non-empty; a stored empty error would then mark the submission invalid, so isValid() would need to ignore errors that have no messages.

Remaining tasks

  • Rename the method to testNormalizedElementId().
  • Choose Option A or B and implement it in src/Model/WebformSubmissionResult.php and/or the test.
  • Confirm phpunit, phpcs and phpstan pass.
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:

Comments

pfrenssen created an issue. See original summary.

farzana.farook made their first commit to this issue’s fork.

kieran.cott made their first commit to this issue’s fork.

kieran.cott’s picture

I've gone with Option B, because I think it's better than methods are named after what they do. This is a little bit more involved than Option A but I think it'll be less confusing to maintain going forward.

kieran.cott’s picture

Status: Active » Needs review

Pipeline pass on MR 89.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

Thanks very much, test passes again!

pfrenssen’s picture

Status: Reviewed & tested by the community » Fixed

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.

Status: Fixed » Closed (fixed)

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