-
Notifications
You must be signed in to change notification settings - Fork 175
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
Reusable server url #14
Changes from 4 commits
f4cb59e
8312dd5
5f8a332
57ed71f
12b8dc1
c13099a
d7c244f
69a1ef0
2ae6823
595b058
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,13 +11,7 @@ public function __construct($serverUrl = null, $serverDirectory = null) { | |
$serverDirectory = realpath(__DIR__ . '/../..'); | ||
} | ||
if ( $serverUrl === null ) { | ||
//Default to the current URL minus the query and "index.php". | ||
$serverUrl = 'http://' . $_SERVER['HTTP_HOST']; | ||
$path = $_SERVER['SCRIPT_NAME']; | ||
if ( basename($path) === 'index.php' ) { | ||
$path = dirname($path) . '/'; | ||
} | ||
$serverUrl .= $path; | ||
$serverUrl = self::determineServerUrl(); | ||
} | ||
|
||
$this->serverUrl = $serverUrl; | ||
|
@@ -26,6 +20,59 @@ public function __construct($serverUrl = null, $serverDirectory = null) { | |
$this->cache = new Wpup_FileCache($serverDirectory . '/cache'); | ||
} | ||
|
||
/** | ||
* Determine the Server Url based on the current request. | ||
* | ||
* Defaults to the current URL minus the query and "index.php". | ||
* | ||
* @static | ||
* | ||
* @return string Url | ||
*/ | ||
public static function determineServerUrl() { | ||
$serverUrl = ( self::isSsl() ? 'https' : 'http' ); | ||
$serverUrl .= '://' . $_SERVER['HTTP_HOST']; | ||
$path = $_SERVER['SCRIPT_NAME']; | ||
|
||
if ( basename($path) === 'index.php' ) { | ||
if ( DIRECTORY_SEPARATOR === '/' ) { | ||
$path = dirname($path) . '/'; | ||
} | ||
else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor quibble: Change this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
// Fix Windows | ||
if ( dirname($path) === '\\' ) { | ||
$path = '/'; | ||
} | ||
else { | ||
$path = str_replace('\\', '/', dirname($path)) . '/'; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the issue with Windows? UNC paths? Network shares? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, if the code is in the domain root 'myserver.com/' on a Windows server, |
||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at this bit again, it seems lines 38 to 47 could be simplified (untested code): $path = str_replace('\\', '/', dirname($path));
//Make sure there's a trailing slash.
if ( substr($path, -1) !== '/' ) {
$path .= '/';
} What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could do. Still would keep the Unix/Windows if/else to avoid accidentally changing a Unix path. Would put the |
||
|
||
$serverUrl .= $path; | ||
return $serverUrl; | ||
} | ||
|
||
/** | ||
* Determine if ssl is used. | ||
* | ||
* @return bool True if SSL, false if not used. | ||
*/ | ||
public static function isSsl() { | ||
if ( isset($_SERVER['HTTPS']) ) { | ||
if ( 'on' === strtolower($_SERVER['HTTPS']) ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No Yoda conditions, please. Just do a straight comparison. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I Intentionally didn't change the code style of this function as it's taken one-on-one from the WP core codebase. Not changing the code style makes it easier to compare for changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I assumed it was copied from somewhere. Still, it's only a 10 line method, I think it's safe to change the code style. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
return true; | ||
} | ||
if ( '1' == $_SERVER['HTTPS'] ) { | ||
return true; | ||
} | ||
} | ||
elseif ( isset($_SERVER['SERVER_PORT']) && ( '443' == $_SERVER['SERVER_PORT'] ) ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, the two above lines should be one line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, done. |
||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
/** | ||
* Process an update API request. | ||
* | ||
|
@@ -215,7 +262,7 @@ protected function logRequest($query) { | |
$columns = array( | ||
isset($_SERVER['REMOTE_ADDR']) ? str_pad($_SERVER['REMOTE_ADDR'], 15, ' ') : '-', | ||
isset($query['action']) ? $query['action'] : '-', | ||
isset($query['slug']) ? $query['slug'] : '-', | ||
isset($query['slug']) ? $query['slug'] : '-', | ||
isset($query['installed_version']) ? $query['installed_version'] : '-', | ||
isset($wpVersion) ? $wpVersion : '-', | ||
isset($wpSiteUrl) ? $wpSiteUrl : '-', | ||
|
@@ -312,10 +359,13 @@ protected function exitWithError($message, $httpStatus = 500) { | |
* You can also set an argument to NULL to remove it. | ||
* | ||
* @param array $args An associative array of query arguments. | ||
* @param string $url The old URL. | ||
* @param string $url The old URL. Optional, defaults to the request url without query arguments. | ||
* @return string New URL. | ||
*/ | ||
protected static function addQueryArg($args, $url) { | ||
protected static function addQueryArg($args, $url = null ) { | ||
if ( !isset($url) ) { | ||
$url = self::determineServerUrl(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we can't. The method is static so it only has access to static properties. Also: I'd advocate for leaving the method as static so you don't have to pass the updateServer object to other classes to have access to this method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI: just checked, in the one call within this class using this method $this->serverUrl is passed correctly, so it won't be evaluated again ;-) |
||
} | ||
if ( strpos($url, '?') !== false ) { | ||
$parts = explode('?', $url, 2); | ||
$base = $parts[0] . '?'; | ||
|
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.
Personally, I'd call this
guessServerUrl
. The update server can make a good guess, but it can't be 100% sure it has correctly determined the URL. Stuff like rewrite rules could still mess it up.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.
Agreed, adjusted.