![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
Opened 13 years ago
Closed 10 years ago
#25156 closed defect (bug) (maybelater)
get_custom_header() should return false when there is no header
| Reported by: | nacin | Owned by: | kovshenin |
|---|---|---|---|
| Priority: | normal | Milestone: | |
| Component: | Customize | Version: | |
| Severity: | normal | Keywords: | has-patch needs-unit-tests |
| Cc: | Focuses: |
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)
Change History (18)
#3
@
13 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
@
13 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:
- http://themes.svn.wordpress.org/_second-foundation/1.0.4.3/header.php (old version)
- http://themes.svn.wordpress.org/alexandria/2.0.1/slider-cheader.php
- http://themes.svn.wordpress.org/aplau/1.2.1/header.php
- http://themes.svn.wordpress.org/bitlumen/1.0.1/functions-settings.php
- http://themes.svn.wordpress.org/spartan/1.2.32/archive.php
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.
#5
@
13 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
@
13 years ago
- Cc added
I imagine a has_custom_header() function may be a good addition at some point.
+1
#10
@
13 years ago
Looked into tests, they would be much easier with #25333 because tests are run after wp_loaded.
#11
@
13 years ago
- Milestone 3.8 → Future Release
It's a bug, but not a regression, so moving to future. I'm a bit concerned what we might break here.
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
10 years ago
#16
@
10 years ago
- Milestone Future Release
- Resolution → maybelater
- Status reviewing → 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.
Asked kovshenin to look this over.