From eefbff7c8b17aa397c9d8f6525d62ec69660ed57 Mon Sep 17 00:00:00 2001 From: Marcelo Serpa <81248+fullofcaffeine@users.noreply.github.com> Date: Thu, 17 Jun 2021 18:38:54 -0500 Subject: [PATCH] Test regression for settings update corrupting some wp_options --- packages/block-library/src/site-logo/edit.js | 62 +++++-------- .../block-library/src/site-logo/index.php | 91 ++++++++++++++++++- .../e2e-tests/specs/misc/settings.test.js | 43 +++++++++ 3 files changed, 158 insertions(+), 38 deletions(-) create mode 100644 packages/e2e-tests/specs/misc/settings.test.js diff --git a/packages/block-library/src/site-logo/edit.js b/packages/block-library/src/site-logo/edit.js index 852446ed635cc..05e225513392d 100644 --- a/packages/block-library/src/site-logo/edit.js +++ b/packages/block-library/src/site-logo/edit.js @@ -255,45 +255,33 @@ export default function LogoEdit( { const [ logoUrl, setLogoUrl ] = useState(); const [ error, setError ] = useState(); const ref = useRef(); - const { mediaItemData, siteLogo, url, stylesheet } = useSelect( - ( select ) => { - const siteSettings = select( coreStore ).getEditedEntityRecord( - 'root', - 'site' - ); - - const themeModOptionName = `theme_mods_${ siteSettings.stylesheet }`; - - siteSettings[ themeModOptionName ] = - siteSettings[ themeModOptionName ] || {}; - const mediaItem = siteSettings[ themeModOptionName ].custom_logo - ? select( coreStore ).getEntityRecord( - 'root', - 'media', - siteSettings[ themeModOptionName ].custom_logo - ) - : null; - return { - mediaItemData: mediaItem && { - url: mediaItem.source_url, - alt: mediaItem.alt_text, - }, - siteLogo: siteSettings[ themeModOptionName ].custom_logo, - url: siteSettings.url, - stylesheet: siteSettings.stylesheet, - }; - }, - [] - ); + const { mediaItemData, siteLogo, url } = useSelect( ( select ) => { + const siteSettings = select( coreStore ).getEditedEntityRecord( + 'root', + 'site' + ); + const mediaItem = siteSettings.site_logo + ? select( coreStore ).getEntityRecord( + 'root', + 'media', + siteSettings.site_logo + ) + : null; + return { + mediaItemData: mediaItem && { + url: mediaItem.source_url, + alt: mediaItem.alt_text, + }, + siteLogo: siteSettings.site_logo, + url: siteSettings.url, + }; + }, [] ); const { editEntityRecord } = useDispatch( coreStore ); - const setLogo = ( newValue ) => { - const settingsVal = {}; - settingsVal[ `theme_mods_${ stylesheet }` ] = { - custom_logo: newValue, - }; - editEntityRecord( 'root', 'site', undefined, settingsVal ); - }; + const setLogo = ( newValue ) => + editEntityRecord( 'root', 'site', undefined, { + site_logo: newValue, + } ); let alt = null; if ( mediaItemData ) { diff --git a/packages/block-library/src/site-logo/index.php b/packages/block-library/src/site-logo/index.php index 1127f4790a3a7..44d63505cc4ff 100644 --- a/packages/block-library/src/site-logo/index.php +++ b/packages/block-library/src/site-logo/index.php @@ -25,6 +25,8 @@ function render_block_core_site_logo( $attributes ) { $custom_logo = get_custom_logo(); + remove_filter( 'wp_get_attachment_image_src', $adjust_width_height_filter ); + if ( empty( $custom_logo ) ) { return ''; // Return early if no custom logo is set, avoiding extraneous wrapper div. } @@ -56,10 +58,27 @@ function render_block_core_site_logo( $attributes ) { $wrapper_attributes = get_block_wrapper_attributes( array( 'class' => implode( ' ', $classnames ) ) ); $html = sprintf( '
%s
', $wrapper_attributes, $custom_logo ); - remove_filter( 'wp_get_attachment_image_src', $adjust_width_height_filter ); return $html; } +/** + * Register a core site setting for a site logo + */ +function register_block_core_site_logo_setting() { + register_setting( + 'general', + 'site_logo', + array( + 'show_in_rest' => array( + 'name' => 'site_logo', + ), + 'type' => 'integer', + 'description' => __( 'Site logo.' ), + ) + ); +} + +add_action( 'rest_api_init', 'register_block_core_site_logo_setting', 10 ); /** * Registers the `core/site-logo` block on the server. @@ -72,4 +91,74 @@ function register_block_core_site_logo() { ) ); } + add_action( 'init', 'register_block_core_site_logo' ); + +/** + * Overrides the custom logo with a site logo, if the option is set. + * + * @param string $custom_logo The custom logo set by a theme. + * + * @return string The site logo if set. + */ +function _override_custom_logo_theme_mod( $custom_logo ) { + $site_logo = get_option( 'site_logo' ); + return false === $site_logo ? $custom_logo : $site_logo; +} + +add_filter( 'theme_mod_custom_logo', '_override_custom_logo_theme_mod' ); + +/** + * Updates the site_logo option when the custom_logo theme-mod gets updated. + * + * This function is hooked on "update_option_theme_mods_$theme" and not + * "pre_set_theme_mod_custom_logo" because by hooking in `update_option` + * the function accounts for remove_theme_mod() as well. + * + * @param mixed $old_value The old option value. + * @param mixed $value The new option value. + */ +function _sync_custom_logo_to_site_logo( $old_value, $value ) { + // Delete the option when the custom logo does not exist or was removed. + // This step ensures the option stays in sync. + if ( empty( $value['custom_logo'] ) ) { + delete_option( 'site_logo' ); + } else { + remove_action( 'update_option_site_logo', '_sync_site_logo_to_custom_logo' ); + update_option( 'site_logo', $value['custom_logo'] ); + add_action( 'update_option_site_logo', '_sync_site_logo_to_custom_logo', 10, 2 ); + } +} + +/** + * Hooks `_sync_custom_logo_to_site_logo` in `update_option_theme_mods_$theme`. + * + * Runs on `setup_theme` to account for dynamically-switched themes in the Customizer. + */ +function _sync_custom_logo_to_site_logo_on_setup_theme() { + $theme = get_option( 'stylesheet' ); + add_action( "update_option_theme_mods_$theme", '_sync_custom_logo_to_site_logo', 10, 2 ); +} +add_action( 'setup_theme', '_sync_custom_logo_to_site_logo_on_setup_theme', 11 ); + +/** + * Updates the custom_logo theme-mod when the site_logo option gets updated. + * + * @param mixed $old_value The old option value. + * @param mixed $value The new option value. + * + * @return void + */ +function _sync_site_logo_to_custom_logo( $old_value, $value ) { + // Delete the option when the custom logo does not exist or was removed. + // This step ensures the option stays in sync. + if ( empty( $value ) ) { + remove_theme_mod( 'custom_logo' ); + } else { + remove_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' ); + set_theme_mod( 'custom_logo', $value ); + add_filter( 'pre_set_theme_mod_custom_logo', '_sync_custom_logo_to_site_logo' ); + } +} + +add_action( 'update_option_site_logo', '_sync_site_logo_to_custom_logo', 10, 2 ); diff --git a/packages/e2e-tests/specs/misc/settings.test.js b/packages/e2e-tests/specs/misc/settings.test.js new file mode 100644 index 0000000000000..e2bf0457cbd90 --- /dev/null +++ b/packages/e2e-tests/specs/misc/settings.test.js @@ -0,0 +1,43 @@ +/** + * WordPress dependencies + */ +import { visitAdminPage } from '@wordpress/e2e-test-utils'; +/** + * Internal dependencies + */ + +async function getOptionsValues( selector ) { + await visitAdminPage( 'options.php' ); + return page.evaluate( ( theSelector ) => { + const inputs = Array.from( document.querySelectorAll( theSelector ) ); + return inputs.reduce( ( memo, input ) => { + memo[ input.id ] = input.value; + return memo; + }, {} ); + }, selector ); +} + +describe( 'Settings', () => { + test( 'Regression: updating a specific option will only change its value and will not corrupt others', async () => { + // We won't select the option that we updated and will also remove some _transient options that seem to change at + // every update (?) + const optionsInputsSelector = + 'form#all-options table.form-table input:not([id*="_transient"]):not([id="blogdescription"])'; + const optionsBefore = await getOptionsValues( optionsInputsSelector ); + + await visitAdminPage( 'options-general.php' ); + await page.type( + 'input#blogdescription', + 'Just another Gutenberg site' + ); + await page.click( 'input#submit' ); + + const optionsAfter = await getOptionsValues( optionsInputsSelector ); + + Object.entries( optionsBefore ).forEach( ( optionBefore ) => { + const [ id ] = optionBefore; + const optionAfter = [ id, optionsAfter[ id ] ]; + expect( optionAfter ).toStrictEqual( optionBefore ); + } ); + } ); +} );