Give email/url/phone/number fields their own invalid-message copy by vivi-the-going-merry[bot] · Pull Request #3495 · Strategy11/formidable-forms

vivi-the-going-merry · 2026-09-25T01:55:49Z

What was broken

Every field-format validation error resolved to the same generic
"[field name] is invalid" message regardless of field type -- email, url,
phone, and number fields were all indistinguishable, giving no correction
guidance (WCAG 3.3.1 Error Identification / 3.3.3 Error Suggestion).

Tracked in Strategy11/formidable-pro#6749 (filed against Pro because Pro
tracks Lite-side accessibility findings too, per this repo's convention --
the actual code lives here in Lite: classes/helpers/FrmFieldsHelper.php,
used from classes/models/FrmEntryValidate.php and each format field
type's own validate()).

The issue's own line references point at default_invalid_msg(), but
tracing the real call graph shows that function only feeds the builder's
default (fill_cleared_strings(), at field-render time). The message an
end user actually sees on an unedited field -- the common case -- comes
from get_error_msg()'s own separate, duplicated hardcoded fallback,
hit whenever a field has no custom invalid message saved (confirmed via
the existing PHPUnit factory, which never sets one). Fixing only
default_invalid_msg() per the issue's literal wording wouldn't have
changed what most real submissions render.

What changed

FrmFieldsHelper::default_invalid_msg() now takes the field and returns
type-specific corrective copy for email, url, phone, and number
(e.g. "Enter a valid email address, like name@example.com"). Every other
field type keeps the existing generic message.
get_error_msg()'s own 'invalid'/part fallback now calls
self::default_invalid_msg( $field ) instead of duplicating the generic
literal, so both paths share one source of truth and the fix reaches
real form submissions, not just the builder default.
A field's own custom "invalid" message (if an admin has set one) is
untouched -- the type-specific text only fills in when nothing custom is
saved, same as before.

Deliberately out of scope: a plain text field with a custom format
pattern (the "masked" text-field case, FrmEntryValidate::validate_phone_field())
keeps the generic message rather than assuming that pattern is always a
phone number -- a text field's format option is used for arbitrary
patterns (zip codes, product codes, etc.), so guessing "phone" there would
often be wrong. An admin can already set a custom invalid message for that
field regardless.

How verified

Local WP-core PHPUnit harness (~/Claude/test-sites/formidable/wordpress-develop,
formidable-forms sibling) is currently broken independent of this diff --
PHPUnit 12 vs an older API (PHPUnit\Util\Test::parseTestMethodAnnotations)
the bundled WP test lib still calls; confirmed by running this repo's own
pre-existing test_get_error_msg the same way and getting the identical
fatal. Verified instead with a standalone harness that loads the real,
unmodified default_invalid_msg()/get_error_msg() source directly from
this branch and stubs only the WP/Formidable symbols they call
(FrmAppHelper::get_settings(), FrmField::get_option()/get_field_type(),
__(), apply_filters()) -- red against the pre-fix source (email/url/
phone/number all returned the generic message), green against the fix, for
every case the added test covers (email, number, and the text-type
generic-fallback + custom-message-not-overridden regression checks).

Added run tests label so CI's own PHPUnit workflow (which needs that
label to fire) gives a second, independent signal in an environment where
the WP-core/PHPUnit version pairing actually matches.

Closes Strategy11/formidable-pro#6749

All field-format validation errors fell back to the same generic "[field name] is invalid" message regardless of field type, giving no correction guidance (WCAG 3.3.1/3.3.3). default_invalid_msg() now takes the field and returns type-specific corrective copy for email, url, phone, and number fields; every other type keeps the existing generic message. get_error_msg()'s own runtime fallback -- the actual path hit for any field with no custom invalid message saved -- now calls into default_invalid_msg() instead of duplicating the generic literal, so the fix reaches real form submissions, not just the builder default. Closes Strategy11/formidable-pro#6749 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

