WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 20 months ago

#23870 assigned enhancement

Filter Glyph for Comment Required Fields

Reported by: cais Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version: 3.5
Component: Comments Keywords: has-patch required-fields
Focuses: template Cc:

Description

Currently the comment-template.php file uses an asterisk (*) as the default glyph for required fields used in the comment_form() function. This glyph is not easily manipulated without having to essentially over-write the entire comment_form() function.

I suggest the glyph be filtered. Therefore if one wants to change it, for example, to a hash (#) symbol then they can simply filter the output; or, if for any other reason one might want to enhance the glyph visibility or utility the filter would then allow for this while minimizing the impact on the default comment form.

Attachments (7)

Filter_Glyph_for_Comment_Required_Fields.patch (2.3 KB) - added by cais 5 years ago.
23870.patch (2.3 KB) - added by SergeyBiryukov 5 years ago.
23870.2.patch (2.3 KB) - added by SergeyBiryukov 5 years ago.
23870.3.patch (2.2 KB) - added by cais 3 years ago.
Refreshed patch file for comment-template.php
23870.4.patch (2.7 KB) - added by cais 21 months ago.
refreshed patch including i18n changes
23870.5.patch (3.9 KB) - added by SergeyBiryukov 21 months ago.
23870.5.alt.patch (4.6 KB) - added by SergeyBiryukov 21 months ago.

Download all attachments as: .zip

Change History (35)

#1 follow-up: @SergeyBiryukov
5 years ago

This glyph is not easily manipulated without having to essentially over-write the entire comment_form() function.

This seems to work for me:

function change_required_fields_glyph_23870( $defaults ) {
	$defaults['fields']['author']     = str_replace( '*', '#', $defaults['fields']['author'] );
	$defaults['fields']['email']      = str_replace( '*', '#', $defaults['fields']['email'] );
	$defaults['comment_notes_before'] = str_replace( '*', '#', $defaults['comment_notes_before'] );
	return $defaults;
}
add_filter( 'comment_form_defaults', 'change_required_fields_glyph_23870' );

#2 @cais
5 years ago

Yes, that seems to work just fine. Although I still think the introduction of a direct filter for the glyph would be more inline with how the function was written and using a str_replace kluge doesn't look as pretty as would a more obviously named filter.

Either way, this solution or adding the filter would work (thus the reason for type: enhancement). Thanks for the snippet!

#3 in reply to: ↑ 1 @cais
5 years ago

Replying to SergeyBiryukov:

This seems to work for me:

To reach a point closer to my OP I would change your snippet to the following:

function required_fields_glyph_23870() {
	$glyph = apply_filters( 'comment_required_glyph_23870', '*' );
	return $glyph;
}
function change_required_fields_glyph_23870( $defaults ) {
	$defaults['fields']['author']     = str_replace( '*', required_fields_glyph_23870(), $defaults['fields']['author'] );
	$defaults['fields']['email']      = str_replace( '*', required_fields_glyph_23870(), $defaults['fields']['email'] );
	$defaults['comment_notes_before'] = str_replace( '*', required_fields_glyph_23870(), $defaults['comment_notes_before'] );
	return $defaults;
}
add_filter( 'comment_form_defaults', 'change_required_fields_glyph_23870' );

Usable but still not as clean as having the filter in core. Perhaps this can be cleaned up but that would be continuing even further off the idea I was suggesting originally of adding one line (the filtered variable) and modifying three others.

#4 @SergeyBiryukov
5 years ago

  • Version changed from trunk to 3.5

I'd suggest 'comment_form_required_field_mark' as the filter name, seems more obvious: 23870.patch.

This could also probably be an additional comment_form() argument rather than a filter: 23870.2.patch.

#5 @cais
5 years ago

I prefer '*_glyph' versus '*_mark' as I relate glyph to symbol (read: asterisk, hash, etc.) but either term is fine as they would both be used to accomplish the same goal of implementing this enhancement. I can also envision this to be used for much more than simply changing the character displayed so the naming convention may not be vitally important as is.

As to an argument rather than a filter ... I would probably think to look for a filter first (as I actually did) versus digging into the function to find the specific argument to address, but again either method is fine as both work. The argument method may be more inline with the rest of the function code even if I actually would prefer the filter myself.

#6 @SergeyBiryukov
4 years ago

  • Focuses template added

#7 @chriscct7
3 years ago

  • Keywords needs-refresh added

#8 @cais
3 years ago

  • Keywords needs-refresh removed

Going back to my original idea with the refreshed patch 23870.3.patch ...

@cais
3 years ago

Refreshed patch file for comment-template.php

This ticket was mentioned in Slack in #core-comments by rachelbaker. View the logs.


2 years ago

#10 @rachelbaker
2 years ago

  • Keywords needs-docs needs-refresh added
  • Milestone changed from Awaiting Review to Future Release

@cais is this a request related to language translations? Would a translatable string be a better fit?

This ticket was mentioned in Slack in #docs by morganestes. View the logs.


21 months ago

#12 @morganestes
21 months ago

  • Keywords reporter-feedback added

#13 @johnbillion
21 months ago

  • Component changed from Comments to I18N
  • Keywords needs-patch good-first-bug added; has-patch needs-testing needs-docs needs-refresh reporter-feedback removed

This ought to be a translatable string IMHO.

#14 @cais
21 months ago

@rachelbaker, @johnbillion - I don't think making the required "mark" part of the translation string would be the same as making it filter-able although an interesting idea to still take into consideration.

If the symbol is part of the translation string it could only be changed by over-writing the default comment strings as seen in the examples above which makes the idea of directly adding a filterable variable as moot and puts the idea of changing the symbol back on the theme or plugin developer that chooses to do so (while still allowing the translator to essentially overwrite it all the same?!)

The premise I was putting forward was to add more of a design element to the symbol versus just making it different (such as via a translation string). I do not have any specific use case where it has been changed and even though I have implemented a filterable method into one of my themes to allow the symbol to be changed I am not aware of any case where it actually has been changed, even by myself. The theme's implementation still uses the default asterisk.

I imagine the two ideas could still be put together and sorted out with a bit more refactoring but I may also have been a bit over-zealous with making "all the things" filter-able in the theme that gave me the impetus to suggest this enhancement.

#15 follow-up: @cais
21 months ago

I've refreshed and adjusted the patch to include i18n relevant changes as well.

I also added a couple more conditional checks to insure required related items are only used if actually required. For example, if (for whatever reason) the get_option( 'require_name_email' ) is false then there is no reason to have any value for $required_text.

@cais
21 months ago

refreshed patch including i18n changes

This ticket was mentioned in Slack in #docs by cais. View the logs.


21 months ago

#17 in reply to: ↑ 15 @SergeyBiryukov
21 months ago

  • Keywords has-patch added; needs-patch removed
  • Are we sure it should be a filter rather than a comment_form() argument?
  • The filter name should start with comment_form rather than comment_fields, for consistency with other comment_form() filters.
  • The new filter needs to be documented as per the WordPress PHP documentation standards.
  • I agree the default string can be translatable, I don't see a reason to alter other strings here though.

Replying to cais:

I also added a couple more conditional checks to insure required related items are only used if actually required. For example, if (for whatever reason) the get_option( 'require_name_email' ) is false then there is no reason to have any value for $required_text.

This makes the check below redundant: ( $req ? $required_text : '' ), but I agree it's better to consolidate these checks in one place. $required_text should also be an empty string if $req is set, but $req_mark is empty.

Any objections to 23870.5.patch?

#18 @cais
21 months ago

Adding the required mark as an argument does work, and the patch in general is obviously more to current standards (mea culpa, mea culpa). It seems the ticket has become more tightening up the comment form than just adding a way to easily change the required mark used ... I'm good with that, too!

#19 @SergeyBiryukov
21 months ago

  • Component changed from I18N to Comments
  • Keywords good-first-bug removed
  • Milestone changed from Future Release to 4.7

Attached a version with the argument, just in case, so we could choose from both options :)

I'm ambivalent at the moment. Most themes only have one comment form, so the argument seems cleaner.

On the other hand, if a theme has more than one form, and/or utilizes other comment_form_* filters, the filter would probably make more sense.

Moving to the milestone to make a decision. I18N is not the main part of the ticket though, so moving back to the Comments component.

#21 @afercia
21 months ago

  • Keywords required-fields added

Related: #37331

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


21 months ago

#23 @jorbin
21 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by desrosj. View the logs.


20 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


20 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


20 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


20 months ago

#28 @jbpaul17
20 months ago

  • Milestone changed from 4.7 to Future Release

Moving this out of the 4.7 milestone given today's Beta 1 window and no apparent movement on this ticket in the last two weeks.

Note: See TracTickets for help on using tickets.