Problem/Motivation
The existing tests cover the settings form, config schema, update 8002 and a page load smoke test. The behavior users rely on has little or no coverage: forward navigation through the progress bar, errors shown again when a page is revisited, the progress bar classes, prevent_next_validation, and the error and page visit logging. Without these, changes such as #3632737 are hard to review, and sites that depend on the markup (for example the has-errors and is-complete classes) can break without notice.
Proposed resolution
Add tests in this order, each as its own MR or alongside the issue that changes that area.
Functional
- Forward navigation: with
forward_navigationon, every page link in the progress bar can be used, moving ahead skips validation, and returning to a page shows its logged errors (WebformNavigationHandler::alterForm(),submitForm()). - Progress bar and tracker classes:
has-errors,is-completeand the current page class fromwebformnavigation_preprocess_webform_progress_tracker()andwebformnavigation_preprocess_webform_progress_bar(). prevent_next_validation: Next with an empty required field moves on when enabled, and stays on the page when disabled.- Final submit: started in #3632737 (
ValidateAllPagesTest).
Kernel
WebformNavigationHelper:logPageVisit()does not log a page twice.getErrors()returns the errors for one page or all pages.logPageErrors()maps nested element errors (parent][child) to the element's page.- Errors for a submission that is not saved yet are stashed, then logged by
postSave()throughlogStashedPageErrors().
webformnavigation_webform_presave(): enabling forward navigation setsdraft,purge,purge_daysandwizard_progress_link. Record the current behavior before it changes.- Log cleanup: deleting a submission removes its
webformnavigation_logrows (postDelete()).
Remaining tasks
- Write the tests above.
- Decide what to do with the
additional_error_messagesetting. It is saved and has a schema, but nothing reads it, so it never appears in the final submit error. Either use it (with a test) or deprecate it, in a separate issue.
User interface changes
None.
API changes
None.
Data model changes
None.
Issue fork webformnavigation-3632767
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
Comment #2
rymcveighComment #4
rymcveighThis issue is ready for review.
Comment #5
shubhamgoel commentedHi @rymcveigh
I read the whole MR diff. The MR adds tests and CI config, plus a typo fix in two templates, and I found no blocking problems.
Suggestions:
- Core version mismatch: the CI comment says Drupal ^10.3 || ^11.0, but webformnavigation.info.yml declares ^9 || ^10 || ^11. The test code uses PHP 8 features (a heredoc closing marker with indentation, static data providers), and Webform 6.x requires Drupal 10 or later. Consider narrowing core_version_requirement in a follow-up. The MR doesn't touch it.
- Package constraint: composer.json allows drupal/webform ^5.16 || ^6.0. The CI comment only says ^10.3 || ^11.0, so you should confirm that 5.x is really tested, or drop it.
Comment #6
rymcveighComment #7
rymcveighThanks for the review @shubhamgoel.
Agreed on both points: the declared support was wider than what is tested. CI installs the current Webform, 6.3.x, which requires `^10.3 || ^11.0`, so the tests run on Drupal 10 and 11. We never tested Webform 5.x or Drupal 9.
A small correction on the PHP point: the tests don't need PHP 8. The heredoc closing marker isn't indented, and the trailing comma after it and static data providers work on PHP 7.3+. The real reason to drop Drupal 9 is that it has been end-of-life since November 2023, and the current Webform no longer supports it.
Rather than a follow-up, I've made the change in this MR (and the same in webformautosave !32):
This will go out as a new minor release, with the dropped support in the release notes. Composer keeps Drupal 9 sites on the last release that supports them.
Comment #8
rymcveighComment #10
rymcveighComment #12
shubhamgoel commentedHi @rymcveigh,
Can you please review the credit for this issue?
Comment #13
rymcveighSorry @shubhamgoel,
I thought you had been listed, but I didn't realize I needed to check your name. It should be resolved now.