Make WordPress Core

Opened 7 weeks ago

Last modified 3 days ago

#60922 accepted defect (bug)

get_custom_logo returns an empty link when no logo image is set

Reported by: afercia's profile afercia Owned by: joedolson's profile joedolson
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch dev-feedback needs-testing commit
Focuses: accessibility Cc:

Description (last modified by 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.

Change History (17)

#1 @afercia
7 weeks ago

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

#2 @sabernhardt
7 weeks ago

  • Component changed from General to Themes

#3 @afercia
7 weeks ago

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.

#4 follow-up: @afercia
7 weeks ago

  • Description modified (diff)

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.

#5 @afercia
6 weeks ago

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

Last edited 6 weeks ago by afercia (previous) (diff)

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


6 weeks ago

#7 @joedolson
6 weeks ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.6
  • Owner set to joedolson
  • Status changed from new to accepted

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.

#8 @afercia
6 weeks ago

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.

This ticket was mentioned in PR #6382 on WordPress/wordpress-develop by @khokansardar.


6 weeks ago
#9

  • Keywords has-patch added; needs-patch removed

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

Trac ticket: #60922

#10 @khokansardar
6 weeks ago

  • Keywords dev-feedback needs-testing added

#11 @joedolson
8 days ago

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.

@khokansardar commented on PR #6382:


8 days ago
#12

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.

#13 @khokansardar
8 days ago

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

#14 @joedolson
5 days ago

  • Keywords commit added

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.

#15 in reply to: ↑ 4 @SergeyBiryukov
4 days ago

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.

Last edited 4 days ago by SergeyBiryukov (previous) (diff)

#16 @joedolson
4 days ago

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.

#17 @joedolson
3 days ago

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.

Note: See TracTickets for help on using tickets.