Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Reusable server url #14

Merged
merged 10 commits into from
Sep 13, 2014
Merged

Reusable server url #14

Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f4cb59e
Improve the re-usability of the addQueryArg() function
jrfnl Aug 31, 2014
8312dd5
Fix issues with dirname() on Windows
jrfnl Aug 31, 2014
5f8a332
Whitespace
jrfnl Aug 31, 2014
57ed71f
Code style
jrfnl Sep 2, 2014
12b8dc1
Small fixes
jrfnl Sep 3, 2014
c13099a
Missed one else
jrfnl Sep 3, 2014
d7c244f
Slight simplification
jrfnl Sep 3, 2014
69a1ef0
Merge branch 'master' of github.com:YahnisElsts/wp-update-server into…
jrfnl Sep 3, 2014
2ae6823
Merge branch 'master' of github.com:YahnisElsts/wp-update-server into…
jrfnl Sep 11, 2014
595b058
Minor changes / code style, method naming
jrfnl Sep 11, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 58 additions & 11 deletions includes/Wpup/UpdateServer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -26,6 +20,56 @@ 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() {
Copy link
Owner

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, adjusted.

$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 {
// Fix Windows
if ( dirname($path) === '\\' ) {
$path = '/';
} else {
$path = str_replace('\\', '/', dirname($path)) . '/';
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the issue with Windows? UNC paths? Network shares?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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, dirname($path) returns '' which in turn will give problems when using that in the url which is passed on in serialized/json format leading to urls like: http://myserver.commypath?query=test

@see http://php.net/manual/en/function.dirname.php

}
}
Copy link
Owner

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 dirname() in a variable though, to avoid doing that three times. Can you live with that ? I'll update the PR.


$serverUrl .= $path;
return $serverUrl;
}

/**
* Determine if ssl is used.
*
* @see WP core - wp-includes/functions.php
*
* @return bool True if SSL, false if not used.
*/
public static function isSsl() {
if ( isset($_SERVER['HTTPS']) ) {
if ( $_SERVER['HTTPS'] == '1' || strtolower($_SERVER['HTTPS']) === 'on' ) {
return true;
}
}
elseif ( isset($_SERVER['SERVER_PORT']) && ( '443' == $_SERVER['SERVER_PORT'] ) ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, the two above lines should be one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} elseif {, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, done.

return true;
}
return false;
}

/**
* Process an update API request.
*
Expand Down Expand Up @@ -214,8 +258,8 @@ 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['action']) ? $query['action'] : '-',
isset($query['slug']) ? $query['slug'] : '-',
isset($query['installed_version']) ? $query['installed_version'] : '-',
isset($wpVersion) ? $wpVersion : '-',
isset($wpSiteUrl) ? $wpSiteUrl : '-',
Expand Down Expand Up @@ -312,10 +356,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();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could just use $this->serverUrl here. It was already detected in the constructor; no need to do it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Passing $this->serverUrl in via the $url parameter would be good practice in the cases that's applicable.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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] . '?';
Expand Down