Make WordPress Core

Opened 19 months ago

Closed 18 months ago

Last modified 17 months ago

#56389 closed enhancement (fixed)

Make wp_required_field_indicator and wp_required_field_message filterable

Reported by: kebbet's profile kebbet Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version: 6.1
Component: General Keywords: has-patch has-unit-tests has-dev-note
Focuses: Cc:

Description

This ticket adds filters for the output of wp_required_field_indicator and wp_required_field_message.

This is a follow up to #54394 which introduced two new function for required fields: wp_required_field_indicator and wp_required_field_message.

A PR was added to the original ticket.

https://github.com/WordPress/wordpress-develop/pull/3088

Attachments (1)

56389.tests.diff (3.4 KB) - added by costdev 19 months ago.
PHPUnit Tests.

Download all attachments as: .zip

Change History (20)

This ticket was mentioned in PR #3088 on WordPress/wordpress-develop by kebbet.


19 months ago
#1

Introduce 3 filters to the 2 new wp_required_field_*-functions.
https://core.trac.wordpress.org/ticket/56389

#2 @audrasjb
19 months ago

  • Milestone changed from Awaiting Review to 6.1
  • Owner set to audrasjb
  • Status changed from new to reviewing

Thanks for opening a new ticket!

#4 @audrasjb
19 months ago

  • Keywords needs-dev-note added

costdev commented on PR #3088:


19 months ago
#5

Should we have unit tests to ensure these filters are always fired? @audrasjb

audrasjb commented on PR #3088:


19 months ago
#6

Ah yes 👍

#7 @sabernhardt
19 months ago

  • Keywords needs-unit-tests added

I need to add a comment on #23870 to check for agreement whether filtering the full markup is a better option than filtering only the glyph. And the participants on that ticket would need props here, as this would close both tickets. (I already took the translated asterisk from that ticket.)

If we do not filter the glyph on its own, we could combine the two lines, as @audrasjb suggested on PR 2982 (I had separated them specifically for the other filter):

/* translators: Character to identify required form fields. */
$indicator = '<span class="required" aria-hidden="true">' . esc_html__( '*' ) . '</span>';

Also, the wp_required_field_indicator description could be improved, perhaps like this:
"Filters the markup for a visual indicator of required form fields."

#8 @sabernhardt
19 months ago

Sample functions to test these filters:

  1. Remove aria-hidden attribute, if it stays in Core (see #55717):
add_filter( 'wp_required_field_indicator', 'wptrac_remove_aria_hidden_indicator', 10, 1 );
function wptrac_remove_aria_hidden_indicator( $indicator ) {
	$indicator = str_replace( ' aria-hidden="true"', '', $indicator );
	return $indicator;
}

add_filter( 'wp_required_field_message', 'wptrac_remove_aria_hidden_message', 10, 1 );
function wptrac_remove_aria_hidden_message( $message ) {
	$message = str_replace( ' aria-hidden="true"', '', $message );
	return $message;
}
  1. Use a non-breaking space before the indicator in the message so it does not wrap to the next line on its own.
add_filter( 'wp_required_field_message', 'wptrac_required_fields_use_nonbreaking_space', 10, 1 );
function wptrac_required_fields_use_nonbreaking_space( $message ) {
	$message = str_replace( ' <span class="required"', '&nbsp;<span class="required"', $message );
	return $message;
}

@costdev
19 months ago

PHPUnit Tests.

#9 @costdev
19 months ago

  • Keywords needs-design-feedback added; needs-unit-tests removed

@kebbet I've added PHPUnit Tests in 56389.tests.diff for these filters (and by extension, the functions themselves). Could you add them to PR 3088?

Also, @audrasjb, since these functions weren't previously covered in the test suite, if you want me to separate the creation of these test classes into separate tickets for history, let me know, and then the tests just for these filters can be added via this ticket.

Last edited 19 months ago by costdev (previous) (diff)

#10 @costdev
19 months ago

  • Keywords has-unit-tests added

#11 @kebbet
19 months ago

@costdev Your patch has been added to PR3088. Thanks!

#12 @mukesh27
19 months ago

  • Keywords commit added

The PR gets sufficient approval and is ready to merge. @costdev, could you please review the PR and provide feedback so that it can be merged?

#13 @costdev
19 months ago

I've reviewed the PR and it looks good to me!

Committers: Please note that once #55717 lands, the tests here will need to be updated to remove aria-hidden="true".

costdev commented on PR #3088:


19 months ago
#14

@SergeyBiryukov In 80ffb13, the test class name was changed from TitleCase to camelCase. In Writing PHPUnit Tests - Test Classes, it says:

Test class names should reflect the filepath, with underscores replacing directory separators and TitleCase replacing camelCase. Thus, the class Tests_Comment_GetCommentClass(), which contains tests for the get_comment_class() function, is located in tests/comment/getCommentClass.php.

I understand this commit was made for consistency. Should the handbook, or the test suite, be updated?

#15 @audrasjb
18 months ago

  • Keywords needs-design-feedback removed
  • Status changed from reviewing to accepted

Tests updated directly in the PR.
Let's ship it.

#16 @audrasjb
18 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 54137:

Comments: Make wp_required_field_indicator() and wp_required_field_message() output filterable.

This changeset introduces two new hooks:

  • wp_required_field_indicator allows developers to filter the HTML output of the wp_required_field_indicator() function.
  • wp_required_field_message does the same for the wp_required_field_message() function.

The changeset also adds new phpunit tests for these filters.

Follow-up to [53888], [54136].

Props kebbet, audrasjb, sabernhardt, costdev, mukesh27.
Fixes #56389.
See #54394.

#18 @hellofromTonya
18 months ago

#23870 was marked as a duplicate.

Note: See TracTickets for help on using tickets.