Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Site Logo]: Add option to remove/clear logo from the block #34820

Merged
merged 10 commits into from
Sep 28, 2021
Merged

[Site Logo]: Add option to remove/clear logo from the block #34820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0112f94
Add button for removing Site Logo image from block
stacimc Sep 14, 2021
6e1ef4b
Try an icon instead of text button for Remove
stacimc Sep 15, 2021
9e88ec2
Delete custom logo when site logo is removed
stacimc Sep 15, 2021
5e19c9b
Linting
stacimc Sep 15, 2021
a22c507
Add label to remove logo button
stacimc Sep 17, 2021
49e7e78
Revert to text-based Remove button
stacimc Sep 20, 2021
af1194a
Slightly increase spacing between Replace and Remove buttons
stacimc Sep 20, 2021
50cf885
Change 'Remove' to 'Reset' to avoid confusion
stacimc Sep 22, 2021
0140ba0
Unhook both Gutenberg and core hooks for deleting site logo
stacimc Sep 24, 2021
22fd747
Update comments
stacimc Sep 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/block-library/src/site-logo/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
Spinner,
ToggleControl,
ToolbarButton,
ToolbarGroup,
Placeholder,
} from '@wordpress/components';
import { useViewportMatch } from '@wordpress/compose';
Expand Down Expand Up @@ -326,7 +327,7 @@ export default function LogoEdit( {
const _siteLogo = siteSettings?.site_logo;
const _readOnlyLogo = siteData?.site_logo;
const _canUserEdit = canUser( 'update', 'settings' );
const _siteLogoId = _siteLogo || _readOnlyLogo;
const _siteLogoId = _canUserEdit ? _siteLogo : _readOnlyLogo;
const mediaItem =
_siteLogoId &&
select( coreStore ).getMedia( _siteLogoId, {
Expand Down Expand Up @@ -380,6 +381,11 @@ export default function LogoEdit( {
setLogo( media.id );
};

const onRemoveLogo = () => {
setLogo( null );
setLogoUrl( undefined );
};

const onUploadError = ( message ) => {
setError( message[ 2 ] ? message[ 2 ] : null );
};
Expand All @@ -393,6 +399,11 @@ export default function LogoEdit( {
onSelect={ onSelectLogo }
onError={ onUploadError }
/>
<ToolbarGroup>
<ToolbarButton onClick={ onRemoveLogo }>
{ __( 'Reset' ) }
</ToolbarButton>
</ToolbarGroup>
</BlockControls>
);

Expand Down
33 changes: 31 additions & 2 deletions packages/block-library/src/site-logo/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ function _sync_custom_logo_to_site_logo( $value ) {
* @param array $old_value Previous theme mod settings.
* @param array $value Updated theme mod settings.
*/
function _delete_site_logo_on_remove_custom_logo( $old_value, $value ) {
function _gutenberg_delete_site_logo_on_remove_custom_logo( $old_value, $value ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't ideal because it means that, when these changes are merged into Core, that Core will have a function prefixed with _gutenberg.

// If the custom_logo is being unset, it's being removed from theme mods.
if ( isset( $old_value['custom_logo'] ) && ! isset( $value['custom_logo'] ) ) {
delete_option( 'site_logo' );
Expand All @@ -142,7 +142,7 @@ function _delete_site_logo_on_remove_custom_logo( $old_value, $value ) {
/**
* Deletes the site logo when all theme mods are being removed.
*/
function _delete_site_logo_on_remove_theme_mods() {
function _gutenberg_delete_site_logo_on_remove_theme_mods() {
if ( false !== get_theme_support( 'custom-logo' ) ) {
delete_option( 'site_logo' );
}
Expand All @@ -160,3 +160,32 @@ function _delete_site_logo_on_remove_custom_logo_on_setup_theme() {
add_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );
}
add_action( 'setup_theme', '_delete_site_logo_on_remove_custom_logo_on_setup_theme', 11 );

/**
* Removes the custom_logo theme-mod when the site_logo option gets deleted.
*/
function _delete_custom_logo_on_remove_site_logo() {
$theme = get_option( 'stylesheet' );

// Unhook update and delete actions for custom_logo to prevent a loop of hooks.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea adding this to avoid accidentally causing a loop of hooks.

✅ Tested that with this code in the Gutenberg plugin, by execution time has_action returns false so these actions aren't registered, however the code doesn't appear to throw any warnings or cause any issues
✅ Tested that with this code copied to the site-title.php file in public_html/wp-includes/blocks/site-logo.php the actions are registered, so this code prevents the loop

For testing I added the following and inspected by debug.log file:

	$has_action_1 = has_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo' );
	$has_action_2 = has_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );

	error_log( 'has actions' );
	error_log( var_export( $has_action_1, true ) );
	error_log( var_export( $has_action_2, true ) );

The results were:

In Gutenberg plugin:

[17-Sep-2021 01:22:46 UTC] has actions
[17-Sep-2021 01:22:46 UTC] false
[17-Sep-2021 01:22:46 UTC] false

In Core:

[17-Sep-2021 01:36:55 UTC] has actions in core!
[17-Sep-2021 01:36:55 UTC] 10
[17-Sep-2021 01:36:55 UTC] 10

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gutenberg's build process is getting in the way here: because _delete_site_logo_on_remove_custom_logo and _delete_site_logo_on_remove_theme_mods are function names in this file, they are being automatically prefixed with gutenberg_.

This means that the unprefixed Core hooks for the site logo block are not removed, and end up running with remove_theme_mod( 'custom_logo' ), the result being that delete_option( 'site_logo' ); can be called twice. There's a check that prevents this from becoming an infinite loop, but I still think we should prevent this from happening.

You can verify this with the following steps

Without changing the build process, we could make the relevant function names in this file different from core, and that would work around the issue... something like

function gutenberg_delete_site_logo_on_remove_custom_logo( $old_value, $value ) { ...

function gutenberg_delete_site_logo_on_remove_theme_mods() { ...

function _delete_custom_logo_on_remove_site_logo() {
	$theme = get_option( 'stylesheet' );

	// Unhook update and delete actions for custom_logo to prevent a loop of hooks.
	// Gutenberg hooks.
	remove_action( "update_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_custom_logo', 10 );
	remove_action( "delete_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_theme_mods' );

	// Core hooks.
	remove_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10 );
	remove_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );

	// Remove the custom logo.
	remove_theme_mod( 'custom_logo' );

	// Restore update and delete actions.
	// Gutenberg hooks.
	add_action( "update_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_custom_logo', 10, 2 );
	add_action( "delete_option_theme_mods_$theme", 'gutenberg_delete_site_logo_on_remove_theme_mods' );

	// Core hooks.
	add_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10, 2 );
	add_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );
}
add_action( 'delete_option_site_logo', '_delete_custom_logo_on_remove_site_logo' );

It's a little strange, as those functions will become prefixed with gutenberg_gutenberg_, but in practice I don't think that matters.

Another option would be to remove _delete_site_logo_on_remove_custom_logo_on_setup_theme from this file, and the two functions it hooks in (_delete_site_logo_on_remove_custom_logo and _delete_site_logo_on_remove_theme_mods).

Because this file is included on the init hook, and _delete_site_logo_on_remove_custom_logo_on_setup_theme is hooked into setup_theme, which runs before init, these functions are never run from Gutenberg (only from Core). They seem to be here only to keep this file consistent with Core. (related issue: #33177)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a check that prevents this from becoming an infinite loop, but I still think we should prevent this from happening.

That explains how I missed it 😱 Thank you for checking this!

I went with the first suggestion to rename the hooks in Gutenberg and unhook/rehook both. I'm a bit torn since I agree it's a little strange, but I think doing so actually makes the hook duplication clearer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That fixed the issue, thanks!

// Remove Gutenberg hooks.
remove_action( "update_option_theme_mods_$theme", '_gutenberg_delete_site_logo_on_remove_custom_logo', 10 );
remove_action( "delete_option_theme_mods_$theme", '_gutenberg_delete_site_logo_on_remove_theme_mods' );

// Remove Core hooks.
remove_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10 );
remove_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );

// Remove the custom logo.
remove_theme_mod( 'custom_logo' );

// Restore update and delete actions.
// Restore Gutenberg hooks.
add_action( "update_option_theme_mods_$theme", '_gutenberg_delete_site_logo_on_remove_custom_logo', 10, 2 );
add_action( "delete_option_theme_mods_$theme", '_gutenberg_delete_site_logo_on_remove_theme_mods' );

// Restore Core hooks.
add_action( "update_option_theme_mods_$theme", '_delete_site_logo_on_remove_custom_logo', 10, 2 );
add_action( "delete_option_theme_mods_$theme", '_delete_site_logo_on_remove_theme_mods' );
Comment on lines +188 to +189
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_delete_site_logo_on_remove_custom_logo and _delete_site_logo_on_remove_theme_mods both won't exist when Core's copy of site-logo.php is updated to be the site-logo/index.php that we see here. This causes a fatal error.

Fatal error: Uncaught TypeError: call_user_func_array(): Argument #1 ($callback) must be a valid callback, function "_delete_site_logo_on_remove_custom_logo" not found or invalid function name

I think we should revert 0140ba0 and work around the issue with Gutenberg's build process in a different way. I'll open a PR to do this.

Copy link
Member

@noisysocks noisysocks Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix here: #36195

}
add_action( 'delete_option_site_logo', '_delete_custom_logo_on_remove_site_logo' );