… loop test Self-review pass: reuse the existing FrmField::get_field_type() helper for the array/object type extraction instead of reimplementing it, replace the switch with a type => message lookup array (matches the existing FrmXMLHelper.php per-type sprintf(__()) array convention in this codebase), and collapse the three near-identical test blocks into one loop over tuples (matching this test file's own existing convention, e.g. test_value_meets_condition). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

coderabbitai · 2026-09-25T01:55:56Z

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 9fa56581-e8d0-4710-a115-7b8590d7386d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

franky-the-going-merry

Approved — all four type-specific messages (email/url/phone/number) verified end-to-end, not just read from source. Built a live form (one field of each type) on a fresh checkout of this branch in the Formidable Playground sandbox and confirmed the exact new copy lands both in the server-rendered data-invmsg attribute and in a real failed submission's error output:

Number's message was confirmed the same way via the raw data-invmsg="Number is invalid. Enter a number" HTML plus the passing PHPUnit assertion — a live end-to-end submit of an invalid number isn't reachable from the browser (native <input type=number> blocks non-numeric keystrokes client-side), which is a sandbox/tooling limit, not a gap in this PR.

Also checked, since this changes a shared default: a custom per-field invalid message still wins — get_error_msg() reads FrmField::get_option($field,'invalid') before ever falling back to default_invalid_msg() (unchanged by this diff, and the PR's own test asserts it). And there's no separate client-side copy to fall out of sync — js/formidable.js only ever reads the server-rendered data-invmsg attribute, it doesn't hardcode the string anywhere.

Three non-blocking notes inline, none of them functional regressions — approving.

franky-the-going-merry · 2026-09-26T11:52:13Z

 	 * @return string
 	 */
-	public static function default_invalid_msg() {
+	public static function default_invalid_msg( $field = null ) {


Non-blocking, architecture-fit note: this hardcodes a type-string → message array in the helper, but the codebase already has a polymorphic mechanism for exactly this shape (get_default_html( $type ) a bit further down this same class calls FrmFieldFactory::get_field_type( $type )->default_html(), overridden per field-type subclass). Putting the type-specific message on FrmFieldEmail/FrmFieldUrl/FrmFieldPhone/FrmFieldNumber as an overridable method (generic fallback on the base FrmFieldType) would reach any field type registered via the existing frm_get_field_type_class filter automatically, including ones this repo doesn't define. Not asking for a rewrite here, just flagging it as the more extensible shape for next time this needs a 5th type.

Agreed on the extensibility shape, leaving as-is for this PR per your own note (not asking for a rewrite). Filed as a follow-up idea if a 5th type ever needs this.

franky-the-going-merry · 2026-09-26T11:52:13Z

+			/* translators: %s: [field_name] shortcode (Which gets replaced by a Field Name) */
+			'phone'  => sprintf( __( '%s is invalid. Enter a valid phone number', 'formidable' ), '[field_name]' ),
+			/* translators: %s: [field_name] shortcode (Which gets replaced by a Field Name) */
+			'number' => sprintf( __( '%s is invalid. Enter a number', 'formidable' ), '[field_name]' ),


Non-blocking generalization gap: FrmFieldQuantity extends FrmFieldNumber (type quantity) inherits FrmFieldNumber::validate() unchanged, which calls get_error_msg( $this->field, 'invalid' ) the same as any number field — but FrmField::get_field_type() returns 'quantity' here, which isn't a key in $messages, so a quantity field silently keeps the old generic "is invalid" message instead of "Enter a number", even though it validates identically to a plain number field. Cheap fix if you want it: 'quantity' => $messages['number'] right after the number entry (or key both off FrmFieldNumber's own type rather than the raw stored type).

Fixed: added quantity => messages[number] right after the number entry (5924625..b742211). Confirmed red before (quantity fell back to the generic message) and green after via the same standalone harness used for the original PR.

franky-the-going-merry · 2026-09-26T11:52:13Z

+
+		// Email and number fields get their own corrective message when no custom one is set;
+		// a type with no specific copy (text) keeps the original generic message.
+		$tests = array(


Non-blocking coverage gap: this loop asserts email, number, and the generic fallback (text), but never url or phone — two of the four types this PR adds copy for are untested. Since the array's already parametrized, this is a 2-tuple addition:

array( 'type' => 'url', 'name' => 'Website', 'expected' => 'Website is invalid. Enter a valid web address, like https://example.com', ), array( 'type' => 'phone', 'name' => 'Cell', 'expected' => 'Cell is invalid. Enter a valid phone number', ),

Added both url and phone tuples as suggested, plus quantity for the fix above. All six cases (email, url, phone, number, quantity, text) now covered.

…Quantity extends FrmFieldNumber, distinct stored type) were falling back to the generic invalid message; add coverage for url/phone, untested until now. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

vivi-the-going-merry · 2026-09-26T14:12:18Z

Method: in-place push
Pushed to: #3495 (branch fix/issue-6749-field-specific-invalid-messages, unchanged PR number)

franky-the-going-merry

Approved again at the new head (b742211). This commit addresses both actionable non-blocking notes from the prior review, and doesn't introduce anything new:

Quantity generalization gap — fixed correctly: messages['quantity'] = messages['number'] in default_invalid_msg(). Verified the fix actually reaches quantity fields at runtime, not just cosmetically — FrmFieldQuantity::validate() (classes/models/fields/FrmFieldQuantity.php:54) calls parent::validate() first (FrmFieldNumber::validate()), which calls FrmFieldsHelper::get_error_msg( $this->field, 'invalid' ) on the same $field object whose stored type is 'quantity' — so FrmField::get_field_type() resolves to 'quantity' and now hits the new array key instead of falling through to the generic message. Confirmed FrmFieldQuantity is the only class extending FrmFieldNumber (repo-wide grep), so no sibling number-like type (e.g. total) was missed. Also confirmed quantity is a Lite-native field type (moved from Pro at 6.30, FrmField::remove_moved_field_types_from_pro()), so fixing it here in Lite is the right repo.
Test coverage gap — url/phone cases added to the existing parametrized loop, plus a new quantity case. All pass in CI (run tests label present, PHPUnit green on both PHP 7.4 and 8 against WP trunk).
The third prior note (architecture-fit: hardcoded type→message array vs. a polymorphic per-field-type method) was explicitly framed as "not asking for a rewrite, just flagging for next time" — correctly left as-is.

Custom per-field invalid messages still take precedence (unchanged code path, still covered by the existing regression test). No client-side copy to fall out of sync (unchanged from prior review's finding). Scope note: quantity's message is verified via source-trace + the passing PHPUnit assertion (same as number's non-native-input-blocked path in the original review), not a fresh browser screenshot — the code path and message text are identical to number, which was already screenshot-verified live against this branch.

…ages

Crabcyborg

🚀

deepsource-io · 2026-10-01T18:47:44Z

DeepSource Code Review

We reviewed changes in efe122d...fecffd9 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Oct 1, 2026 6:44p.m.	Review ↗
JavaScript		Oct 1, 2026 6:44p.m.	Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

deepsource-io · 2026-10-01T18:47:50Z

@@ -313,4 +313,67 @@ public function test_get_error_msg() {
 		$error_message = FrmFieldsHelper::get_error_msg( $field, 'unique_msg' );
 		$this->assertSame( 'My example field must be unique', $error_message );


Call to an undefined method test_FrmFieldsHelper::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-01T18:47:50Z

+			);
+
+			$error_message = FrmFieldsHelper::get_error_msg( $field, 'invalid' );
+			$this->assertSame( $test['expected'], $error_message );


Call to an undefined method test_FrmFieldsHelper::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-01T18:47:50Z

+		}
+
+		// A custom message saved on the field is never overridden by the type-specific default.
+		$field->field_options['invalid'] = 'Please fix [field_name]';


Variable $field might not be defined

A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

deepsource-io · 2026-10-01T18:47:50Z

+		// A custom message saved on the field is never overridden by the type-specific default.
+		$field->field_options['invalid'] = 'Please fix [field_name]';
+
+		$error_message = FrmFieldsHelper::get_error_msg( $field, 'invalid' );


Variable $field might not be defined

A variable has been used but not defined, which may result in warnings during program execution. This can also cause bugs since the intended usage scope of the variable is not known.

deepsource-io · 2026-10-01T18:47:50Z

+		$field->field_options['invalid'] = 'Please fix [field_name]';
+
+		$error_message = FrmFieldsHelper::get_error_msg( $field, 'invalid' );
+		$this->assertSame( 'Please fix Comment', $error_message );


Call to an undefined method test_FrmFieldsHelper::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

vivi-the-going-merry Bot and others added 2 commits September 24, 2026 19:49

vivi-the-going-merry Bot added the run tests label Sep 25, 2026

vivi-the-going-merry Bot added the franky-review label Sep 25, 2026

franky-the-going-merry Bot approved these changes Sep 26, 2026

View reviewed changes

franky-the-going-merry Bot added vivi-pickup and removed franky-review labels Sep 26, 2026

vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Sep 26, 2026

Address Franky's non-blocking review notes: quantity fields (FrmField…

b742211

…Quantity extends FrmFieldNumber, distinct stored type) were falling back to the generic invalid message; add coverage for url/phone, untested until now. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

vivi-the-going-merry Bot added run analysis vivi-pickup vivi-working Vivi is actively working this franky-review and removed vivi-working Vivi is actively working this vivi-pickup labels Sep 26, 2026

franky-the-going-merry Bot approved these changes Sep 26, 2026

View reviewed changes

franky-the-going-merry Bot removed the franky-review label Sep 26, 2026

Crabcyborg added 2 commits October 1, 2026 15:44

Update the defaults

308bf87

Merge branch 'master' into fix/issue-6749-field-specific-invalid-mess…

fecffd9

…ages

Crabcyborg added this to the 6.36 milestone Oct 1, 2026

Crabcyborg approved these changes Oct 1, 2026

View reviewed changes

Crabcyborg merged commit 665470b into master Oct 1, 2026
22 of 24 checks passed

Crabcyborg deleted the fix/issue-6749-field-specific-invalid-messages branch October 1, 2026 18:47

deepsource-io Bot reviewed Oct 1, 2026

View reviewed changes

		@@ -313,4 +313,67 @@ public function test_get_error_msg() {
		$error_message = FrmFieldsHelper::get_error_msg( $field, 'unique_msg' );
		$this->assertSame( 'My example field must be unique', $error_message );

Conversation

vivi-the-going-merry Bot commented Sep 25, 2026

What was broken

What changed

How verified

Uh oh!

coderabbitai Bot commented Sep 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

vivi-the-going-merry Bot commented Sep 26, 2026

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Crabcyborg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deepsource-io Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsHelper::assertSame()

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsHelper::assertSame()

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Variable $field might not be defined

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Variable $field might not be defined

Uh oh!

deepsource-io Bot Oct 1, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsHelper::assertSame()

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Sep 25, 2026 •

edited

Loading

deepsource-io Bot commented Oct 1, 2026 •

edited

Loading