Ticket #2860 (closed defect (bug): wontfix)

Opened 4 years ago

Last modified 10 months ago

Using wp_redirect when headers are sent

Reported by: robmiller Owned by: robmiller
Priority: normal Milestone:
Component: General Version: 2.1
Severity: normal Keywords: has-patch 2nd-opinion
Cc:

Description

Currently wp_redirect() redirects by sending a Refresh/Location header, making it unusable when the headers have already been sent (such as on an admin page).

This patch checks if the headers are sent and if so echos a meta refresh instead. What do people think about this?

Attachments

2860.diff Download (0.6 KB) - added by robmiller 4 years ago.

Change History

robmiller — 4 years ago
  • keywords bg|has-patch bg|2nd-opinion added
  • owner changed from anonymous to robmiller
  • status changed from new to assigned

ryan — 4 years ago

Right now we only call wp_redirect() when redirecting after sending a cookie. wp_redirect() uses Refresh to work-around an IIS Set-Cookie bug. Does headers_sent() evaluateto true if cookies have been sent? If so, does http-equiv='refresh' workaround the IIS set cookie bug? Just want to make sure we don't break IIS users.

I'm not sure and I'm afraid I don't have IIS to test. I'm pretty sure cookies will definitely be set, though, since it's making an entirely new request and not simply being redirected á la a Header('Location: '); call.

 This comment has the answer. It won't hurt anyone to patch this though. It'll only leave some IIS users right where they already are because it's a false negative (false returned falsely) which mimics prior logic.

However, Meta tags don't belong anywhere but the head section and will invalidate xhtml if they appear elsewhere. I have tested this on the validator.w3.org

RuddO — 4 years ago

Doesn't meta refresh only work in the head tag?

It works everywhere but is only valid in the head.

  • keywords has-patch 2nd-opinion added; bg|has-patch bg|2nd-opinion removed
  • milestone changed from 2.0.4 to 2.2

If the page is already expecting to redirect, it should not have any content displayed, and it would seem pretty easy just to write a valid HTML document along the lines of 

<html>
  <head><meta refresh-magic-syntax></head>
  <body>
    <p>Sorry, automatic redirection failed.
    You should <a href="wherever-we-are-redirecting">continue here</a>.
  </body>
</html>

That way, you get Javascript redirection as a fallback plus a plain old link as a secondary fallback.

  • milestone changed from 2.2 to 2.3

ryan — 3 years ago
  • milestone changed from 2.3 to 2.4 (next)
  • milestone changed from 2.5 to 2.6

Bumping for 2.5 feature freeze.

  • status changed from assigned to closed
  • resolution set to wontfix
  • milestone 2.9 deleted

this has been open for 3 years without a commit.

if the current implementation was dysfunctional in genuine use cases, it would have been reported over and over again.

and the suggested patch would hinder debugging efforts on post requests.

Note: See TracTickets for help on using tickets.