3
\$\begingroup\$

Currenty coding a PHP script that will have a CRON to auto run it. The point is that it checks the user in Mysql and if it has expired, it will delete it from the admin table and set status to expired. I have used time() and then when it gets INSERT When his trial starts, it starts for example "1347054850" in the "Start" and then i add "900" on it using +, and the result is "1347055750", So the trial is for 15 minutes. But the point, will this scipt work propery or does it have issues?

<?php
$Sql = new mysqli("localhost", "root", "*******", "serveradmin");
if ($Sql->connect_error) { die("Sorry, Could not connect (".$Sql->connect_errno.") ".$Sql->connect_error);}
$Data = $Sql->query("SELECT * FROM trial");
while ($q = $Data->fetch_assoc()) {
$Start = $q['Start'];
$Stop = $q['Stop'];
$Steam = $q['Steam'];
$Result = $Start - $Stop;
$Time = time();
if ($q['Expired'] == "True") {echo "All expired \n";} else {
if ($Stop <= $Time) { 
echo "Deleted ".$Steam." From trial Reason: Expired"; 
$Sql->Query("DELETE FROM `sm_admins` WHERE `identity` = '".$Steam."'");
$Sql->Query("UPDATE trial SET `Expired` = 'True' WHERE `Steam` = '".$Steam."'");
} 
else 
{ 
echo "All ok, 0 deleted"; 
}
}
}
?>
\$\endgroup\$
1
  • \$\begingroup\$ what is steam in $steam =$q['steam'];? \$\endgroup\$ – pranay kumar Jun 11 '19 at 7:43
5
\$\begingroup\$
  1. Proper and consistent indentation would improve the readability a lot. Here is mine:

    <?php
        $Sql = new mysqli("localhost", "root", "*******", "serveradmin");
        if ($Sql->connect_error) { 
            die("Sorry, Could not connect (" . $Sql->connect_errno . ") " . $Sql->connect_error);
        }
        $Data = $Sql->query("SELECT * FROM trial");
        while ($q = $Data->fetch_assoc()) {
            $Start = $q['Start'];
            $Stop = $q['Stop'];
            $Steam = $q['Steam'];
            $Result = $Start - $Stop;
            $Time = time();
            if ($q['Expired'] == "True") {
                echo "All expired \n";
            } else {
                if ($Stop <= $Time) { 
                    echo "Deleted " . $Steam . " From trial Reason: Expired"; 
                    $Sql->Query("DELETE FROM `sm_admins` WHERE `identity` = '" . $Steam . "'");
                    $Sql->Query("UPDATE trial SET `Expired` = 'True' WHERE `Steam` = '" . $Steam . "'");
                } else { 
                    echo "All ok, 0 deleted"; 
                }
            }
        }
    ?>
    
  2. These conditions are good candidates for guard clauses:

    if ($q['Expired'] == "True") {
        echo "All expired \n";
    } else {
    

    and

    if ($Stop <= $Time) { 
    

    Here is the modified version:

    while ($q = $Data->fetch_assoc()) {
        $Start = $q['Start'];
        $Stop = $q['Stop'];
        $Steam = $q['Steam'];
        $Result = $Start - $Stop;
        $Time = time();
        if ($q['Expired'] == "True") {
            echo "All expired \n";
            continue;
        }
        if ($Stop > $Time) { 
            echo "All ok, 0 deleted"; 
            continue;
        }
        echo "Deleted " . $Steam . " From trial Reason: Expired"; 
        $Sql->Query("DELETE FROM `sm_admins` WHERE `identity` = '" . $Steam . "'");
        $Sql->Query("UPDATE trial SET `Expired` = 'True' WHERE `Steam` = '" . $Steam . "'");
    }
    

    References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code

  3. Furthermore, if you are not interested in those records whose Expired flag is True and Stop attribute is bigger than the current time you could modify the SQL query to filter those records, for example:

    $Data = $Sql->query("SELECT * FROM trial WHERE Expired != `True` AND Stop > " . $Time);
    

    (If you are using string concatenation to create SQL commands be careful about SQL injections attacks. The value of $Time comes from the time() function so it can't contain any malicious data here.)

  4. I'd consider using MySQL's datetime attributes for storing the date informations and using MySQL's date and time functions for the comparison. I'd improve the database structure and make the raw data readable. You might want to prepare for situations when the current time is different between the webserver and the database server.

  5. $mysql->query has a return value. You might want to check whether it's FALSE and print a proper error message.

  6. PHP functions are not case sensitive, but according to this answer you might want to modify ->Query(...) to ->query(...).

  7. This line seems unnecessary, since the $Result variable is not used:

    $Result = $Start - $Stop;
    
  8. The delete and update statement uses the $Steam variable as the part of the query without escaping it. I think it could cause 2nd order SQL attacks.

\$\endgroup\$
2
\$\begingroup\$

Everything must be done on the database level. It is incomparably resource-friendly. Instead of fetching all the data from database and looping over many rows most of which do not even meet the condition, we must set this condition right in the query, to fetch the only rows we need.

Better yet, instead of deleting and updating rows one by one, we must delete and update based on the condition as well.

The logic with information messages is also a bit flawed so I fixed it as well. You cannot tell "All ok, 0 deleted" if only one row is OK.

And as a last stroke, lets make the mysqli connection more robust and also put it in a separate file.

<?php
include 'mysqli.php';

$time = time();
$res = $mysqli->query("SELECT Steam FROM trial WHERE Stop <= $time");
$expired = $res->fetch_all();

if ($expired) {
    $mysqli->query("UPDATE trial SET `Expired` = 'True' WHERE Stop <= $time");
    $mysqli->query("DELETE FROM sm_admins a, trial t 
                  WHERE t.Expired='True' AND a.identity = t.Steam");
    foreach ($expired as $row) {
        echo "Deleted $row[0] From trial Reason: Expired\n"; 
    }
} else { 
    echo "All ok, 0 deleted"; 
}
\$\endgroup\$
1
  • \$\begingroup\$ It might be worth a note justifying how you trust $time to be interpolated, which would normally ring query-injection alarm bells. \$\endgroup\$ – Toby Speight Jun 11 '19 at 11:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.