WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 6 years ago

#21282 closed enhancement (maybelater)

Introduce wp_header() pluggable function

Reported by: sirzooro Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.4.1
Component: Bootstrap/Load Keywords: has-patch 2nd-opinion needs-refresh
Focuses: Cc:

Description

I will be nice to have pluggable function wrapper for PHP header() function - this will allow to unit test HTTP headers sent by WordPress. Attached patch adds it, and replaces all existing calls to header() with it.

Question: some of exiting calls had errors silenced using @, some not. In my code I added @. Should I keep it, remove it or maybe add extra param to function to conditionally silence errors?

Attachments (1)

21273.diff (39.8 KB) - added by sirzooro 9 years ago.

Download all attachments as: .zip

Change History (7)

@sirzooro
9 years ago

#1 @scribu
9 years ago

  • Keywords 2nd-opinion added

We already have a wp_redirect() function, which should be used instead of header( "Location: ..." ).

I don't see how changing the other headers, such as "Content Type" would help with testing.

Also, I personally don't like pluggable functions.

#2 @sirzooro
9 years ago

When header() function is used in tested code, PHPUnit will report error in tests because of "Cannot modify header information - headers already sent" warning. Therefore header() function in tested code has to be either prefixed with @ or stubbed. My patch allows to use the latter approach.

Regarding pluggable functions - I can use another approach, e.g. call wp_header action from wp_header() function, and hook a function to it.

#3 @bpetty
9 years ago

As this patch stands right now, it will silence any errors with production site code sending back headers, not just unit tests, so I don't think this should be applied as is.

At least in all the situations that involve redirect headers, those *should* be calling wp_redirect(), and wp_redirect() is also already unit testable. So any header() calls used for redirect should be changed to use that, especially since plugins using filters on redirects are expecting WordPress to use that call before any redirect.

I'm still not sure how to handle anywhere else that uses any other type of header though, but I also don't think there's one blanket solution for every method that calls header(). In many cases, those are the primary functionality of those functions, and they need to be tested that the headers are correctly generated (even if they aren't sent).

#4 @nacin
7 years ago

  • Component changed from General to Bootstrap/Load

#5 @swissspidy
6 years ago

  • Keywords needs-refresh added

#6 @johnbillion
6 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.