Reusable server url #14
Conversation
- Support SSL protocol - Make the serverUrl code re-usable
| @@ -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'] : '-', | |||
There was a problem hiding this comment.
The extra space is intentional: it's used to align similar statements.
There was a problem hiding this comment.
I sort of figured that, but then looked at the other five lines and saw you didn't space those out...
There was a problem hiding this comment.
Fair enough. Lets take out the space.
There was a problem hiding this comment.
Sorry, already put it back in ;-)
|
Updated the PR |
| $path = str_replace('\\', '/', dirname($path)) . '/'; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
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.
Can you think of an example path where that would actually happen? On Unix-like systems the directory separator would already be a forward slash, so the |
|
I was just thinking - and forgive me if this sounds ignorant -, but can unix paths contain the '' character as part of the path ? If it could, keeping the switch is best. Think a Unix path like |
Apparently, the answer is yes. I looked around and found a number of pages discussing ways to delete files with a backslash in the name. So it must be possible to create a file like that (why anyone would actually do that is beyond me, but there you go). |
|
Ok, then we should keep the if/else like it is. I'll update the PR with the current master and then it should be fine to merge. |
… Reusable-server-url Conflicts: includes/Wpup/UpdateServer.php
|
@YahnisElsts Anything I still need to do about this one before you can merge ? |
| return true; | ||
| } | ||
| } | ||
| elseif ( isset($_SERVER['SERVER_PORT']) && ( '443' == $_SERVER['SERVER_PORT'] ) ) { |
Just some minor things. See my comments above. |
| * | ||
| * @return string Url | ||
| */ | ||
| public static function determineServerUrl() { |
There was a problem hiding this comment.
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.
All done & PR updated. |
Refactor server URL detection as a separate static method
Improve the re-usability of the addQueryArg() method