Streamlining Script PHP

[StefanVonS]

Can anyone suggest if there is a way to compress this code a bit? I'm new to PHP and want to learn how to efficiently code from the start. If you have a suggestion, assume I'm an idiot when explaining

Code: Select all

<?php //register.php
//DB Login information & Errors
$path = $_SERVER['DOCUMENT_ROOT'];
require "$path/Smarty/Smarty.class.php";

$smarty = new Smarty();
$smarty->template_dir = "$path/temp/smarty/templates";
$smarty->compile_dir  = "$path/temp/smarty/templates_c";
$smarty->cache_dir    = "$path/temp/smarty/cache";
$smarty->config_dir   = "$path/temp/smarty/configs";

require_once 'login.php';
$db_server = mysql_connect($db_hostname, $db_username, $db_password);
if (!$db_server) die("Unable to connect to MySQL: " . mysql_error());
mysql_select_db($db_database)
   or die("Unable to select database: " . mysql_error());

//Salt Declaration
$salt1 = "qm&h*";
$salt2 = "pg!@";
 if (isset($_POST['delete']) && $deleteuser != "")
   {
                $deleteuser = mysql_entities_fix_string($_POST['deleteuser']);
      $query = "DELETE FROM users WHERE username='$deleteuser'";
                mysql_query($query);
      if (!mysql_query($query))
      {
         echo "DELETE failed: $query<br>" .
         mysql_error() . "<p>";
      }
   }
if(isset($_POST['fname']) && ($_POST['sname']) && ($_POST['user']) && ($_POST['pass']))

    {
        //Move info from the form, if it has been filled out, clean it, and pass it to new variables
        $fnametemp = mysql_entities_fix_string($_POST['fname']);
        $snametemp = mysql_entities_fix_string($_POST['sname']);
        $usertemp = mysql_entities_fix_string($_POST['user']);
        $passtemp = mysql_entities_fix_string($_POST['pass']);
       
        $dupcheck = "SELECT * FROM users WHERE username = '$usertemp'";

       
            $testresult = mysql_query($dupcheck);
            $rows = mysql_num_rows($testresult);

         if($rows==0)

        {
    //Create an MD5 hash of the password line utilizing the salts declared above
    $passfinal = MD5("$salt1$passtemp$salt2");


        $query = "INSERT INTO users VALUES ('$fnametemp','$snametemp','$usertemp','$passfinal')";

        $result = mysql_query($query);
            if (!$result) die ("Database access failed: " . mysql_error());
            echo <<<_END
<pre>
Thank you $fnametemp, you have successfully registered.  Please continue to <a href=/TempProject/authenticate.php>Log In</a>
</pre>
_END;

        }

        else die("Duplicate User");

    }

else
{
    echo <<<_END



_END;

}
$query = "SELECT * FROM users";
$result = mysql_query($query);
$rows = mysql_num_rows($result);
for ($j = 0 ; $j < $rows ; ++$j)
{
   $results[] = mysql_fetch_array($result);
}

mysql_close($db_server);
$smarty->assign('results', $results);
$smarty->display("register.tpl");


function mysql_entities_fix_string($string)
{
   return htmlentities(mysql_fix_string($string));
}

function mysql_fix_string($string)
{
   if (get_magic_quotes_gpc()) $string = stripslashes($string);
   return mysql_real_escape_string($string);
}


?>


[morphine]

- Line 21: I see you have ' if $deleteuser!="" ', however $deleteuser at this point is not declared.
- Lines 25/26: you're doing the same query twice, not necessary.
- Line 32: " if(isset($_POST['fname']) && ($_POST['sname']) && ($_POST['user']) && ($_POST['pass'])) " - I'm pretty sure that here you really meant " if ( isset($_POST['fname']) && isset($_POST['sname']) ... ) "
- Lines 72-76: what's this echo() statement for, if it's empty?
- mysql_entities_fix_string - I see here that you're HTML-encoding your strings for database queries. Didn't you really want mysql_real_escape_string()?
- Avoid any sort of HTML output in the middle of your code. I see that you've already learned of Smarty, use it for all your output.
- INSERT and UPDATE queries do not return a result set. What you might want to check, however, is the number of affected rows, using mysql_affected_rows().
- Sometimes it's handier to use empty() instead of isset() because you can verify that the target variable is both set and contains a value.

General advice:

- Be consistent in your indentation. You'll thank me in 6 months when you get have to back to this and have forgotten about it.
- if(), for(), etc: if you have a single statement, you don't need the curly braces. I.e.: " if( something ) do_something; " is equivalent to "if ( something ) { do_something }".
- Database row loops: there's a slightly simpler form, i.e.:

Code: Select all

while( $row=mysql_fetch_row($result ) )
 $results[]= $row;


- Always define constants for your paths and include a file with those. That is, create a file called for instance "config.php" where you state your path constants and other stuff, i.e.:

Code: Select all

define('SMARTY_ROOT', $_SERVER['DOCUMENT_ROOT'].'/temp/smarty' );
define('DB_SERVER', 'localhost');
define('SALT1', "qm&h*");
... and so on.


- Former advice also applies to utility functions, like "mysql_entities_fix_string". Keep these in a separate file and include as necessary.

Clean version (IMO):

Code: Select all

