MyBB Community Forums

Hi, I am learning the basics of PHP/MySQL, I have so far wrote my first script, can it be improved?

Many Thanks

include.php

<?php
mysql_connect("localhost", "root", "") or die(mysql_error());
mysql_select_db("user") or die(mysql_error());
$salt = "RaNdOmSa1tStR1Ng";
?>

login.php

<?php
session_start(); // session start
require_once('include.php');
$form = $_POST['submit'];
$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string(md5($salt.$_POST['password']));
if(isset($form)) {
 if(!empty($username) && !empty($password)) {
  $sql = mysql_query("SELECT * FROM `user` WHERE username='$username' and password='$password'");
    if( mysql_num_rows($sql) != 0 ) { //success
    $row=mysql_fetch_array($sql);
    $_SESSION['logged-in'] = true;
    $_SESSION['username'] = $row['username'];
     header('Location: user.php');
     exit;
    } else { $error = "Incorrect login details"; }
 } else { $error = "All information is not filled out correctly"; }
}
?>
Login
<?php
echo "$error";
?>
<form action="<?php echo htmlentities($_SERVER['PHP_SELF']); ?>" method="post">
<table cellspacing='1' cellpadding='5'>
<tr><td class='listtitle' colspan='2'>Enter Login Details</td></tr>
<tr><td class='list' align='right'>Username:</td><td class='list'><input name="username" type="text" /></td></tr>
<tr><td class='list' align='right'>Password:</td><td class='list'><input name="password" type="password" /></td></tr>
<tr><td class='listtitle' align='right' colspan='2'><input name="submit" type="submit" value="Log In" /></td></tr>
</table>
</form>

user.php

<?php
session_start(); // session start
require_once('include.php');
// is the one accessing this page logged in or not?
if (!isset($_SESSION['logged-in']) || $_SESSION['logged-in'] !== true) {
 
    // not logged in, move to login page
    header('Location: login.php');
    exit;
}
?>
Welcome Page <br>
<a href="logout.php">logout</a>

For a first script, it works pretty well. You might want to consider putting the session_start() in include.php if you're going to have it on each page.

A small thing which isn't really a hassle but a tip, with this line:

$password = mysql_real_escape_string(md5($salt.$_POST['password']));

there isn't much need to escape it if you're md5'ing it as well, since md5 is only hexadecimal.

As resig said, you don't need to use mysql_real_escape_string as you are using md5 already, the value returned by that function can't do any harm at all to mysql.

Everything else is pretty good IMO.

(2009-03-23, 10:02 AM)resig Wrote: [ -> ]For a first script, it works pretty well. You might want to consider putting the session_start() in include.php if you're going to have it on each page.

A small thing which isn't really a hassle but a tip, with this line:

$password = mysql_real_escape_string(md5($salt.$_POST['password']));

there isn't much need to escape it if you're md5'ing it as well, since md5 is only hexadecimal.

(2009-03-23, 04:29 PM)Sergio Montoya Wrote: [ -> ]As resig said, you don't need to use mysql_real_escape_string as you are using md5 already, the value returned by that function can't do any harm at all to mysql.

Everything else is pretty good IMO.

Thank You's Very Much

Looks good other than what the others have mentioned. I do suggest however, to organize your code in terms of indentation, line breaks, etc, that way you'll find things quicker, it'll be much more organized, and if you pass it on to others, they can easily adapt to it.
Best Regards.

You know what would be an awesome addition to the PHP code function in MYBB? Line numbers! Why don't we have line numbers!