WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 20 months ago

#30988 closed defect (bug) (fixed)

Setting's default value doesn't apply on preview window

Reported by: Aniruddh Owned by: ocean90
Milestone: 4.1.1 Priority: normal
Severity: normal Version: 4.1
Component: Customize Keywords: has-patch fixed-major
Focuses: administration Cc:
PR Number:

Description

Hello,

Ever since I have upgraded my wordpress installation from 4.0 to 4.1, I am seeing the default value of my Cutsomizer's setting doesn't apply on preview window. The options table is empty and customizer's preview window is supposed to read default values I have mentioned. ( Just like it did in previous versions )

/* BG COLOR  */
$wp_customize->add_setting( 'my_settings[bg_color]', array(
	'default'        => '#CCCCCC',
	'capability'     => 'edit_theme_options',
	'type'           => 'option',
) );

I read it on frontend this way:

$my_option = get_option( 'my_settings' );
$my_option['bg_color'];

Change History (36)

#1 @westonruter
5 years ago

  • Owner set to westonruter
  • Status changed from new to assigned

#2 @westonruter
5 years ago

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

I believe this is a regression introduced by [30102] in #28580. Now that we only POST settings to the Customizer preview if they have actually been changes in them (if they're dirty), the preview filter needs to be updated to use the setting's default value if the option/theme_mod doesn't already exist.

#3 @westonruter
5 years ago

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

In 30988.diff:

  • Add $default param to WP_Customize_Manager::post_value() (and add protected WP_Customize_Manager::parse_post_data() method)
  • Update WP_Customize_Setting::_preview_filter() to use original or default value if no post_value supplied for setting

#4 @westonruter
5 years ago

I've been working on unit tests and, as usual, I found some edge cases that I didn't account for in my initial patch. Latest work can be seen here: https://github.com/xwp/wordpress-develop/pull/62/files#diff-8582bb972a2a8283eebe2050f95f3c19

#5 @westonruter
5 years ago

  • Keywords needs-unit-tests removed
  • Owner changed from westonruter to ocean90
  • Status changed from assigned to reviewing