<?php //register.php
//DB Login information & Errors
$path = $_SERVER['DOCUMENT_ROOT'];
require "$path/Smarty/Smarty.class.php";

require_once 'login.php';

$db_server = mysql_connect($db_hostname, $db_username, $db_password);

if ( !$db_server )
   die("Unable to connect to MySQL: " . mysql_error());
   
if ( !mysql_select_db($db_database) )
   die("Unable to select database: " . mysql_error());

$smarty = new Smarty();
$smarty->template_dir = "$path/temp/smarty/templates";
$smarty->compile_dir  = "$path/temp/smarty/templates_c";
$smarty->cache_dir    = "$path/temp/smarty/cache";
$smarty->config_dir   = "$path/temp/smarty/configs";

//Salt Declaration
$salt1 = "qm&h*";
$salt2 = "pg!@";

if (isset($_POST['delete']) ) {

   $deleteuser = mysql_entities_fix_string($_POST['deleteuser']);
   
   $query = "DELETE FROM users WHERE username='$deleteuser'";
   
   if ( !mysql_query($query) ) {
   
         echo "DELETE failed: $query<br>" .
         mysql_error() . "<p>";
   }
}

if( !empty($_POST['fname']) && !empty($_POST['sname']) && !empty($_POST['user']) && !empty($_POST['pass']) ) {

   //Move info from the form, if it has been filled out, clean it, and pass it to new variables
   $fnametemp = mysql_entities_fix_string($_POST['fname']);
   $snametemp = mysql_entities_fix_string($_POST['sname']);
   $usertemp = mysql_entities_fix_string($_POST['user']);
   $passtemp = mysql_entities_fix_string($_POST['pass']);
       
   $dupcheck = "SELECT * FROM users WHERE username = '$usertemp'";

   $testresult = mysql_query($dupcheck);
   $rows = mysql_num_rows($testresult);

   if ( $rows>0 )
      die("Duplicate user");

   //Create an MD5 hash of the password line utilizing the salts declared above
   $passfinal = md5("$salt1$passtemp$salt2");

   $query = "INSERT INTO users VALUES ('$fnametemp','$snametemp','$usertemp','$passfinal')";

   if (! mysql_query($query) )
      die ("Database access failed: " . mysql_error());
      
   echo <<<_END
<pre>
Thank you $fnametemp, you have successfully registered.  Please continue to <a href=/TempProject/authenticate.php>Log In</a>
</pre>
_END;
}

$query = "SELECT * FROM users";

$result = mysql_query($query);
$rows = mysql_num_rows($result);

while( $row=mysql_fetch_array($result) )
   $results[]= $row;

mysql_close($db_server);

$smarty->assign('results', $results);
$smarty->display("register.tpl");


function mysql_entities_fix_string($string) {

   return htmlentities(mysql_fix_string($string));
}

function mysql_fix_string($string) {

   if (get_magic_quotes_gpc())
      $string = stripslashes($string);
      
   return mysql_real_escape_string($string);
}
?>

[StefanVonS]

I'm still working through your suggestions, but meanwhile, my intent with the htmlentities function is to render useless any and all code out of client input, then pass it through the mysqlfixstring function to further strip out any mysql code. Are we on the same page?

Code: Select all

function mysql_entities_fix_string($string)
{
   return htmlentities(mysql_fix_string($string));
}

function mysql_fix_string($string)
{
   if (get_magic_quotes_gpc()) $string = stripslashes($string);
   return mysql_real_escape_string($string);
}


[Nitrodist]

Can we get line numbers built into the [code] sections?

[morphine]

I'm still working through your suggestions, but meanwhile, my intent with the htmlentities function is to render useless any and all code out of client input, then pass it through the mysqlfixstring function to further strip out any mysql code. Are we on the same page?

Yes, though I don't think that those functions are doing what you originally think.

First and foremost: mysql_real_escape_string() is the proper function to use when securing strings to be used into database queries. It supersedes mysql_escape_string(), which is deprecated, and takes multibyte character sets and what not into account. If you use this function, then there is no more tweaking necessary.

Second: what htmlentities does is transform input text with any sort of special characters into their HTML entity equivalent. For instance, inputting "a á <" will be transformed into "a &aacute; &lt;". This is something that you want to do on output, when pulling data from MySQL, not into it. For instance, it's always good security practice (to avoid XSS attacks) to run text through this function when said text has been previously input into the database by the user. Example:

Code: Select all

$res= mysql_query("SELECT post FROM posts WHERE id=1");

$row= mysql_fetch_assoc($res);

echo htmlentities($row['post']);


Third: related to the previous, browsers these days are all Unicode-aware. That means that you don't have to (and shouldn't) convert every possible character into its HTML entity. Which means that you can use htmlspecialchars() instead of htmlentities() (I've been guilty of doing this until fairly recently). This applies especially if you're doing any sort of exchanging data via JSON, which expects everything to be in UTF-8 (Unicode) by default.

Fourth: when you get around to switching from the mysql_*() functions to using the mysqli_*() functions, or PDO or something of the like, you can and should start using prepared statements for database insertions/updates. That means that instead of doing all that escaping and whatnot, you first prepare a query by doing "INSERT INTO posts ( post, subject ) VALUES ( ?, ? )" and then assigning variables to the question marks. The database library will take care of the rest of you. If you do stuff this way, you remove the human factor of forgetting to escape some string.