-
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
Log class rework #472
Log class rework #472
Conversation
@@ -43,7 +43,7 @@ | |||
$_GET['id'] = (int) $_GET['id']; | |||
|
|||
if (!$userbank->GetProperty("user", $_GET['id'])) { | |||
$log = new CSystemLog("e", "Getting admin data failed", "Can't find data for admin with id '" . $_GET['id'] . "'"); | |||
Log::add("e", "Getting admin data failed", "Can't find data for admin with id $_GET[id]."); |
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.
Seems like the same here and a few other places
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.
PHP resolves variables in double quoted strings. Variable parsing
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.
I'm so out of date with PHP, I always thought you had to use curly
@@ -76,7 +76,7 @@ function BlockPlayer($check, $sid, $num, $type, $length) | |||
|
|||
if (!$userbank->HasAccess(ADMIN_OWNER | ADMIN_ADD_BAN)) { | |||
$objResponse->redirect("index.php?p=login&m=no_access", 0); | |||
$log = new CSystemLog("w", "Hacking Attempt", $username . " tried to process a playerblock, but doesnt have access."); | |||
Log::add("w", "Hacking Attempt", "$username tried to process a playerblock, but doesnt have access."); |
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.
$username
is interpreted as a string?
$aid = filter_var($_SESSION['aid'], FILTER_VALIDATE_INT) ? $_SESSION['aid'] : -1; | ||
$host = filter_var($_SERVER['REMOTE_ADDR'], FILTER_VALIDATE_IP) ? $_SERVER['REMOTE_ADDR'] : ''; | ||
|
||
self::$dbs->query( |
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.
I thought it's prepare()
not query()
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.
SourceBans++'s proprietary database class has some function name differences to PDO
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.
Gotcha, that func name is quite misleading
echo 'You don\'t have access to this!'; | ||
die(); | ||
Log::add("w", "Hacking Attempt", $userbank->GetProperty('user')." tried to upload a demo, but doesn't have access."); | ||
die("You don't have access to this!"); |
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.
Using double-quotes not recommended by official guides, if string not includes PHP variables.
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.
To be honest that is such a minor detail and since there isn't really a performance benefit of using single quotes
vs double quotes
I would pass this one of and would concentrate on more serious issues like not using Dependency Injection
.
Description
Reworked the log class to have a cleaner code base.
Motivation and Context
Cleaner code base.
Features needed until merging
Testing needed until merging
Types of changes
Checklist: