get_custom_logo returns an empty link when no logo image is set

[afercia]

When a custom logo image is not set, get_custom_logo() returns an empty link regardless.

Empty links are far from ideal for SEO and accessibility and should be avoided.

Example of the returned HTML:

<a href="http://localhost:8889/" class="custom-logo-link" rel="home"></a>

With the Site Logo block of the editor, the HTML would be:

<div class="wp-block-site-logo">
    <a href="http://localhost:8889/" class="custom-logo-link" rel="home"></a>
</div>

where the link markup comes from get_custom_logo()

To reproduce:

• Use Twenty Twent-Four as active theme.
• Go to the site editor, edit the Blog Home template.
• Select the Site Logo block. Add it if necessary.
• Set a logo image and save.
• Go to the Media Library and delete the image you set as site logo.
• Go to the front end.
• Inspect the source and observe the site logo link is an empty link.

[afercia]

Note: I created a GitHub issue for a similar problem with the Site Title but that's more responsibility of the Gutenberg editor. See ​https://github.com/WordPress/gutenberg/issues/60467

[afercia]

I was undecided on the component to assign to this ticket. Not sure 'Themes' is fully appropriate for a function that is meant for general templating.

[afercia]

After some debugging, turns out the root problem is that get_custom_logo() first retrives the logo ID by retrieving get_theme_mod( 'custom_logo' ). That option is set when setting a site logo but it's noc cleared when the image is deleted from the Media Library (or deleted from the file system).

get_custom_logo() does check if the$custom_logo_id exists but that comes from the site_logo option in the wp_options table. That value still exists even when the referenced image is deleted from the Media Library.

get_custom_logo() does _not_ check whether the image actually exists. When the image is deleted from the Media Library, ​wp_get_attachment_image returns an empty string and there's no check for that, the markup is rendered regardless and prints an empty string.

Looks like this function assumes the image does exist when the custom_logo/site_logo option is set, which isn't true.

[afercia]

Note: the custom logo id passed to the ​get_custom_logo_image_attributes filter in this function is documented as int but it's actually a string. See #36640

[joedolson]

I think that the scenarios where this will be triggered are relatively uncommon, since most of the time when somebody removes their site logo they'll notice the impact on their site and change it. However, we can protect against this problem relatively easily.

There are two aspects to address: 1) unsetting the theme mod when the site logo is deleted and 2) correctly handling the case where wp_get_attachment_image returns an empty string.

[afercia]

the scenarios where this will be triggered are relatively uncommon,

Yes I'd agree they are uncommon but, still, WordPress shouldn't output empty links, ever. The output fix is simple enough to be addressed.

There are two aspects to address: 1) unsetting the theme mod when the site logo is deleted and 2) correctly handling the case where wp_get_attachment_image returns an empty string.

Not sure the first aspect can be solved easily. I'm not sure we should try to fix it by deleting the theme mod option. Technically, the option _is_ set because users did so in the admin interface. It's just the image that may be missing.

How about to consider to check whether the image actually exists and outputs a notice in the admin? Ideally, also in the editor.

Fix site logo missing empty link render for SEO and accessibility purpose.

Trac ticket: #60922

[joedolson]

So, this issue is actually specific to the logo block; it does not occur with other themes. The site logo block does not set the custom_logo theme mod; it instead sets an option 'site_logo' and overrides the theme mod with that option's value.

While we should still create an exit to handle the case where get_custom_logo() returns an empty string, we should also add a handler like _delete_attachment_theme_mod() that removes the site_logo setting when the attachment is deleted. There are a number of existing handlers in wp-includes\blocks\site-logo.php that cover cases where the theme mod is updated, but none of them appear to handle the case where the image is deleted outside of the editor. That change probably needs to happen in Gutenberg.

We should handle this in both get_custom_logo() and in has_custom_logo(), so that the checks match the real state. Commented on PR, @khokansardar.

This looks good, but we should probably add a similar check in has_custom_logo() so that the conditional is consistent with what we output.

