New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wpcom_vip_maybe_skip_old_slug_redirect(): Improve compatibility with local setups #1381
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor typo/carry over from a previous version of the function.
vip-helpers/vip-caching.php
Outdated
|
||
//We look to see if a malformed url (represented by 'http:' ) is right after the starting / in DOCUMENT_URI hence position 1 | ||
if ( is_404() && ( 1 === strpos( $_SERVER['DOCUMENT_URI'], 'http:' ) || 1 === strpos( $_SERVER['DOCUMENT_URI'], 'https:' ) ) ) { | ||
if ( is_404() && ( 1 === strpos( $uri, 'http:' ) || 1 === strpos( $uri, 'https:' ) ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're comparing to e.g.: http://othersite.com/random_text and looking to match the start of the string, this should be comparisons to 0 since string indexes start at 0.
if ( is_404() && ( 1 === strpos( $uri, 'http:' ) || 1 === strpos( $uri, 'https:' ) ) ) { | |
if ( is_404() && ( 0 === strpos( $uri, 'http:' ) || 0 === strpos( $uri, 'https:' ) ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brandon-m-skinner How does that match up with the comment then?
//We look to see if a malformed url (represented by 'http:' ) is right after the starting / in DOCUMENT_URI hence position 1
As per the function DocBlock, what we're looking for is an invalid URL e.g. http://example.com/http://othersite.com/random_text
, which means the DOCUMENT_URI and REQUEST_URI would be /http:...
, so looking for http:
or https:
starting at index 1.
We could just start at index 0 and look for /http:
or /https:
if that makes it easier to understand? (I think it would be)
Also, do we really need to check for DOCUMENT_URI if REQUEST_URI seems to be the most popular key to check? Compare instances of DOCUMENT_URI to instances of REQUEST_URI just in this repo.
Switch `$_SERVER[‘DOCUMENT_URI’]` to `$_SERVER[‘REQUEST_URI’] since that is the more popular key to be supported, particularly in local setups. Amends logic by searching for `/http:` at position 0 instead of `http:` at position 1 (and likewise for “https:”). Tidies documentation. Fixes #976.
b0e5c2a
to
21e4d08
Compare
Re-jigged, and rebased on to latest |
r961-stacks |
Description
Add support for local development servers that do not have
$_SERVER['DOCUMENT_URI']
defined.Fixes #976.
Needs actual testing.
Checklist
Please make sure the items below have been covered before requesting a review:
Steps to Test
$_SERVER['DOCUMENT_URI']
is undefined.