Make WordPress Core

Opened 15 years ago

Closed 13 years ago

#10187 closed defect (bug) (fixed)

wp_redirect and canonical redirect do not work as expected on IIS

Reported by: peaceablewhale's profile peaceablewhale Owned by: dd32's profile dd32
Milestone: 3.1 Priority: normal
Severity: normal Version: 2.8
Component: HTTP API Keywords: has-patch dev-feedback
Focuses: Cc:

Description

To address a bug affecting IIS <=5, wp_redirect use header("Refresh: 0; url=$location") instead of header("Location: $location") on all versions of IIS, which is unexpected.

Attachments (6)

10187.patch (991 bytes) - added by peaceablewhale 15 years ago.
10187_ruslany.patch (1.3 KB) - added by ruslany 14 years ago.
Updated patch that preserves the fixes for apache/cgi
10187_drop_iis5.patch (1.2 KB) - added by peaceablewhale 14 years ago.
Removed the call to status_header()
10187-10435-merged.patch (4.9 KB) - added by peaceablewhale 14 years ago.
10187-10435-merged.2.patch (4.8 KB) - added by peaceablewhale 14 years ago.
10187-16880-merged.patch (2.9 KB) - added by kcristiano 13 years ago.
Refreshed to rev 16880

Download all attachments as: .zip

Change History (52)

#1 @peaceablewhale
15 years ago

This patch depends on #10186. Tested with PHP 5.2.9-2 on IIS 5.1 via FastCGI.

#2 @peaceablewhale
15 years ago

Patch updated due to #10186.

#3 @westi
15 years ago

  • Milestone 2.9 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I don't think at the point it time we should be adding fixes to issues in IIS5.

Windows 2000 is already in Extend Support and there are two newer major versions of IIS available.

Closing as WONTFIX

#4 @dd32
15 years ago

  • Milestone set to Future Release
  • Resolution wontfix deleted
  • Status changed from closed to reopened

This isnt addressing a bug in IIS5.

WP includes a fix to address IIS5, Which is breaking redirects on IIS > 5

#5 @peaceablewhale
15 years ago

The existing WordPress code prevents newer IIS to send correct HTTP redirect headers and I am trying to fix that.

#6 @westi
15 years ago

  • Keywords needs-patch added; has-patch tested removed
  • Milestone changed from Future Release to 2.9

Sorry. I read the logic the wrong way round.

I still don't think a global server version is correct.

Better to set a $iis_supports_location_header global and use that that use server version in the function.

#7 @peaceablewhale
15 years ago

How about dropping support to IIS <=5 completely?

#8 @peaceablewhale
15 years ago

  • Keywords has-patch needs-review added; needs-patch removed

A patch that drops support to IIS <=5 has been uploaded

#9 @ruslany
15 years ago

  • Cc ruslany@… added

I think that dropping support for IIS <= 5 is a good suggestion. Proper PHP support has been added to IIS starting from 5.1.

If support for IIS <=5 is dropped then the patch looks good to me.

#10 @janeforshort
14 years ago

  • Milestone changed from 2.9 to Future Release

Most recent patch doesn't show evidence of community testing, and we're in beta, so punting.

#11 @ruslany
14 years ago

  • Keywords needs-review removed
  • Milestone changed from Future Release to 3.0
  • Owner set to westi
  • Status changed from reopened to assigned

Verified the latest patch on Windows 7 with IIS 7.5. It works as expected.

Note that at the moment there is a bug in IIS FastCGI module in IIS 5.1, IIS 6.0 and IIS 7.0 which causes all redirect calls from PHP to have status code 302. The update for the FastCGI module with the fix for this will be available soon. But I think the patch should be applied anyway, even before the IIS FastCGI bug is fixed. That way WP will start using proper redirects instead of refresh header. Users who have not updated IIS FastCGI will see HTTP 302 redirects (which is still better than refresh header). User who have updated the IIS FastCGI will see HTTP 301 redirects.

#12 @Denis-de-Bernardy
14 years ago

both patches are plain wrong. they remove fixes for apache/php as cgi.

#13 @Denis-de-Bernardy
14 years ago

  • Keywords needs-patch added; has-patch removed

@ruslany
14 years ago

Updated patch that preserves the fixes for apache/cgi

#14 @ruslany
14 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Attached an updated patch that preserves the fixes for apache/cgi.

@peaceablewhale
14 years ago

Removed the call to status_header()

#15 follow-up: @peaceablewhale
14 years ago

As the status code has been passed to the header() function, I don't think we still need to call status_header(). An updated patch has been attached.

#16 @hakre
14 years ago

Related: #10435

#17 in reply to: ↑ 15 @hakre
14 years ago

  • Keywords tested added; needs-testing removed

Replying to peaceablewhale:

As the status code has been passed to the header() function, I don't think we still need to call status_header(). An updated patch has been attached.

Reviewed and tested that. Can confirm it. I see no difference here wether or not status_header() is called. Compared the plain headers output as well.

#18 @hakre
14 years ago

Related: #3528

#19 @peaceablewhale
14 years ago

  • Component changed from General to HTTP
  • Keywords early added
  • Summary changed from wp_redirect doesn't work as expected on IIS to wp_redirect and canonical redirect do not work as expected on IIS

Merged with #10435

#20 @hakre
14 years ago

Reviewed the patch again: I think the check for DOMDocument does the deal for now, the function where this applies to could be referenced. The comment is a bit long to read for one line of code but it does the job. If DOMDocument is replaced later on for compability reasons this code should be refactored again. Looks good so far.

Maybe ruslany can test the merged patch again? Just in case.

#21 @ruslany
14 years ago

