my first script. can you review?

[Zangeel]

been studying php for days now n i finally put this together in a few minutes, i want to see if i did everything correctly :) can you check it for mistakes? and add suggestions!

is a registration script.

<?php

/**
* @Author Me
**/

if(isset($POST["submit"])) {


if(!$_POST["username"] || !$_POST["pass"] || !$_POST["pass2"] || !$_POST["email"]) {

die("You did not fill out the required fields!");

} else {

if(!get_magic_quotes_gpc()) {
$_POST["username"] = addslashes($_POST["username"]);
$_POST["password"] = addslashes($_POST["password"]);
$_POST["email"] = addslashes($_POST["email"]);
$_POST["username"] = mysql_real_escape_string($_POST["username"]);
$_POST["password"] = mysql_real_escape_string($_POST["password"]);
$_POST["email"] = mysql_real_escape_string($_POST["email"]);

}

if($_POST["pass"] != $_POST["pass2"]) {
die("Passwords Did Not Match!");
}

$usercheck = $_POST["username"];
$check = mysql_query("SELECT username FROM users WHERE username = '$usercheck'");

if(mysql_num_rows($check) != 0) {
die("Username Is in Use!");
} else {

$_POST["pass"] = md5($_POST["pass"]);

$insert = "INSERT INTO users (username, password, email)
VALUES ('".$_POST['username']."', '".$_POST['pass']."', '".$_POST["email"]."')";
mysql_query($insert);


echo("<font size=5> Registration Successfull. Please Login! </font>");

}
}


} else {
?>


<form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
<table border="0">
<tr><td>Username:</td><td>
<input type="text" name="username" maxlength="60">
</td></tr>
<tr><td>Password:</td><td>
<input type="password" name="pass" maxlength="10">
</td></tr>
<tr><td>Confirm Password:</td><td>
<input type="password" name="pass2" maxlength="10">
</td></tr>
<tr><td>E-Mail:</td><td>
<input type="text" name="email" maxlength="50">
</td></tr>
<tr><th colspan=2><br /><input type="submit" name="submit" value="Register"></th></tr> </table>
</form>
<?php
}
?>


thanks!

[Iszak]

Just quickly looking over it, a) you wouldn't want to use die because this won't display the form.. it'll terminate the script. b) What if get_magic_quotes_gpc() existing? It won't escape them etc etc? Also what about checking password one to password two, it doesn't account for case sensative, so use md5 and check the strings.

[Zangeel]

thanks for the reply :)

hows this revision:


<?php

/**
* @Author Me
**/

if(isset($POST["submit"])) {


if(!$_POST["username"] || !$_POST["pass"] || !$_POST["pass2"] || !$_POST["email"]) {

echo("You did not fill out the required fields!");

} else {

if(!get_magic_quotes_gpc()) {
$_POST["username"] = addslashes($_POST["username"]);
$_POST["password"] = addslashes($_POST["password"]);
$_POST["email"] = addslashes($_POST["email"]);
}

$_POST["username"] = mysql_real_escape_string($_POST["username"]);
$_POST["password"] = mysql_real_escape_string($_POST["password"]);
$_POST["email"] = mysql_real_escape_string($_POST["email"]);



if(md5($_POST["pass"]) != md5($_POST["pass2"])) {
echo("Passwords Did Not Match!");
} else {

$usercheck = $_POST["username"];
$check = mysql_query("SELECT username FROM users WHERE username = '$usercheck'");

if(mysql_num_rows($check) != 0) {
echo("Username Is in Use!");
} else {

$_POST["pass"] = md5($_POST["pass"]);

$insert = "INSERT INTO users (username, password, email)
VALUES ('".$_POST['username']."', '".$_POST['pass']."', '".$_POST["email"]."')";
mysql_query($insert);


echo("<font size=5> Registration Successfull. Please Login! </font>");

}
}
}


} else {
?>


<form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
<table border="0">
<tr><td>Username:</td><td>
<input type="text" name="username" maxlength="60">
</td></tr>
<tr><td>Password:</td><td>
<input type="password" name="pass" maxlength="10">
</td></tr>
<tr><td>Confirm Password:</td><td>
<input type="password" name="pass2" maxlength="10">
</td></tr>
<tr><td>E-Mail:</td><td>
<input type="text" name="email" maxlength="50">
</td></tr>
<tr><th colspan=2><br /><input type="submit" name="submit" value="Register"></th></tr> </table>
</form>
<?php
}
?>



look better?

[Iszak]

Yeah it looks fine except when giving them slashes you're using $_POST['password' when the form displayed no input called 'password'. As well as I would have put the check if the passwords are the same as an 'elseif' after checking if any of the fields are blank, here's my take of it, untested of course.

<?php

if (isset($POST["submit"]))
{
if(!$_POST["username"] || !$_POST["pass"] || !$_POST["pass2"] || !$_POST["email"])
{
echo "You did not fill out the required fields!";
}

elseif (md5($_POST["pass"]) != md5($_POST["pass2"]))
{
echo "Passwords Did Not Match!";
}

else
{
if (!get_magic_quotes_gpc())
{
$username = addslashes($_POST["username"]);
$password = addslashes($_POST["pass"]);
$email = addslashes($_POST["email"]);
}

else
{
$username = mysql_real_escape_string($_POST["username"]);
$password = mysql_real_escape_string($_POST["pass"]);
$email = mysql_real_escape_string($_POST["email"]);
}

$sql = "SELECT username FROM users WHERE username='$username'";
$query = mysql_query($sql);

if (mysql_num_rows($check) != 0)
{
echo "Username Is in Use!";
}

else
{
$password = md5($password);

$sql = "INSERT INTO users
(username, password, email) VALUES
('{$username}', '{$password}', '{$email}')";
mysql_query($sql);
echo "<font size=5> Registration Successfull. Please Login!</font>";
}
}
}

