Skip to content

Give email/url/phone/number fields their own invalid-message copy #3495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vivi-the-going-merry wants to merge 3 commits into
base: master
Choose a base branch
from
+91 −4
Open

Give email/url/phone/number fields their own invalid-message copy #3495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec94d76
Give email/url/phone/number fields their own invalid-message copy
vivi-the-going-merry[bot] Sep 25, 2026
5924625
Simplify: reuse FrmField::get_field_type(), lookup array over switch,…
vivi-the-going-merry[bot] Sep 25, 2026
b742211
Address Franky's non-blocking review notes: quantity fields (FrmField…
vivi-the-going-merry[bot] Sep 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions classes/helpers/FrmFieldsHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ private static function fill_cleared_strings( $field, array &$field_array ) {
$frm_settings = FrmAppHelper::get_settings();
$field_array['invalid'] = $frm_settings->re_msg;
} else {
$field_array['invalid'] = self::default_invalid_msg();
$field_array['invalid'] = self::default_invalid_msg( $field );
}
}

Expand All @@ -304,11 +304,36 @@ private static function fill_cleared_strings( $field, array &$field_array ) {
}

/**
* Default "invalid" validation message. Gives field-type-specific correction guidance
* for field types where the format requirement isn't obvious from the label alone
* (WCAG 3.3.1/3.3.3), and falls back to a generic message for every other type.
*
* @since 6.8.3
* @since 6.35 Added the $field param for a type-specific message.
*
* @param array|object|null $field Optional. Field to check the type of.
*
* @return string
*/
public static function default_invalid_msg() {
public static function default_invalid_msg( $field = null ) {

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$type = $field ? FrmField::get_field_type( $field ) : '';
$messages = array(
/* translators: %s: [field_name] shortcode (Which gets replaced by a Field Name) */
'email' => sprintf( __( '%s is invalid. Enter a valid email address, like name@example.com', 'formidable' ), '[field_name]' ),
/* translators: %s: [field_name] shortcode (Which gets replaced by a Field Name) */
'url' => sprintf( __( '%s is invalid. Enter a valid web address, like https://example.com', 'formidable' ), '[field_name]' ),
/* 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]' ),

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
// Quantity validates identically to number (FrmFieldQuantity extends FrmFieldNumber) but is a distinct stored type.
$messages['quantity'] = $messages['number'];

if ( isset( $messages[ $type ] ) ) {
return $messages[ $type ];
}

/* translators: %s: [field_name] shortcode (Which gets replaced by a Field Name) */
return sprintf( __( '%s is invalid', 'formidable' ), '[field_name]' );
}
Expand Down Expand Up @@ -465,8 +490,7 @@ public static function get_error_msg( $field, $error ) {
),
'invalid' => array(
'full' => __( 'This field is invalid', 'formidable' ),
/* translators: %s: Field name */
'part' => sprintf( __( '%s is invalid', 'formidable' ), '[field_name]' ),
'part' => self::default_invalid_msg( $field ),
),
'blank' => array(
'full' => $frm_settings->blank_msg,
Expand Down
63 changes: 63 additions & 0 deletions tests/phpunit/fields/test_FrmFieldsHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,4 +310,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 );
}

/**
* @covers FrmFieldsHelper::get_error_msg
* @covers FrmFieldsHelper::default_invalid_msg
*/
public function test_get_error_msg_invalid_is_field_type_specific() {
$form_id = $this->factory->form->create();

// Email, url, phone, number, and quantity 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(

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
),

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

array(
'type' => 'email',
'name' => 'Email',
'expected' => 'Email is invalid. Enter a valid email address, like name@example.com',
),
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',
),
array(
'type' => 'number',
'name' => 'Age',
'expected' => 'Age is invalid. Enter a number',
),
array(
'type' => 'quantity',
'name' => 'Amount',
'expected' => 'Amount is invalid. Enter a number',
),
array(
'type' => 'text',
'name' => 'Comment',
'expected' => 'Comment is invalid',
),
);

foreach ( $tests as $test ) {
$field = $this->factory->field->create_and_get(
array(
'name' => $test['name'],
'form_id' => $form_id,
'type' => $test['type'],
)
);

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

// 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' );
$this->assertSame( 'Please fix Comment', $error_message );
}
}
Loading