Opened 16 years ago
Closed 14 years ago
#10187 closed defect (bug) (fixed)
wp_redirect and canonical redirect do not work as expected on IIS
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (52)
#3
@
16 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
@
16 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
@
16 years ago
The existing WordPress code prevents newer IIS to send correct HTTP redirect headers and I am trying to fix that.
#6
@
16 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.
#8
@
16 years ago
- Keywords has-patch needs-review added; needs-patch removed
A patch that drops support to IIS <=5 has been uploaded
#9
@
16 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
@
15 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
@
15 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.
#14
@
15 years ago
- Keywords has-patch needs-testing added; needs-patch removed
Attached an updated patch that preserves the fixes for apache/cgi.
#15
follow-up:
↓ 17
@
15 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.
#17
in reply to:
↑ 15
@
15 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.
#19
@
15 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
@
15 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
@
15 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
@
15 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
@
15 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.
#27
@
15 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
@
15 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.
#30
follow-up:
↓ 31
@
15 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
@
15 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
#34
@
15 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
@
14 years ago
- Keywords dev-feedback added; tested early removed
Dion, what's left here for 3.1?
#37
@
14 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
#39
follow-up:
↓ 41
@
14 years ago
iis7_supports_permalinks() is only defined in the admin. That causes an error in redirect_canonical.
#41
in reply to:
↑ 39
@
14 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)
#42
@
14 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
@
14 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.
This patch depends on #10186. Tested with PHP 5.2.9-2 on IIS 5.1 via FastCGI.