WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 7 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: Customize Keywords: has-patch needs-unit-tests
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 2 years ago.
25156.2.diff (1.1 KB) - added by kovshenin 23 months ago.

Download all attachments as: .zip

Change History (16)

@nacin2 years ago

comment:1 @nacin2 years ago

  • Keywords has-patch commit added

comment:2 @nacin23 months ago

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

Asked kovshenin to look this over.

comment:3 @nacin23 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 @kovshenin23 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.

@kovshenin23 months ago

comment:5 @kovshenin23 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 @lancewillett23 months ago

  • Cc lancewillett added

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

+1

comment:7 @nacin22 months ago

  • Milestone changed from 3.7 to 3.8

comment:8 @wonderboymusic21 months ago

Is this Unit Test-able, @kovshenin?

comment:9 @kovshenin21 months ago

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

comment:10 @kovshenin21 months ago

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

comment:11 @nacin20 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 @dreamwhisper19 months ago

  • Cc baumann@… added

comment:13 @nacin18 months ago

  • Component changed from Themes to Appearance

comment:14 @celloexpressions7 months ago

  • Keywords needs-unit-tests added; commit removed
Note: See TracTickets for help on using tickets.