@joedolson I have update the PR.

[khokansardar]

@joedolson thanks your suggestions. I have made some changes in has_custom_logo() and get_custom_logo() for better consistent with output.

[joedolson]

I think this is now good to go; I made a change to fix the failing test, and now I think this is solid.

Tested following the reproduction steps in the original report and this patch does fix the issue; also tested misc. other cases with other themes, and did not identify any regressions.

[SergeyBiryukov]

Replying to afercia:

After some debugging, turns out the root problem is that get_custom_logo() first retrives the logo ID by retrieving get_theme_mod( 'custom_logo' ). That option is set when setting a site logo but it's not cleared when the image is deleted from the Media Library (or deleted from the file system).

After some more debugging in a coding session with @afercia, it looks like there is a difference between how Site Logo is handled for classic themes and block themes:

• For classic themes, when deleting an image previously set as the site logo, the custom_logo theme mod is properly cleared by the _delete_attachment_theme_mod() function, hooked to delete_attachment.
• For block themes, it's a bit more complicated: the Site Logo block uses a different data source, a site_logo option instead of a theme mod (see ​GB #32065), and contains some code to sync between the two. The option is supposed to be cleared by _delete_site_logo_on_remove_custom_logo() when the theme mod is removed, but that does not happen when deleting the attachment. If this conditional​ is adjusted to also take the site_logo option into account, then the option can be properly cleared too.

To summarize, while the existing PR essentially fixes the symptom, it might still be good to go, as it does make get_custom_logo() and has_custom_logo() more future-proof.

Ideally, the root cause should also be fixed by clearing the site_logo option when the related attachment is deleted.

It can also be up for a future discussion whether or not it's a good idea to store the site logo globally, not depending on the theme, as themes might have different requirements for the logo size, and the image cropping step is no longer a part of the process. This appears to be a relatively recent change from the previous behavior, see ​GB #32065.

[joedolson]

Yes, that's the same as what I observed in comment 11, above. I'm unsure whether that should be resolved in Gutenberg or in core, however.

Overall, it seems like a larger issue about whether or not changing the mechanism for saving that information is a good idea at all, but this fix would resolve the accessibility problem and better future-proof the code immediately, without preventing any future changes to fix the issue with the site logo block's behavior.

I think that changing the site logo block behavior should be a separate ticket, and this one should move forward as is.

[joedolson]

I added a commit to the PR to delete the site_logo option in the _delete_attachment_theme_mod function. That seems like a sensible thing to include in this ticket.

[joedolson]

In 58213:

Themes: Accessibility: Logo block returns empty link when image not set.

The logo block does not use theme mods, and instead saves a site-wide option site_logo to use as the site logo.

Add handling for cases where the logo image does not exist and delete the site_logo option when the attachment configured as site logo is deleted.

Props afercia, joedolson, khokansardar, SergeyBiryukov.
Fixes #60922.

Fixed in r58213

[afercia]

Worth noting the editor allows to set the Site Logo image to be used as a Site Icon. See attached screenshot. Guess it, the Site Icon image ID is saved to a site_icon option. When deleting the image from the Media Library, the site_icon option is not deleted. Should this be addressed in a follow-up?
Cc @SergeyBiryukov

[joedolson]

I think that feels like a separate issue, to me. Should be addressed, but it's not really directly related to the topic of this ticket, and should probably be a separate ticket.

[greenshady]

Noting that this change seemed to introduce a new bug here: https://core.trac.wordpress.org/ticket/61408

[joedolson]

In 58407:

Themes: Fix attachment rendered as site logo on attachment page.

Fix issue where the attachment thumbnail would be rendered as the site logo on attachment single templates if no site logo is set. Avoid calling wp_attachment_is_image() with no value, since that function will fallback to the global $post variable. Follow up to [58213]. See #60922.

Props greenshady, krupajnanda, hmbashar, rajinsharwar, joedolson.
Fixes #61408.