WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#10187 closed defect (bug) (fixed)

wp_redirect and canonical redirect do not work as expected on IIS

Reported by: peaceablewhale Owned by: 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 8 years ago.
10187_ruslany.patch (1.3 KB) - added by ruslany 8 years ago.
Updated patch that preserves the fixes for apache/cgi
10187_drop_iis5.patch (1.2 KB) - added by peaceablewhale 8 years ago.
Removed the call to status_header()
10187-10435-merged.patch (4.9 KB) - added by peaceablewhale 8 years ago.
10187-10435-merged.2.patch (4.8 KB) - added by peaceablewhale 7 years ago.
10187-16880-merged.patch (2.9 KB) - added by kcristiano 7 years ago.
Refreshed to rev 16880

Download all attachments as: .zip

Change History (52)

#1 @peaceablewhale
8 years ago

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

#2 @peaceablewhale
8 years ago

Patch updated due to #10186.

#3 @westi
8 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
8 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
8 years ago

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

#6 @westi
8 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
8 years ago

How about dropping support to IIS <=5 completely?

#8 @peaceablewhale
8 years ago

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

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

#9 @ruslany
8 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
8 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
8 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
8 years ago

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

#13 @Denis-de-Bernardy
8 years ago

  • Keywords needs-patch added; has-patch removed

@ruslany
8 years ago

Updated patch that preserves the fixes for apache/cgi

#14 @ruslany
8 years ago

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

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

@peaceablewhale
8 years ago

Removed the call to status_header()

#15 follow-up: @peaceablewhale
8 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
8 years ago

Related: #10435

#17 in reply to: ↑ 15 @hakre
8 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
8 years ago

Related: #3528

#19 @peaceablewhale
8 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
8 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
8 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
7 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
7 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
7 years ago

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

#26 @peaceablewhale
7 years ago

Patch updated for WordPress 3.0.

#27 @dd32
7 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
7 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
7 years ago

  • Milestone changed from Awaiting Triage to 3.1

#30 follow-up: @dd32
7 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
7 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
7 years ago

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

#33 @peaceablewhale
7 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
7 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
7 years ago

  • Keywords dev-feedback added; tested early removed

Dion, what's left here for 3.1?

#36 @dd32
7 years ago

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

#37 @dd32
7 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
7 years ago

  • Cc azizur added

#39 follow-up: @nacin
7 years ago

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

#40 @peaceablewhale
7 years ago

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

#41 in reply to: ↑ 39 @kcristiano
7 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
7 years ago

Refreshed to rev 16880

#42 @dd32
7 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
7 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
7 years ago

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

#45 @dd32
7 years ago

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

#46 @dd32
7 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.