Make WordPress Core

Opened 9 years ago

Closed 5 years ago

Last modified 5 years ago

#34290 closed defect (bug) (fixed)

Not possible to have a percent symbol contained in the default value of a theme_mod

Reported by: kuus's profile kuus Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.4
Component: Customize Keywords: has-patch has-unit-tests
Focuses: javascript, administration Cc:

Description

For instance after having added a setting like this

$wp_customize->add_setting( 'my-id', array(
  'default' => '10%',
  'transport' => 'postMessage'
));

in the customize screen source code (in the settings JSON) is printed:

s["my-id"] = {"value":"10","transport":"postMessage","dirty":false};

the % is stripped out.
The line responsible for this is: http://git.io/vCz2R
with the sprintf function. I don't know what could be a good fix to this.
Many thanks

Attachments (1)

34290.diff (741 bytes) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (25)

#1 in reply to: ↑ description @SergeyBiryukov
9 years ago

Hi @kuus, welcome to Trac!

Thanks for the report. Have you tried escaping the value with another %, e.g. 'default' => '10%%'?

#2 @kuus
9 years ago

Hello @SergeyBiryukov, yes I thought about that but I hoped there could have been a better solution. Mainly because I am using, inside a custom control class, the $setting->default which differs from the $setting->value() even though no valued have been stored to the database. Tell me if you need more clarification. I would also like to know what @westonruter thinks about this.

#3 @moonomo
9 years ago

Just a note, I had to use option instead of theme_mod due to this for a particular setting. My use case was to introduce some predefined macro for user to input for Footer String i.e 'Copyright © %CUR_YEAR%.'. Although, it could have #CUR_YEAR# or something else, theme_mod went crazy for the % without saying anything about it.

#4 follow-up: @westonruter
9 years ago

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

Interesting issue. I'm not sure if this is something that should be accounted for in the Customizer or if it is just an idiosyncrasy for how theme mods work. Since it is get_theme_mod() that is doing the sprintf(), then I'm inclined to the former latter. The only alternative I can think of is for the Customizer to explicitly str_replace( '%', '%%', $setting->default) when it is accessing a theme mod's default value, but that could have other unintended consequences.

Would be worth a patch to explore what can be done.

#5 @kuus
9 years ago

To me the best would be to fix this issue in theme_mods API directly since that is where the problem lies. So, first of all, it might be a stupid question, but I don't understand why that sprintf is there in the get_theme_mod function. Is it really needed?

#6 @anonymized_13665966
9 years ago

@westonruter @kuus It might be worth perform a regex on the string and only carry out the string replacement if the % character comes immediately after a number?

#7 @kuus
9 years ago

@davetgreen I don't think the regex is the way to go, because the percent symbol could be anywhere in a string, it does not necessarily comes after a number.

I did a bit of research in the codebase to see where this sprintf was introduced and it seems since the beginning (2007)...here the commit that introduced the get_theme_mod function and the theme.php at that time.
@matt it seems that you wrote that function (not sure if you are the same Matt though)
I am still wondering why this sprintf, it seems to me that we could just get rid of it and solve the bug this way..what do you think?

Last edited 9 years ago by kuus (previous) (diff)

#8 @ocean90
9 years ago

#36952 was marked as a duplicate.

#9 @kuus
7 years ago

I've re-stumbled upon this issue but now the problem is almost inverse than the ticket description. Instead of having the % symbol stripped out now it is doubled..

so for instance after having added a setting like this

<?php
$wp_customize->add_setting( 'my-id', array(
  'default' => '10%',
  'transport' => 'postMessage'
));

in the customize screen source code (in the settings JSON) is printed:

s["my-id"] = {"value":"10%%","transport":"postMessage","dirty":false,"type":"theme_mod"};

..any idea? perhaps @westonruter ?

#10 @kuus
7 years ago

  • Resolution set to invalid
  • Status changed from new to closed

I think I was doing something wrong..closing as not an issue anymore apparently.

#11 @ocean90
7 years ago

  • Milestone Future Release deleted

#12 @SergeyBiryukov
7 years ago

#43824 was marked as a duplicate.

#13 in reply to: ↑ 4 @SergeyBiryukov
7 years ago

  • Milestone set to Awaiting Review
  • Resolution invalid deleted
  • Status changed from closed to reopened

Apparently still an issue, per #43824.

Replying to westonruter:

The only alternative I can think of is for the Customizer to explicitly str_replace( '%', '%%', $setting->default) when it is accessing a theme mod's default value, but that could have other unintended consequences.

Related: [24630], #21241.

#14 @daviedR
7 years ago

Facing the same issue here, my case is:

get_theme_mod( 'mykey', '100%' );

when mykey has no value, the function would return '100' instead of '100%'. The '%' character is stripped out because of the sprintf function.

In the function description:

If the modification name does not exist, then the $default will be passed through sprintf() PHP function with the first string the template directory URI and the second string the stylesheet directory URI.

Is there any reason why WordPress use sprintf to convert first and second %s into template and stylesheet directory URI?

#15 follow-up: @aristath
7 years ago

Most people that have issues with this is because we're working with CSS values and need to be able to use %.
Right now this is what the get_theme_mod function uses:

	if ( is_string( $default ) ) {
		$default = sprintf( $default, get_template_directory_uri(), get_stylesheet_directory_uri() );
	}

The sprintf there is useful... I've used it to set default values for image theme-mods where I need the default value to be an image contained in the theme and I'm too bored to type get_template_directory_url. I'm not saying it's right... It's just fast and it works.

Why don't we simply change it to this:

	if ( is_string( $default ) && ( false !== strpos( $default, '%s' ) || false !== strpos( $default, '%1&s' ) || false !== strpos( $default, '%2$s' ) ) ) {
		$default = sprintf( $default, get_template_directory_uri(), get_stylesheet_directory_uri() );
	}

f we don't want to strip % symbols then checking for %s, %1$s and %2$s before sprintf looks like a reasonable and safe solution. This way sprintf will only run if we've got a syntax where it actually makes sense to use sprintf. Otherwise this is a really annoying bug...

Last edited 7 years ago by aristath (previous) (diff)

#16 @SergeyBiryukov
6 years ago

#47645 was marked as a duplicate.

@SergeyBiryukov
6 years ago

#17 @SergeyBiryukov
6 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from reopened to accepted

34290.diff is a patch along the lines of comment:15.

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


5 years ago

#19 in reply to: ↑ 15 @noisysocks
5 years ago

Let's add unit tests here so that we have a better understanding of the edge cases involving different control sequence and random % signs.

Replying to aristath:

If we don't want to strip % symbols then checking for %s, %1$s and %2$s before sprintf looks like a reasonable and safe solution. This way sprintf will only run if we've got a syntax where it actually makes sense to use sprintf. Otherwise this is a really annoying bug...

The problem with this approach is that it doesn't work with all valid printf control sequences. For example, %100s is a valid sequence which pads the input string to be 100 characters wide.

Should we consider moving away from printf strings?

#20 @noisysocks
5 years ago

  • Keywords needs-unit-tests added

#21 @SergeyBiryukov
5 years ago

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

In 46395:

Themes: In get_theme_mod(), only run the sprintf() replacement on the default value if there's a string format pattern found in the value.

This prevents standalone percent symbols from being stripped out, e.g. in a default value like 100%.

Props aristath, kuus, moonomo, westonruter, davetgreen, daviedR, katielgc, noisysocks, SergeyBiryukov.
Fixes #34290.

#22 @SergeyBiryukov
5 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#23 @jqz
5 years ago

The fix introduces other problems - see #49421.

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


5 years ago

Note: See TracTickets for help on using tickets.