WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#28637 closed defect (bug) (wontfix)

get_theme_mod returns empty string instead of default

Reported by: philipjohn Owned by: valendesigns
Milestone: Priority: normal
Severity: normal Version: 3.9.1
Component: Customize Keywords: has-patch has-unit-tests close
Focuses: administration Cc:

Description

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

Attachments (9)

28637a.diff (450 bytes) - added by philipjohn 5 years ago.
First pass at a patch
28637b.diff (450 bytes) - added by philipjohn 5 years ago.
empty() not isset()
28637c.diff (542 bytes) - added by philipjohn 5 years ago.
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.
28637d.diff (540 bytes) - added by philipjohn 5 years ago.
28637.1.diff (532 bytes) - added by MikeHansenMe 5 years ago.
Updated with tabs instead of spaces
28637.2.diff (614 bytes) - added by valendesigns 5 years ago.
Changed to be compliant with the Handbook Brace Style.
28637-general-unit-tests.diff (1.2 KB) - added by MikeHansenMe 4 years ago.
General theme_mod unit tests
patch-27023.diff (591 bytes) - added by ivanblagdan 4 years ago.
A test that fails without the 28637-2 patch
28637.3.diff (2.7 KB) - added by valendesigns 4 years ago.

Download all attachments as: .zip

Change History (37)

@philipjohn
5 years ago

First pass at a patch

#1 @obenland
5 years ago

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.

#2 @nofearinc
5 years ago

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?

#3 @MikeHansenMe
5 years ago

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

#4 @philipjohn
5 years ago

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
5 years ago

empty() not isset()

@philipjohn
5 years ago

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.

#5 @philipjohn
5 years ago

  • Keywords has-patch added

@philipjohn
5 years ago

#6 @philipjohn
5 years ago

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

#7 @obenland
5 years ago

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.

#8 @philipjohn
5 years ago

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
5 years ago

Updated with tabs instead of spaces

@valendesigns
5 years ago

Changed to be compliant with the Handbook Brace Style.

#9 @MikeHansenMe
4 years ago

  • Milestone changed from Awaiting Review to 4.4

@valendesigns patch still applies.

#10 @obenland
4 years ago

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?

#11 @valendesigns
4 years ago

  • Keywords needs-unit-tests added
  • Owner set to valendesigns
  • Status changed from new to assigned

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.

#12 @MikeHansenMe
4 years ago

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.

#13 @obenland
4 years ago

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.

#14 @MikeHansenMe
4 years ago

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.

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

#15 @valendesigns
4 years ago

@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.

#16 @MikeHansenMe
4 years ago

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

#17 @valendesigns
4 years ago

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.

#18 @MikeHansenMe
4 years ago

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
4 years ago

General theme_mod unit tests

#19 @MikeHansenMe
4 years ago

  • Keywords needs-unit-tests removed

Added some theme_mod unit tests.

#20 @wonderboymusic
4 years ago

In 33812:

Add some theme mod unit tests.

Props MikeHansenMe.
See #28637.

#21 @wonderboymusic
4 years ago

  • Keywords needs-unit-tests added

we need tests that fail and demonstrate the bug

@ivanblagdan
4 years ago

A test that fails without the 28637-2 patch

#22 @ivanblagdan
4 years ago

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
4 years ago

#23 follow-up: @valendesigns
4 years ago

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.

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


4 years ago

#25 @SergeyBiryukov
4 years ago

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

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

#26 in reply to: ↑ 23 ; follow-up: @obenland
4 years ago

  • Keywords close added; dev-feedback removed

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.

#27 in reply to: ↑ 26 @kuus
4 years ago

I totally agree with @obenland for what it worth.

#28 @westonruter
4 years ago

  • Milestone 4.4 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.