In 30988.2.diff:

  • 9e00392 Use require_once instead of require in WP_Customize_Manager (for unit testing, but also good regardless)
  • daf6183 Update multidimensional to account for an array overriding a string or object value (otherwise fatal error can ensue)
  • 6a86c92 Store the response of value() when calling preview() for use if no post_value() (this is the key commit for this ticket)
  • Add unit tests for WP_Customize_Setting::__construct() and WP_Customize_Setting::preview(), with 139 assertions. (See also #28579)

Full changes in PR: https://github.com/xwp/wordpress-develop/pull/62/files

#6 follow-up: @Aniruddh
5 years ago

  • Keywords needs-patch added; has-patch removed

After further testing ( with patched class-wp-customize-manager.php & class-wp-customize-settings.php ) I have found out that customizer does not save the default values to the database. It only saves the values of the settings whose values are changed.

#7 in reply to: ↑ 6 @westonruter
5 years ago

  • Owner changed from ocean90 to westonruter

Replying to Aniruddh:

After further testing ( with patched class-wp-customize-manager.php & class-wp-customize-settings.php ) I have found out that customizer does not save the default values to the database. It only saves the values of the settings whose values are changed.

Thanks for discovering that. It makes sense in the case of options specifically, because update_option() will do no-op if it detects that the value being saved is identical to the value already in the DB. So I think what may be happening is that since we're filtering the DB value to be the default value, when you try save a default into the DB then it does a no-op.

#8 follow-ups: @Aniruddh
5 years ago

Sorry, I did not mention that when I tested the patch, I deleted the option row from the table.

So, I was under impression that customizer will save default values to empty table row but instead it saved values of those settings whose default values were changed.

#9 in reply to: ↑ 8 @westonruter
5 years ago

  • Keywords reporter-feedback added

Replying to Aniruddh:

Sorry, I did not mention that when I tested the patch, I deleted the option row from the table.

So, I was under impression that customizer will save default values to empty table row but instead it saved values of those settings whose default values were changed.

So you did delete_option( 'my_settings' ) before testing? And then when saving the my_settings[bg_color] setting with the value #CCCCCC the result was that no my_settings option was added? And yet if you were to try saving my_settings[bg_color] to something else, like #000000, then it succeeded in creating the my_settings option?

I don't see why this would happen according to my theory above, because the setting's preview() method doesn't get called when a customize_save WP Ajax request is made. I tried registering a setting for a non-existent option, and when I tried saving the setting with the default value, the value was successfully saved to the DB as a new option. (Could your issue actually be #28701? Do you have an external object cache?)

Could you share some more of your sample code, preferably a self-contained plugin, which I can use to reproduce the issue?

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


5 years ago

#11 @westonruter
5 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner changed from westonruter to ocean90

I couldn't reproduce any defect with the patch I wrote, and it does fix a flaw where default values fail to apply (though in reality any default value supplied to the WP_Customize_Setting should be used as the second $default arg to get_option and get_theme_mod anyway).

@Aniruddh: Please provide more detailed steps to reproduce the problem, if it still exists with the patch.

#12 @ocean90
5 years ago

30988.2.diff:

  • Tests: It's not clear enough which test is specific for this issue. Also there are to many assertions in one tests. These should be splitted up. Closures should be removed too.
  • parse_post_data(): Can't we introduce private function post_values() and use $this->post_values(); instead of $this->parse_post_data(); $this->_post_values;`?
  • I'm missing a test case to verify the issue.

#13 @westonruter
5 years ago

  • Keywords needs-patch added; has-patch removed
  • Owner changed from ocean90 to westonruter

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


5 years ago

#15 @westonruter
5 years ago

  • Keywords has-patch added; needs-testing reporter-feedback needs-patch removed
  • Owner changed from westonruter to ocean90

#16 @westonruter
5 years ago

@ocean90: in 30988.4.diff:

Full changes in PR: https://github.com/xwp/wordpress-develop/pull/62/files

#17 @westonruter
5 years ago

In 30988.5.diff, I made WP_Customize_Manager::unsanitized_post_values() public instead of private for the sake of #30936. I also include a unit test.

This ticket was mentioned in Slack in #core-customize by ocean90. View the logs.


5 years ago

#19 in reply to: ↑ 8 ; follow-up: @ocean90
5 years ago

Replying to Aniruddh:

So, I was under impression that customizer will save default values to empty table row but instead it saved values of those settings whose default values were changed.

I could reproduce this issue or at least a regression from 4.0.

My code:

add_action( 'customize_register', function() {
	global $wp_customize;

	$wp_customize->add_panel( 'foo', array(
		'title'      => 'Foo',
		'capability' => 'edit_theme_options',
	) );

	$wp_customize->add_section( 'bar', array(
		'title'      => 'Bar',
		'capability' => 'edit_theme_options',
		'panel'      => 'foo'
	) );

	$wp_customize->add_setting( 'foo_text', array(
		'default'    => 'Lorem',
		'capability' => 'edit_posts',
		'type'       => 'option',
	) );

	$wp_customize->add_control( 'foo_text', array(
		'label'    => 'Foo & bar',
		'section'  => 'bar',
		'type'     => 'textarea',
	) );

	$wp_customize->add_setting( 'foo_color', array(
		'default'    => '#f00',
		'capability' => 'edit_theme_options',
		'type'       => 'option',
	) );

	$wp_customize->add_control( new WP_Customize_Color_Control( $wp_customize, 'foo_color', array(
		'label'   => 'Color',
		'section' => 'bar',
	) ) );
});

add_action( 'wp_head', function() {
	var_dump( get_option( 'foo_color' ) );
	var_dump( get_option( 'foo_text' ) );
});

4.0:
Front page without Customizer:

bool(false)
bool(false)

Front page in Customizer:

string(4) "#f00"
string(5) "Lorem"

Front page in Customizer, color changed:

string(7) "#6900db"
string(5) "Lorem"

Changes saved, and front page without Customizer:

string(7) "#6900db"
string(5) "Lorem" ) // Default value is stored

Trunk with patch applied:
Front page without Customizer:

bool(false)
bool(false)

Front page in Customizer:

string(4) "#f00"
string(5) "Lorem"

Front page in Customizer, color changed:

string(7) "#6900db"
string(5) "Lorem"

Changes saved, and front page without Customizer:

string(7) "#6900db"
bool(false) // Default value isn't stored

#20 in reply to: ↑ 19 @westonruter
5 years ago

Replying to ocean90:

I could reproduce this issue or at least a regression from 4.0.

This is actually expected. Remember in 4.1 via #29983 we only POST dirty settings (ones that have changed), so the default value attached to a setting will always be directly used in the Customizer preview as opposed to the post value if the setting was not changed.

