Site Logo: Delete sitelogo option rather than syncing it when setting theme mod #26895

ockham · 2020-11-11T14:34:08Z

Description

Tentative fix for #25173. Will update with more details and testing instructions a bit later today.

The Site Logo block (introduced in #18811) uses the custom logo theme mod concept. This allows it to be set both through the actual Site Block UI, and through the customizer. However, being a theme mod, a custom logo is stored per-theme, and is lost when switching themes.

The Site Logo block thus tapped into two hooks to extend the theme mod behavior, so a site logo would persist across theme changes:

theme_mod_custom_logo is called when the get_custom_logo() is called (e.g. by a theme, or the Site Block). The Site Block added the override_custom_logo_theme_mod in order to return the sitelogo option if it's set, and otherwise fall back to the custom logo theme mod.
pre_set_theme_mod_custom_logo is called whenever a custom logo is set via set_theme_mod() (e.g. by the customizer). The Site Block added the sync_site_logo_to_theme_mod filter to it, and propagated the changes to the sitelogo option.

The latter caused #25173: If a custom logo was removed via remove_theme_mod through the customizer, the pre_set_theme_mod_custom_logo filter isn't run (there are no filters run from inside remove_theme_mod). Thus, while the custom logo theme mod was deleted, the sitelogo remained untouched. This means the next time get_custom_logo was called, the override_custom_logo_theme_mod filter would kick in, and return the sitelogo option.

One way to fix this would be to add a filter or hook to remove_theme_mod that we can tap into (props @sirreal ), but that's a change that would need to be made in Core.

I think we can get away with a simpler fix:

We change the pre_set_theme_mod_custom_logo filter to a function called delete_site_logo_when_setting_theme_mod, which does just that: When the custom logo theme mod is set (customizer), it deletes the sitelogo option. That way, the override_custom_logo_theme_mod won't find a site logo, and return the custom logo theme mod.

All other behavior should be retained as desired.

How has this been tested?

See #25173 for instructions how to repro the issue, and verify that it's fixed.
Furthermore, this issue was caught by the test_get_custom_logo and test_has_custom_logo Core unit tests when run against GB 9.3. Verify that with this change, they now pass.

Types of changes

Bugfix.

Checklist:

My code is tested.
My code follows the WordPress code style.

cc/ @carolinan @sirreal @scruffian @pablinos @creativecoder

… theme mod

github-actions · 2020-11-11T14:44:51Z

Size Change: 0 B

Total Size: 1.19 MB

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	0 B
`build/annotations/index.js`	3.77 kB	0 B
`build/api-fetch/index.js`	3.42 kB	0 B
`build/autop/index.js`	2.83 kB	0 B
`build/blob/index.js`	664 B	0 B
`build/block-directory/index.js`	8.71 kB	0 B
`build/block-directory/style-rtl.css`	943 B	0 B
`build/block-directory/style.css`	942 B	0 B
`build/block-editor/index.js`	133 kB	0 B
`build/block-editor/style-rtl.css`	11.2 kB	0 B
`build/block-editor/style.css`	11.1 kB	0 B
`build/block-library/editor-rtl.css`	8.91 kB	0 B
`build/block-library/editor.css`	8.91 kB	0 B
`build/block-library/index.js`	147 kB	0 B
`build/block-library/style-rtl.css`	8.1 kB	0 B
`build/block-library/style.css`	8.1 kB	0 B
`build/block-library/theme-rtl.css`	792 B	0 B
`build/block-library/theme.css`	793 B	0 B
`build/block-serialization-default-parser/index.js`	1.87 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.06 kB	0 B
`build/blocks/index.js`	48 kB	0 B
`build/components/index.js`	171 kB	0 B
`build/components/style-rtl.css`	15.3 kB	0 B
`build/components/style.css`	15.3 kB	0 B
`build/compose/index.js`	9.87 kB	0 B
`build/core-data/index.js`	14.8 kB	0 B
`build/data-controls/index.js`	821 B	0 B
`build/data/index.js`	8.73 kB	0 B
`build/date/index.js`	11.2 kB	0 B
`build/deprecated/index.js`	768 B	0 B
`build/dom-ready/index.js`	571 B	0 B
`build/dom/index.js`	4.46 kB	0 B
`build/edit-navigation/index.js`	11.1 kB	0 B
`build/edit-navigation/style-rtl.css`	881 B	0 B
`build/edit-navigation/style.css`	885 B	0 B
`build/edit-post/index.js`	306 kB	0 B
`build/edit-post/style-rtl.css`	6.43 kB	0 B
`build/edit-post/style.css`	6.42 kB	0 B
`build/edit-site/index.js`	22.6 kB	0 B
`build/edit-site/style-rtl.css`	3.95 kB	0 B
`build/edit-site/style.css`	3.95 kB	0 B
`build/edit-widgets/index.js`	26.3 kB	0 B
`build/edit-widgets/style-rtl.css`	3.16 kB	0 B
`build/edit-widgets/style.css`	3.16 kB	0 B
`build/editor/editor-styles-rtl.css`	476 B	0 B
`build/editor/editor-styles.css`	478 B	0 B
`build/editor/index.js`	42.5 kB	0 B
`build/editor/style-rtl.css`	3.85 kB	0 B
`build/editor/style.css`	3.85 kB	0 B
`build/element/index.js`	4.62 kB	0 B
`build/escape-html/index.js`	735 B	0 B
`build/format-library/index.js`	6.86 kB	0 B
`build/format-library/style-rtl.css`	547 B	0 B
`build/format-library/style.css`	548 B	0 B
`build/hooks/index.js`	2.16 kB	0 B
`build/html-entities/index.js`	623 B	0 B
`build/i18n/index.js`	3.57 kB	0 B
`build/is-shallow-equal/index.js`	713 B	0 B
`build/keyboard-shortcuts/index.js`	2.52 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.1 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	476 B	0 B
`build/list-reusable-blocks/style.css`	476 B	0 B
`build/media-utils/index.js`	5.31 kB	0 B
`build/notices/index.js`	1.77 kB	0 B
`build/nux/index.js`	3.4 kB	0 B
`build/nux/style-rtl.css`	671 B	0 B
`build/nux/style.css`	668 B	0 B
`build/plugins/index.js`	2.56 kB	0 B
`build/primitives/index.js`	1.43 kB	0 B
`build/priority-queue/index.js`	790 B	0 B
`build/redux-routine/index.js`	2.83 kB	0 B
`build/reusable-blocks/index.js`	3.05 kB	0 B
`build/rich-text/index.js`	13.4 kB	0 B
`build/server-side-render/index.js`	2.77 kB	0 B
`build/shortcode/index.js`	1.69 kB	0 B
`build/token-list/index.js`	1.27 kB	0 B
`build/url/index.js`	4.06 kB	0 B
`build/viewport/index.js`	1.83 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.22 kB	0 B

_{compressed-size-action}

sirreal · 2020-11-11T16:34:02Z

I'll restate in other words to make sure I share the understanding:

There are two values: a theme-specific custom_logo and a site option sitelogo.
Before this change:
- Setting custom_logo would also set sitelogo.
- Retrieving custom_logo would first attempt to retrieve sitelogo and fallback on custom_logo.
After this change:
- Setting custom_logo deletes sitelogo.
- Retrieving custom_logo is the same.

One change (related to the fact that this fixes #25173) is that if you set a sitelogo via the block then change it via the customizer, you'll wind up with custom_logo and no sitelogo.

Will the block continue to work with custom_logo? I took a quick look and I only see it working with sitelogo:

gutenberg/packages/block-library/src/site-logo/edit.js

Lines 233 to 251 in 4fa1856

    
           const { mediaItemData, sitelogo, url } = useSelect( ( select ) => { 
        
           	const siteSettings = select( 'core' ).getEditedEntityRecord( 
        
           		'root', 
        
           		'site' 
        
           	); 
        
           	const mediaItem = select( 'core' ).getEntityRecord( 
        
           		'root', 
        
           		'media', 
        
           		siteSettings.sitelogo 
        
           	); 
        
           	return { 
        
           		mediaItemData: mediaItem && { 
        
           			url: mediaItem.source_url, 
        
           			alt: mediaItem.alt_text, 
        
           		}, 
        
           		sitelogo: siteSettings.sitelogo, 
        
           		url: siteSettings.url, 
        
           	}; 
        
           }, [] );

ockham · 2020-11-11T19:08:48Z

One change (related to the fact that this fixes #25173) is that if you set a sitelogo via the block then change it via the customizer, you'll wind up with custom_logo and no sitelogo.

Will the block continue to work with custom_logo? I took a quick look and I only see it working with sitelogo:

gutenberg/packages/block-library/src/site-logo/edit.js

Lines 233 to 251 in 4fa1856

const { mediaItemData, sitelogo, url } = useSelect( ( select ) => {

const siteSettings = select( 'core' ).getEditedEntityRecord(

'root',

'site'

);

const mediaItem = select( 'core' ).getEntityRecord(

'root',

'media',

siteSettings.sitelogo

);

return {

mediaItemData: mediaItem && {

url: mediaItem.source_url,

alt: mediaItem.alt_text,

},

sitelogo: siteSettings.sitelogo,

url: siteSettings.url,

};

}, [] );

Ah, good point, I hadn't thought about that 🤔 It seems like this would no longer work with this change.

Maybe we can change the client stuff to read from the custom_logo theme mod instead -- provided that there's an endpoint for that. (And I wonder if the theme mod would need to be registered in the theme_supports settings 🤔 )

ockham · 2020-11-11T21:16:29Z

Maybe we can change the client stuff to read from the custom_logo theme mod instead -- provided that there's an endpoint for that.

Seems like there isn't one yet. We can probably register it one-off style to Gutenberg (and work on having it integrated into Core). Means we'd need to inflate the scope of this PR though.

ockham · 2020-11-11T21:42:52Z

Aside: This PR, as it is now, would fix the custom logo issue sufficiently in a context where the Site Editor isn't enabled (and the Site Logo block is thus unavailable), right @sirreal? 😬

pablinos · 2020-11-12T06:56:34Z

The latter caused #25173: If a custom logo was removed via remove_theme_mod through the customizer, the pre_set_theme_mod_custom_logo filter isn't run (there are no filters run from inside remove_theme_mod)

I know it's not ideal as we're relying on the implementation of remove_theme_mod but internally that is updating an option, so we could hook into the update_option hook.

Reading through the logic in this PR, it seems like it might fix the problem by preventing the logo setting from persisting between themes and/or syncing with the custom_logo setting. We could make that choice and have it be initialised from the custom_logo setting, and then prevent it being edited via the customizer, effectively making the logo block the owner of the setting.

ockham · 2020-11-12T14:35:29Z

I've just noticed that while this PR fixes the issue of setting a custom logo and later removing it (both through the customizer), it doesn't actually fix the case of setting a logo through the Site Logo block, and removing it through the customizer. It appears that we really need to tap into remove_theme_mod to that end 😕

ockham · 2020-11-12T14:42:02Z

The latter caused #25173: If a custom logo was removed via remove_theme_mod through the customizer, the pre_set_theme_mod_custom_logo filter isn't run (there are no filters run from inside remove_theme_mod)

I know it's not ideal as we're relying on the implementation of remove_theme_mod but internally that is updating an option, so we could hook into the update_option hook.

Yeah, that would be an option. I'm a bit wary of it though since it's very low-level. If we filter update_option, we'll need to add a check whether the updated theme_mod option contains custom_logo or not, and depending on that, remove the sitelogo option.

Reading through the logic in this PR, it seems like it might fix the problem by preventing the logo setting from persisting between themes and/or syncing with the custom_logo setting.

Yep, I think that's accurate.

We could make that choice and have it be initialised from the custom_logo setting, and then prevent it being edited via the customizer, effectively making the logo block the owner of the setting.

Yeah, that's a good point. There's also the (tangential) question whether all FSE enabled themes need to support the custom_logo theme mod in order for the Site Logo block to work? And if the Customizer will be enabled at all for FSE themes? 🤔

pablinos · 2020-11-13T13:32:48Z

Yeah, that would be an option. I'm a bit wary of it though since it's very low-level. If we filter update_option, we'll need to add a check whether the updated theme_mod option contains custom_logo or not, and depending on that, remove the sitelogo option.

Yes that's my concern is that we'd be tying it to the implementation of remove_theme_mod but that is unlikely to change, and we could use it as an interim solution while we get a hook into that function.

Yep, I think that's accurate.

If that's the case, then we're essentially removing the functionality altogether. We could delete the code that's doing the syncing, and probably use something simpler.

Yeah, that's a good point. There's also the (tangential) question whether all FSE enabled themes need to support the custom_logo theme mod in order for the Site Logo block to work? And if the Customizer will be enabled at all for FSE themes? 🤔

Yes, that's something to consider. Ideally the Customizer wouldn't be needed for FSE themes, but I suppose it will take some time before it can be removed completely.

ockham · 2020-11-17T11:06:32Z

Yeah, that would be an option. I'm a bit wary of it though since it's very low-level. If we filter update_option, we'll need to add a check whether the updated theme_mod option contains custom_logo or not, and depending on that, remove the sitelogo option.

Yes that's my concern is that we'd be tying it to the implementation of remove_theme_mod but that is unlikely to change, and we could use it as an interim solution while we get a hook into that function.

Yeah, that sounds reasonable.

Yep, I think that's accurate.

If that's the case, then we're essentially removing the functionality altogether. We could delete the code that's doing the syncing, and probably use something simpler.

Hmm, my impression was that we're actually retaining most of the functionality. It's true that we're removing the (option-writing) sync, but that's more of an implementation detail; due to how the override (at read time) works, we still get a de-facto sync, at least in one direction: get_custom_logo() will continue to return the site logo, if it is set. It's just the other way round that's not working now: the site logo block won't show a custom logo, if one is set (as pointed out by Jon).

ockham · 2020-11-19T16:22:43Z

@pablinos Could you (or your team) maybe carry this over the finish line, along the lines we've discussed? I'm currently rather busy with other Gutenthings, so it'd be really great if y'all could take care of that ❤️

pablinos · 2020-11-20T14:12:31Z

No problem @ockham. I've been meaning to sort out a PR for the update_option version. I'll get on it.

Hmm, my impression was that we're actually retaining most of the functionality.

You're right, sorry. I think we're retaining the sync one way. If the theory works hopefully we can fix it, but otherwise, we can go with this for now.

ockham · 2020-11-23T12:06:44Z

No problem @ockham. I've been meaning to sort out a PR for the update_option version. I'll get on it.

Awesome, thanks a lot, @pablinos 🙏 ! Feel free to add me as a reviewer 😄

Hmm, my impression was that we're actually retaining most of the functionality.

You're right, sorry. I think we're retaining the sync one way. If the theory works hopefully we can fix it, but otherwise, we can go with this for now.

👍

ockham · 2021-04-27T17:48:41Z

Closing in favor of #30427 (which seems to be sufficient, now that the Customizer is disabled for FSE themes).

Site Logo: Delete sitelogo option rather than syncing it when setting…

4fa1856

… theme mod

ockham added [Type] Bug An existing feature does not function as intended [Status] In Progress Tracking issues with work in progress [Block] Site Logo Affects the Site Logo Block labels Nov 11, 2020

ockham self-assigned this Nov 11, 2020

ockham requested review from scruffian, youknowriad, WunderBart, sirreal and carolinan November 11, 2020 16:10

ockham marked this pull request as ready for review November 11, 2020 16:10

ockham requested review from ajitbohra, Soean and talldan as code owners November 11, 2020 16:10

sirreal assigned pablinos and unassigned ockham Nov 23, 2020

ockham mentioned this pull request Nov 26, 2020

Discussion: Move FSE auto-draft creation into a read-time filter #27284

Closed

Base automatically changed from master to trunk March 1, 2021 15:44

ockham closed this Apr 27, 2021

ockham deleted the fix/custom-logo-theme-mod-site-logo-issue branch April 27, 2021 17:48

ockham mentioned this pull request Jun 8, 2021

The image from the site logo block can not be removed from the customizer #25173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Site Logo: Delete sitelogo option rather than syncing it when setting theme mod #26895

Site Logo: Delete sitelogo option rather than syncing it when setting theme mod #26895

ockham commented Nov 11, 2020 •

edited

Loading

github-actions bot commented Nov 11, 2020

sirreal commented Nov 11, 2020

ockham commented Nov 11, 2020

ockham commented Nov 11, 2020

ockham commented Nov 11, 2020

pablinos commented Nov 12, 2020

ockham commented Nov 12, 2020

ockham commented Nov 12, 2020 •

edited

Loading

pablinos commented Nov 13, 2020

ockham commented Nov 17, 2020

ockham commented Nov 19, 2020

pablinos commented Nov 20, 2020

ockham commented Nov 23, 2020

ockham commented Apr 27, 2021

Site Logo: Delete sitelogo option rather than syncing it when setting theme mod #26895

Site Logo: Delete sitelogo option rather than syncing it when setting theme mod #26895

Conversation

ockham commented Nov 11, 2020 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

github-actions bot commented Nov 11, 2020

sirreal commented Nov 11, 2020

ockham commented Nov 11, 2020

ockham commented Nov 11, 2020

ockham commented Nov 11, 2020

pablinos commented Nov 12, 2020

ockham commented Nov 12, 2020

ockham commented Nov 12, 2020 • edited Loading

pablinos commented Nov 13, 2020

ockham commented Nov 17, 2020

ockham commented Nov 19, 2020

pablinos commented Nov 20, 2020

ockham commented Nov 23, 2020

ockham commented Apr 27, 2021

ockham commented Nov 11, 2020 •

edited

Loading

ockham commented Nov 12, 2020 •

edited

Loading