Verified the merged patch. It works correctly.

Also, instead of checking for DOMDocument, the code could call phpversion() and check if it is PHP 5. This could be tracked by a separate ticket perhaps?

#22 @dd32
14 years ago

This seems acceptable to me.. But I'd like to hear other Devs thoughts on this first.

The proposed solution:

  • Remove <= IIS5.0 support
  • Move all redirects to use Location header instead of the Refresh header.

#23 @dd32
14 years ago

  • Milestone changed from 3.0 to 3.1

This would be better off handled early in a dev cycle however i believe. 3.1 material.

#25 @peaceablewhale
14 years ago

Any update to this issue? Will the patch be checked in?

#26 @peaceablewhale
14 years ago

Patch updated for WordPress 3.0.

#27 @dd32
13 years ago

  • Owner changed from westi to dd32
  • Status changed from assigned to accepted

Cross reference to a problem caused by this: #14904 (Install page not redirecting to wp-admin/ on IIS6)

#28 @knutsp
13 years ago

I have tested 10187-10435-merged.2.patch such:

On IIS6 (PHP5.3, fcgi, no rewrite module) while requesting the root (front page) on an uninstalled blog with a present wp-config.php. Redirects perfectly to wp-admin/install.php

On IIS7 (PHP5.3, fcgi, with rewrite module) for canonical redirects, testing with ?p=id and ?page_id=id. Redirects perfectly to the correct permalink.

#29 @dd32
13 years ago

  • Milestone changed from Awaiting Triage to 3.1

#30 follow-up: @dd32
13 years ago

Based on #14904, It seems that $is_IIS has been set too early for iis5 environments, and as a result, redirects to the installation page havn't been working properly there in 3.0.

As IIS 5.0 Mainstream & Extended support timeframes from Microsoft are over, I feel removing the IIS hacks is appropriate at this time.

#31 in reply to: ↑ 30 @peaceablewhale
13 years ago

Replying to dd32:

Based on #14904, It seems that $is_IIS has been set too early for iis5 environments, and as a result, redirects to the installation page havn't been working properly there in 3.0.

As IIS 5.0 Mainstream & Extended support timeframes from Microsoft are over, I feel removing the IIS hacks is appropriate at this time.

+1 on removing the IIS hacks

#32 @dd32
13 years ago

(In [15682]) Retire IIS 3,4,5 Set-Cookie redirection workaround. See [2436] for original implementation. See #10187

#33 @peaceablewhale
13 years ago

It's not necessary to have

if ( php_sapi_name() != 'cgi-fcgi' )

status_header($status);

in the patch, as the status is passed to the header() function.

In addition, [15682] does not fully fix #10187 due to missing fix on canonical redirect.

#34 @dd32
13 years ago

In addition, [15682] does not fully fix

This ticket isn't quite closed yet, That commit is just the start of fixing the ticket.

It's not necessary to have
if ( php_sapi_name() != 'cgi-fcgi' )
status_header($status);

I'm still confused on that one, it wasn't being applied to IIS which is why I left it out of that initial patch, Until I can work out why it was originally added (ie. the server versions in question) i'm tempted to leave it in there for now.

#35 @nacin
13 years ago

  • Keywords dev-feedback added; tested early removed

Dion, what's left here for 3.1?

#36 @dd32
13 years ago

(In [16797]) Enable canonical redirections for Permalink suporting IIS. Props peaceablewhale See #10187

#37 @dd32
13 years ago

I'm honestly unsure of what to do with the status_header() call at this stage.

I'm tempted to restore it to the following to be on the safe side for compatibility with other server environments.. This basically restores it to pre-3.1 whilst removing the Refresh hacks for IIS.

if ( !$is_IIS && php_sapi_name() != 'cgi-fcgi' )
		status_header($status)

Reviewing in 3.2 to remove status_header() completely with a move to PHP5 might be the better ground on this one to call the ticket closed for 3.1

#38 @azizur
13 years ago

  • Cc azizur added

#39 follow-up: @nacin
13 years ago

iis7_supports_permalinks() is only defined in the admin. That causes an error in redirect_canonical.

#40 @peaceablewhale
13 years ago

In my patch, I moved iis7_supports_permalinks() to wp-includes/vars.php.

#41 in reply to: ↑ 39 @kcristiano
13 years ago

  • Cc kcristiano@… added

Replying to nacin:

iis7_supports_permalinks() is only defined in the admin. That causes an error in redirect_canonical.

I applied peaceablewhale's patch to latest trunk on IIS 7 and IIS 7.5. Works fine. I will attach a patch refreshed off of 16880. (1st attempt to upload a patch, let me know if it is incomplete or flat out wrong)

@kcristiano
13 years ago

Refreshed to rev 16880

#42 @dd32
13 years ago

:( I thought I committed that change, But I didn't.
One question I was having at the time, was is vars.php the best location for a function, it is afterall, a vars.php file..

Can anyone think of a better place than functions.php? rewrite.php isnt the best location either

#43 @nacin
13 years ago

Rewrite.php makes the most sense outside of functions.php. We do have the non-WP rule bits in rewrite, so it's not too much a stretch.

#44 @dd32
13 years ago

(In [16904]) Fix fatal error on IIS after r16797. props peaceablewhale. Limit variable string searching when possible. See #10187

#45 @dd32
13 years ago

(In [16905]) Other piece of r16904. Limit variable string searching when possible. See #10187

#46 @dd32
13 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [16907]) Restore pre-3.1 "Status:" header handling for IIS in wp_redirect. Revist in 3.2 to see if it's still required. Fixes #10187

Note: See TracTickets for help on using tickets.