I think your plugin code needs to be tweaked, for instance:

  • 30988-plugin.php

     
    11<?php
     2$foo_color_default = '#f00';
     3$foo_text_default = 'Lorem';
    24
    3 add_action( 'customize_register', function() {
     5// These add_option() eliminate the need for supplying default values for settings,
     6// and it eliminates the need to supply the 2nd $default arg to get_option()
     7add_option( 'foo_color', $foo_color_default );
     8add_option( 'foo_text', $foo_text_default );
     9
     10add_action( 'customize_register', function() use ( $foo_color_default, $foo_text_default ) {
    411        global $wp_customize;
    512
    613        $wp_customize->add_panel( 'foo', array(
    add_action( 'customize_register', function() { 
    1522        ) );
    1623
    1724        $wp_customize->add_setting( 'foo_text', array(
    18                 'default'    => 'Lorem',
     25                'default'    => $foo_text_default, // not really needed because add_option() call used with default supplied
    1926                'capability' => 'edit_posts',
    2027                'type'       => 'option',
    2128        ) );
    add_action( 'customize_register', function() { 
    2734        ) );
    2835
    2936        $wp_customize->add_setting( 'foo_color', array(
    30                 'default'    => '#f00',
     37                'default'    => $foo_color_default, // not really needed because add_option() call used with default supplied
    3138                'capability' => 'edit_theme_options',
    3239                'type'       => 'option',
    3340        ) );
    add_action( 'customize_register', function() { 
    3845        ) ) );
    3946});
    4047
    41 add_action( 'wp_head', function() {
    42         var_dump( get_option( 'foo_color' ) );
    43         var_dump( get_option( 'foo_text' ) );
     48add_action( 'wp_head', function() use ( $foo_color_default, $foo_text_default ) {
     49        var_dump( get_option( 'foo_color', $foo_color_default ) ); // second argument not needed since add_option() call used
     50        var_dump( get_option( 'foo_text', $foo_text_default ) ); // second argument not needed since add_option() call used
    4451});

https://gist.github.com/westonruter/1717cadbb3ef581291ba

Without doing add_option() or supplying the default arg to the get_option() call, then the frontend would not get the default values to display without first visiting the Customizer.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


5 years ago

#22 @ocean90
5 years ago

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

In 31329:

Ensure that WP_Customize_Setting::value() returns default value for setting if not dirty.

There was regression introduced by #28580 where only changed (dirty) settings now are POST'ed to the Customizer preview.

  • Allow WP_Customize_Manager::post_value() to accept a second $default argument.
  • Introduce WP_Customize_Manager::unsanitized_post_values() for accessing previously-private member variable _post_values.
  • Do require_once instead of require for Customizer classes.
  • Add unit tests for WP_Customize_Manager and WP_Customize_Setting.

props westonruter.
fixes #30988.

#23 @ocean90
5 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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


5 years ago

#25 @boonebgorges
5 years ago

In 31339:

Use temporary variable for array in Tests_Customize_Setting::test_preview_standard_types_multidimensional().

The syntax previously used - call_user_func( 'foo' )['bar'], where foo()
returns an array - is not valid on all supported versions of PHP, and was
breaking the CI builds.

See #30988.

#26 @ocean90
5 years ago

In 31360:

Customizer: Add changelog entry for the 'default' parameter added in [31329].

props westonruter.
see #30988.

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


5 years ago

#28 @ocean90
5 years ago

[31342] missed the ticket.

#29 @dd32
5 years ago

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

In 31410:

Ensure that WP_Customize_Setting::value() returns default value for setting if not dirty.

There was regression introduced by #28580 where only changed (dirty) settings now are POST'ed to the Customizer preview.

  • Allow WP_Customize_Manager::post_value() to accept a second $default argument.
  • Introduce WP_Customize_Manager::unsanitized_post_values() for accessing previously-private member variable _post_values.
  • Do require_once instead of require for Customizer classes.
  • Add unit tests for WP_Customize_Manager and WP_Customize_Setting.

Props westonruter, boonebgorges.
Merges [31329] [31339] [31342] [31360] to the 4.1 branch.
Fixes #30988.

#30 @SergeyBiryukov
20 months ago

In 42799:

Tests: Correct assertion in Tests_WP_Customize_Setting::test_constructor_with_args().

Props jipmoors.
See #30988, #43218.

Note: See TracTickets for help on using tickets.