else
{
?>


<form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
<table border="0">
<tr>
<td>Username:</td>
<td><input type="text" name="username" maxlength="60"></td>
</tr>
<tr>
<td>Password:</td>
<td><input type="password" name="pass" maxlength="10"></td>
</tr>
<tr>
<td>Confirm Password:</td>
<td><input type="password" name="pass2" maxlength="10">
</td>
</tr>
<tr>
<td>E-Mail:</td>
<td><input type="text" name="email" maxlength="50"></td>
</tr>
<tr>
<th colspan=2>
<br />
<input type="submit" name="submit" value="Register">
</th>
</tr>
</table>
</form>
<?php
}
?>


As you can see I tend to spread my code out more over more lines, it makes it easier for me to read. As well as the HTML. Unlike HTML PHP ignores the whitespace IIRC. Again it's untested.

[Fou-Lu]

I would reverse the idea of how the magic quotes are checked:

if (get_magic_quotes_gpc())
{
$_POST['value'] = stripslashes($_POST['value']);
}

$_POST['value'] = mysql_real_escape_string($_POST['value']);

Save yourself 6 bytes of code ;)
On top of it, you can create an included global file that recursively strips everything out of the superglobals without needing to be concerned if mysql_real_escape_string is being checked against.

This is still 10x better than the one you found on the net though. Glad to see that security is your #1 priority, thats a good practice that will follow you in all of your languages.

[Zangeel]

thanks a lot guys this is really helpin me out.

Optimized Script:
<?php

/**
* @Author Me
**/

if(isset($POST["submit"])) {


if(!$_POST["username"] || !$_POST["pass"] || !$_POST["pass2"] || !$_POST["email"]) {

echo("You did not fill out the required fields!");

} elseif(md5($_POST["pass"]) != md5($_POST["pass2"])) {

echo("Passwords Did Not Match!");

}
else {

if(get_magic_quotes_gpc()) {
$username = stripslashes($_POST["username"]);
$password = stripslashes($_POST["pass"]);
$email = stripslashes($_POST["email"]);
}

$username = mysql_real_escape_string($_POST["username"]);
$password = mysql_real_escape_string($_POST["pass"]);
$email = mysql_real_escape_string($_POST["email"]);

$check = mysql_query("SELECT username FROM users WHERE username = '$username'");

if(mysql_num_rows($check) != 0) {
echo("Username Is in Use!");

}

else

{


$password = md5($password);

$insert = "INSERT INTO
users (username, password, email)
VALUES ('{$username}', '{$password}', '{$email}')";

mysql_query($insert);


echo("<font size=5> Registration Successfull. Please Login! </font>");

}
}
} else {

?>


<form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
<table border="0">
<tr>
<td>Username:</td>
<td><input type="text" name="username" maxlength="60"></td>
</tr>
<tr>
<td>Password:</td>
<td><input type="password" name="pass" maxlength="10"></td>
</tr>
<tr>
<td>Confirm Password:</td>
<td><input type="password" name="pass2" maxlength="10">
</td>
</tr>
<tr>
<td>E-Mail:</td>
<td><input type="text" name="email" maxlength="50"></td>
</tr>
<tr>
<th colspan=2>
<br />
<input type="submit" name="submit" value="Register">
</th>
</tr>
</table>
</form>
<?php
}
?>

[oesxyl]

anything you write now, you must rewrite later, so a better thing is to learn to use function or even better oop. This way you have also the avantage of debuging/fixing/testing using tests files of small snippet of code instead of a complex one.

regards

[Zangeel]

yea learnin OOP is my next step but im tryin to get the basics down, like storin things in the DB, cookies, sessions, then hopefully ill get to OOP

[oesxyl]

yea learnin OOP is my next step but im tryin to get the basics down, like storin things in the DB, cookies, sessions, then hopefully ill get to OOP
learn to design, implementation is not a big deal. You can use any language after 6 month of work with him, and in 2 years you can be like a fish in the water, but this is only implementation, learning language construct, library, .... :)
design is not language dependent and all this stuff you mention can be implemented( and is) in other language so you must understand how it work not how is implemented in php.
More than that, almost all php designers know that php oop is not a good place to start with oop.

regards

[Fou-Lu]

learn to design, implementation is not a big deal. You can use any language after 6 month of work with him, and in 2 years you can be like a fish in the water, but this is only implementation, learning language construct, library, .... :)
design is not language dependent and all this stuff you mention can be implemented( and is) in other language so you must understand how it work not how is implemented in php.
More than that, almost all php designers know that php oop is not a good place to start with oop.

regards

Totally agreed. My first PHP OOP (except for db connections) was in 5.0.1. Bad choice, their oop engine was a great start but questionable in its outcome (children calls to private methods triggering parents private methods, what a nightmare). Since its gotten a lot better.
For an good oop language to start with, I'd probably recommend java. A lot can be done with it, and its jvm executed so you don't need to worry too much about screwing up.
Remember, languages are merely tools that allow us to create things with programming. Programming itself is a science that once learned can be applied with any tool - except lisp, I hate lisp >.<