get_theme_mod returns empty string instead of default

[philipjohn]

In some instances, get_theme_mod() is returning an empty string when a value for default is provided. Given an empty string is of little use it should probably (!) be treated as false and the default returned instead.

Steps to reproduce:

• With a customiser image control available in your theme, set an image and save
• Now remove that image
• Using get_theme_mods to retrieve that setting, when passed a default, will return an empty string rather than the default

[philipjohn]

First pass at a patch

[obenland]

Thanks for bringing this to our attention, philipjohn.
Based on your steps to reproduce, it sounds like this is more of an issue with how the image information is removed, than with get_theme_mod() itself.

[nofearinc]

The patch does the following check:

    if ( isset( $mods[$name] ) && ! isset( $mods[$name] ) ) {

Since it's an expression that always returns false ( 1 && 0 ), did you mean to test for

    if ( ! empty( $mods[$name] ) ) {

or something else instead?

[MikeHansenMe]

I think obenland is right here. It needs to be unset instead of set to an empty string when it is removed.

[philipjohn]

I should have known not to be trying to do patches quickly before going to the pub! You are right, nofearinc, it should be empty not isset.

I'll look into the unsetting as per obenland's point though and see where that takes me...

[philipjohn]

empty() not isset()

[philipjohn]

Following from obenland's suggestion, I have a new patch which instead checks whether the new value for the theme mod is empty. If it is, the array index for the 'theme_mod_{$theme}' option is removed, which results in get_theme_mod() reliably returning false rather than an empty string.

[philipjohn]

Slight improvement, based on feedback from johnbillion, to be explicit about checking for an empty string, not just using empty().

[obenland]

Again, IMO this is not an issue with get_theme_mod(), but rather with a value being set (in this case an empty string) when it shouldn't.

[philipjohn]

Yep, the new patch address that directly obenland - get_theme_mod() isn't changed at all. Rather, I'm updating set_theme_mod to check if the 'new' value is an empty string. If so, it makes sure that an empty string is NOT stored in the database. Therefore, when get_theme_mod() retrieves the value, it'll return false.

[MikeHansenMe]

Updated with tabs instead of spaces

[valendesigns]

Changed to be compliant with the Handbook Brace Style.

[MikeHansenMe]

@valendesigns patch still applies.

[obenland]

I'm on the fence about this. Why would an empty string not be a valid value for a theme mod? false and 0 are, correct?
On the other hand, when would an empty string be of any use?

[valendesigns]

I think there should be some unit tests or at the very least more discussion on potential side effects. Just returning false instead of an empty sting may cause confusion in current implementations. We should acknowledge any edge cases or competing perceptions before committing.

[MikeHansenMe]

I don't think it is that empty string is not valid just that it is returning an empty string instead of the default when removed. So removing makes it an empty string instead of the default. Returning false allows the default to be used.

[obenland]

If an empty string is a valid value, why would you want to have the default returned instead?

And yes, unit tests are needed here, good point valendesigns.

[MikeHansenMe]

In the steps to reproduce they remove the image they had set. What is happening is now an empty string is being returned so whatever is depending on it is getting an empty string and likely not handling it. I think the correct behavior here is for it to be unset, then when it checks for the theme mod the default will be used.

so if someone is using get_theme_mod('thing', '​http://url-to-default-image.com/image.png' );

It will return an empty string instead of the default.

I can write a sample plugin or something to confirm this is correct but that is my understanding of it.

[valendesigns]

@MikeHansenMe I would prefer unit tests over a plugin. We need to work out assumptions and that unsetting the return value is the right thing to do.

[MikeHansenMe]

Yea. I can write unit tests. I just though a plugin would explain the use case more. I'll get something together.

[valendesigns]

The sad part is that I just did a search for get_theme_mod unit tests and there are none is isolation. There's some in the context of the Customizer getting and setting values, but not from the front-end perspective. We could use more coverage here anyhow.

Maybe a plugin will demonstrate the issue better, IDK. If you feel that's the case then do what you think is right and we'll go from there.

[MikeHansenMe]

So I think some of the confusion is because the example talks about removing an image and using the default. I think that is correct but I think we should fix that elsewhere. I see the problem now how a blanket change like this could have side effects. So we have 2 issues

1) in customizer it sets the theme_mod to an empty string when removed (need to find a core example)
2) get_theme_mod returns and empty string. (which I think is valid in other use cases, so we cant make the change in the patch)

I will write some unit tests.

[MikeHansenMe]

General theme_mod unit tests

[MikeHansenMe]

Added some theme_mod unit tests.

[wonderboymusic]

In 33812:

Add some theme mod unit tests.

Props MikeHansenMe.
See #28637.

[wonderboymusic]

we need tests that fail and demonstrate the bug

[ivanblagdan]

A test that fails without the 28637-2 patch

[ivanblagdan]

I've added a small test for the default in case theres a empty string being returned and tested the 28637-2 patch as well.

[valendesigns]

I've added a patch 28637.3.diff​ that takes into account the fact that we more than likely have old data saved and only making a change to set_theme_mod will not fix the issue at hand. When the expected result is the default value but the mod has already saved an empty value, changes in set_theme_mod will have zero effectiveness until the next time the mods are saved. So we need to also add a check in get_theme_mod for an empty string value, which was discussed earlier but I don't think we had a clear view of why it needed to happen. I've added 2 new tests that demonstrate that defaults are returned with these simple empty string checks.

I'm certain some serious debate around this patch is required, but it's now obvious to me that an empty string is an invalid value. Why would we save empty strings to the database, and why would an empty string override the default? In what logical situation can we come up with that would make an empty string valid? I'm having a difficult time thinking of one. Can anyone present a case where an empty string would be the desired result and one which false could not be used as an alternative?

The only thing I can think of that is problematic is with user assumptions. A situation where the user has explicitly checked for an empty string because they found that the function would save this kind of value. They're relying on assumptions that should never have existed in the first place. Though we do need to account for these kinds of issues it shouldn't stop us from fixing incorrect logic and rewriting the assumptions of these two functions.

[SergeyBiryukov]

@obenland: Could you take a look at 28637.3.diff​?

[obenland]

Replying to valendesigns:

why would an empty string override the default?

Because it was explicitly set at some point. It even might have been set as a way to avoid returning the default value.

In what logical situation can we come up with that would make an empty string valid? I'm having a difficult time thinking of one. Can anyone present a case where an empty string would be the desired result and one which false could not be used as an alternative?

We should not make assumptions about developers intent when they set a theme mod to an empty string. Why would an empty string not be a valid value? Setting a theme mod to anything should return that anything and not the default. IMO the default should only be returned if the theme mod was removed, which is the proper function in the API to invalidate it.

The only thing I can think of that is problematic is with user assumptions.

Yet another reason to leave it as is, it has the potential to break backwards compatibility.

[kuus]

I totally agree with @obenland for what it worth.

[westonruter]

I suppose the only path forward with this is to introduce a new theme_mod function that _does_ return the default value instead of an empty string, so that backwards-compatibility is not violated.