WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 5 weeks ago

#21876 closed defect (bug) (fixed)

unregister_default_headers() will sometimes return null

Reported by: conner_bw Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: minor Version: 3.4.2
Component: Appearance Keywords: has-patch dev-feedback
Focuses: Cc:

Description

File: wp-includes/theme.php
Function: unregister_default_headers()
Line: ~1046

Problem:
PHPDoc comment says: @return True on success, false on failure.

Expected:
true or false:

Actual:
var_dump(unregister_default_headers(array('FAKE'))); will return NULL

Suggested fix, change:

function unregister_default_headers( $header ) {
	global $_wp_default_headers;
	if ( is_array( $header ) ) {
		array_map( 'unregister_default_headers', $header );
	} elseif ( isset( $_wp_default_headers[ $header ] ) ) {
		unset( $_wp_default_headers[ $header ] );
		return true;
	} else {
		return false;
	}
}

To:

function unregister_default_headers( $header ) {
	global $_wp_default_headers;
	if ( is_array( $header ) ) {
	        $_ret = array_map( 'unregister_default_headers', $header );
	        return in_array(false, $_ret, true) ? false : true;
	} elseif ( isset( $_wp_default_headers[ $header ] ) ) {
		unset( $_wp_default_headers[ $header ] );
		return true;
	} else {
		return false;
	}
}

Thank you for your consideration.

Attachments (4)

21876.diff (646 bytes) - added by MikeHansenMe 17 months ago.
patch based on code sample above minus shorthand
21876.2.diff (607 bytes) - added by aubreypwd 8 weeks ago.
Updated patch
21876.3.diff (863 bytes) - added by SergeyBiryukov 8 weeks ago.
21876.4.diff (842 bytes) - added by MikeHansenMe 8 weeks ago.

Download all attachments as: .zip

Change History (14)

comment:1 SergeyBiryukov19 months ago

  • Component changed from General to Themes

MikeHansenMe17 months ago

patch based on code sample above minus shorthand

comment:2 nacin3 months ago

  • Component changed from Themes to Appearance

comment:3 ircbot8 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by aubreypwd. View the logs.

aubreypwd8 weeks ago

Updated patch

comment:4 aubreypwd8 weeks ago

  • Keywords has-patch dev-feedback added

SergeyBiryukov8 weeks ago

comment:5 SergeyBiryukov8 weeks ago

  • Milestone changed from Awaiting Review to 3.9

comment:6 follow-up: obenland8 weeks ago

I'm usually always in support of functions returning a single data type. But in this case I wonder if it wouldn't be a legitimate case of having two types, bool|void.

What is gained by having the function return true as soon as we have one successful removal of a number of header to be removed? Would you want the function to return true if one header was removed, but the other 5 that you passed weren't?

MikeHansenMe8 weeks ago

comment:7 in reply to: ↑ 6 SergeyBiryukov7 weeks ago

Replying to obenland:

What is gained by having the function return true as soon as we have one successful removal of a number of header to be removed? Would you want the function to return true if one header was removed, but the other 5 that you passed weren't?

Unless I'm missing something, the patch should only return true if all the headers were removed.

21876.4.diff was my initial thought as well, but 21876.3.diff seemed a bit less cryptic.

comment:8 aubreypwd7 weeks ago

Unless I'm missing something, the patch should only return true if all the headers were removed.

What is gained by having the function return true as soon as we have one successful removal of a number of header to be removed?

If we did something like true if all were removed, and an array() of headers that weren't removed, I'm not sure what the receiving function would do with that either.

I am assuming (because of my lack of full understanding of the function) that a person that does:

function _remove_twenty_ten_headers(){
    unregister_default_headers( array(
        'berries',
        'cherryblossom',
        'concave',
        'fern',
        'forestfloor',
        'inkwell',
        'path' ,
        'sunset')
    );
}
 
add_action( 'after_setup_theme', 'jorbin_remove_twenty_ten_headers', 11 );

...they just want to make sure those are removed, as they maybe cause problems with the child theme, etc.

I think the same array() could be returned with some information that would help the function or developer find out why false was returned. Might be more helpful for the developer if a header wasn't removed for any other reason than it was not in the array. Maybe help the developer take out unnecessary code?

array(
        'berries'=>true, //removed
        'cherryblossom'=>true,

        'concave'=>array( 
        	false, //not removed
        	'not_present' //why
        ),
        
        'fern'=>array(
        	false, //not removed
        	'unable_to_remove' //why
        ),
        'forestfloor'=>true,
        'inkwell'=>true,
        'path'=>true,
        'sunset'=>true
    );

I know var_dump() is one of my favorite functions :)

Last edited 7 weeks ago by aubreypwd (previous) (diff)

comment:9 nacin7 weeks ago

I think I'm with obenland on having this continue to return void when multiple headers are passed.

comment:10 nacin5 weeks ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 27575:

Clarify that unregister_default_headers() returns void in some situations. fixes #21876.

Note: See TracTickets for help on using tickets.