WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 3 months ago

#25156 reviewing defect (bug)

get_custom_header() should return false when there is no header

Reported by: nacin Owned by: kovshenin
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Appearance Keywords: has-patch commit
Focuses: Cc:

Description

Follow-up to #20871, discovered when working on #14531 (see [25135]).

When a custom header is removed but a default is specified, get_custom_header() will return that default information. This incorrect.

When the header_image theme mod shows that a header is removed, we should immediately return false.

I am suggesting a false return value in lieu of an empty stdClass as objects are not false-y, and thus it allows for if ( get_custom_header() ) {. (Accessing a property of a non-object merely triggers a notice, versus a fatal error for methods, so this is backwards compatible.)

I imagine a has_custom_header() function may be a good addition at some point.

This wasn't noticed because Twenty Twelve (and other default themes) check get_header_image() first, which returns false when there is no custom header. (has_custom_header() would probably be a bool cast of either get_header_image() or get_custom_header().) Twenty Thirteen doesn't use get_custom_header() at all, in part (I'm guessing) because it doesn't support variable width or variable height.

Attachments (2)

25156.diff (706 bytes) - added by nacin 8 months ago.
25156.2.diff (1.1 KB) - added by kovshenin 7 months ago.

Download all attachments as: .zip

Change History (15)

nacin8 months ago

comment:1 nacin8 months ago

  • Keywords has-patch commit added

comment:2 nacin7 months ago

  • Owner set to kovshenin
  • Status changed from new to reviewing

Asked kovshenin to look this over.

comment:3 nacin7 months ago

Looking at $url = get_theme_mod( 'header_image', get_theme_support( 'custom-header', 'default-image' ) ); in get_header_image(). Seems like we need to handle the case where get_theme_mod() returns false and there is no default-image or default random image. Not sure if this does that.

comment:4 kovshenin7 months ago

Seems like we need to handle the case where get_theme_mod() returns false and there is no default-image or default random image. Not sure if this does that.

It does, but it returns an empty string, not false. get_custom_header on the other hand doesn't. Most themes check the result of get_header_image before running get_custom_header. However, looking around the themes directory I found at least five themes that attempt to access the properties of get_custom_header directly:

As you mentioned, they'll trigger a notice, but I don't think it will really break anything because the image URL is empty either way. Still looking into default and random, maybe we should return false there for consistency too.

kovshenin7 months ago

comment:5 kovshenin7 months ago

25156.2.diff returns false for get_header_image and get_custom_header if a default header image was not set and random is turned off.

comment:6 lancewillett7 months ago

  • Cc lancewillett added

I imagine a has_custom_header() function may be a good addition at some point.

+1

comment:7 nacin7 months ago

  • Milestone changed from 3.7 to 3.8

comment:8 wonderboymusic5 months ago

Is this Unit Test-able, @kovshenin?

comment:9 kovshenin5 months ago

I don't see why not. Will throw in some tests early next week.

comment:10 kovshenin5 months ago

Looked into tests, they would be much easier with #25333 because tests are run after wp_loaded.

comment:11 nacin5 months ago

  • Milestone changed from 3.8 to Future Release

It's a bug, but not a regression, so moving to future. I'm a bit concerned what we might break here.

comment:12 dreamwhisper4 months ago

  • Cc baumann@… added

comment:13 nacin3 months ago

  • Component changed from Themes to Appearance
Note: See TracTickets for help on using tickets.