WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 months ago

#25156 closed defect (bug) (maybelater)

get_custom_header() should return false when there is no header

Reported by: nacin Owned by: kovshenin
Milestone: 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 3 years ago.
25156.2.diff (1.1 KB) - added by kovshenin 3 years ago.

Download all attachments as: .zip

Change History (18)

@nacin
3 years ago

#1 @nacin
3 years ago

  • Keywords has-patch commit added

#2 @nacin
3 years ago

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

Asked kovshenin to look this over.

#3 @nacin
3 years 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.

#4 @kovshenin
3 years 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.

@kovshenin
3 years ago

#5 @kovshenin
3 years 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.

#6 @lancewillett
3 years ago

  • Cc lancewillett added

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

+1

#7 @nacin
3 years ago

  • Milestone changed from 3.7 to 3.8

#8 @wonderboymusic
3 years ago

Is this Unit Test-able, @kovshenin?

#9 @kovshenin
3 years ago

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

#10 @kovshenin
3 years ago

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

#11 @nacin
3 years 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.

#12 @dreamwhisper
3 years ago

  • Cc baumann@… added

#13 @nacin
3 years ago

  • Component changed from Themes to Appearance

#14 @celloexpressions
23 months ago

  • Keywords needs-unit-tests added; commit removed

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


3 months ago

#16 @celloexpressions
3 months ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from reviewing to closed

Closing for now per the slack conversation above due to lack of interest. It sounds like a good idea, but doesn't seem to be something people run into much in practice. If anyone's interested in working on this, please feel free to reopen.

Note: See TracTickets for help on using tickets.