Make WordPress Core

Opened 4 years ago

Last modified 4 years ago

#49484 reviewing feature request

Add option 'OR' to wp_sprintf_l()

Reported by: pedromendonca's profile pedromendonca Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Formatting Keywords: has-patch
Focuses: Cc:

Description

The result of a this function is filtered by wp_sprintf_l() with ' and ' type delimiters.

Would it be awesome to include a third optional argument, a delimiter type ' or ', keeping the default ' and ' for backward compatibility.

Current use:
wp_sprintf( '%s: %l', __( 'Some cool numbers' ), array( 1, 5, 10, 15 ) );

Output:
Some cool numbers: 1, 5, 10, and 15

Example of use:
wp_sprintf( '%s: %l', __( 'Some cool numbers' ), array( 1, 5, 10, 15 ), 'OR' );

Optional output:
Some cool numbers: 1, 5, 10, or 15

Attachments (3)

49484.diff (3.9 KB) - added by pedromendonca 4 years ago.
Patch to add options to wp_sprintf_l()
49484-1.diff (4.0 KB) - added by pedromendonca 4 years ago.
Add revised patch with unnecessary spaces removed
49484-2.diff (3.9 KB) - added by pedromendonca 4 years ago.
Revised patch with fixed typo

Download all attachments as: .zip

Change History (17)

#1 @johnbillion
4 years ago

  • Component changed from I18N to Formatting
  • Keywords needs-patch added

Interesting idea.

#2 @pedromendonca
4 years ago

Some additional considerations

  1. Adding a third parameter to wp_sprintf() is not possible, because the current second parameter is ...$args, which has variable lenght and must be kept as the last parameter.
  1. A single wp_sprintf() can have multiple %l placeholders, with both ' and '/' or ', so it doesn't make sense to set it function wide.

The solution

Currently wp_sprintf() supports %l placeholder that sets a list ' and ' type delimited.
It's possible to add support for more placeholders:

  • %l$o - $o stands for OR - Outputs ' or ' delimiters
  • %l$a - $a stands for AND - Outputs ' and ' delimiters and is an alias for the default %l

The solution should allow all the below placeholder types:

  • Normal: %l$o, %l$a, %l (alias for %l$a)
  • Numbered: %2$l$o, %2$l$a, %2$l (alias for %2$l$a)

I've tested this successfully, I'll publish a patch below for everyone interested in testing this.

Last edited 4 years ago by pedromendonca (previous) (diff)

@pedromendonca
4 years ago

Patch to add options to wp_sprintf_l()

@pedromendonca
4 years ago

Add revised patch with unnecessary spaces removed

#3 @pedromendonca
4 years ago

  • Summary changed from Add option 'OR' to wp_sprintf() to Add option 'OR' to wp_sprintf_l()

#4 @pedromendonca
4 years ago

  • Keywords has-patch added; needs-patch removed

#5 @pedromendonca
4 years ago

Usage and testing

Normal and Numbered placeholders

wp_sprintf(
	'Test default: %l | Test OR: %l$o | Test AND: %l$a',
	array( 1, 3, 5, 7 ),
	array( 2, 4, 6, 8 ),
	array( 10, 20, 30, 40 )
);
wp_sprintf(
	'Test default: %1$l | Test OR: %2$l$o | Test AND: %3$l$a',
	array( 1, 3, 5, 7 ),
	array( 2, 4, 6, 8 ),
	array( 10, 20, 30, 40 )
);

Output: Test default: 1, 3, 5, and 7 | Test OR: 2, 4, 6, or 8 | Test AND: 10, 20, 30, and 40

Arrays of only 2 items

wp_sprintf(
	'Test default: %l | Test OR: %l$o | Test AND: %l$a',
	array( 1, 3 ),
	array( 2, 4 ),
	array( 10, 20 )
);

Output: Test default: 1 and 3 | Test OR: 2 or 4 | Test AND: 10 and 20

Last edited 4 years ago by pedromendonca (previous) (diff)

This ticket was mentioned in PR #161 on WordPress/wordpress-develop by pedro-mendonca.


4 years ago
#6

This ticket was mentioned in Slack in #core-i18n by pedromendonca. View the logs.


4 years ago

@pedromendonca
4 years ago

Revised patch with fixed typo

This ticket was mentioned in Slack in #polyglots by pedromendonca. View the logs.


4 years ago

#9 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.6
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#10 @TimothyBlynJacobs
4 years ago

Do we need the extra $? In other words, what about %la and %lo`. There is precedent for alphanumeric characters used as flags.

https://www.man7.org/linux/man-pages/man3/sprintf.3.html
https://en.wikipedia.org/wiki/Printf_format_string#Custom_format_placeholders

I worry that the $ will be confused with the position syntax.

#11 @pedromendonca
4 years ago

I understand, the main issue is backward compatibility.

If a plugin or theme uses %la, it will currently be parsed as a list + an ‘a’ concatenated.

If an old code uses %lall (“%l” concatenated with “all” - bad example but possible as it is the expected behavior), with the new function version the “a” would be absorbed as a parameter and cropped out from the text string.

As for numbering, the $ tells the code that the next character is a parameter, and not a text character from the string.

I think an upgrade to this function shouldn’t break existing code.

Last edited 4 years ago by pedromendonca (previous) (diff)

#12 @TimothyBlynJacobs
4 years ago

That's a good point, but when would a user use %la currently? Wouldn't that give you an invalid sentence which is the exact purpose of the l directive?

>>> wp_sprintf( 'Hello %la', [ 'James', 'Jill' ] );
=> "Hello James and Jilla"

>>> wp_sprintf( 'Hello %l$a', [ 'James', 'Jill' ] );
=> "Hello James and Jill$a"

I think someone intentionally using %la or %l$a are both quite unlikely.

#13 @helen
4 years ago

  • Milestone changed from 5.6 to Future Release

No technical consensus, punting from 5.6. Also a little confused about the $length variable and its different value between the and case and the default case - thought they were the same?

Also curious if there are any other delimiters that may be wanted? I have never personally used this and actually did not know it existed so I'd love to know more about usage.

#14 @johnbillion
4 years ago

I've used this function several times but only because I know it exists. I doubt it has any discoverability.

Note: See TracTickets for help on using tickets.