#34290 closed defect (bug) (fixed)
Not possible to have a percent symbol contained in the default value of a theme_mod
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (25)
#1
in reply to:
↑ description
@
9 years ago
#2
@
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
@
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:
↓ 13
@
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
@
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
@
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
@
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?
#9
@
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
@
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.
#13
in reply to:
↑ 4
@
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.
#14
@
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:
↓ 19
@
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...
#17
@
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
@
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
beforesprintf
looks like a reasonable and safe solution. This waysprintf
will only run if we've got a syntax where it actually makes sense to usesprintf
. 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?
Hi @kuus, welcome to Trac!
Thanks for the report. Have you tried escaping the value with another
%
, e.g.'default' => '10%%'
?