White_Lily Posted November 6, 2012 Share Posted November 6, 2012 (edited) Hi I am trying to adjust my registration form because another web developer in the company said that all my empty() checking was making the form checking slow, and to put them into a foreach array with the same message for each failed field check. I tried to do this however i have come accross a parse error that makes no sense to me. Here is my code: <?php if($_POST["submitReg"]){ $validateArray = array("name" => "text", "email" => "email"); foreach($validateArray as $key => $value) { if($value == "text"){ $$key = htmlspecialchars(mysql_real_escape_string($_POST[$key])); }else{ $$key = mysql_real_escape_string($_POST[$key]); } if(empty($$key)) { $msg3 = "Please fill in all fields of this form."; } } $regName = htmlspecialchars(mysql_real_escape_string($_POST["name"])); $regUsername = htmlspecialchars(mysql_real_escape_string($_POST["username"])); $regPassword = htmlspecialchars(mysql_real_escape_string($_POST["password"])); $regREpassword = htmlspecialchars(mysql_real_escape_string($_POST["rePassword"])); $email = mysql_real_escape_string($_POST["email"]); if(preg_match("^[a-zA-Z0-9_.-]+@[a-zA-Z0-9-]+.[a-zA-Z0-9-.]+$", $_POST["email"]) === 0){ $msg3 .= "Please enter a valid email."; }else{ if($regPassword != $regREpassword || $regREpassword != $regPassword){ $msg3 .= "The password fields didn't match."; }else{ $check = select("*", "members", "username = '$regUsername'"); $assoc = mysql_fetch_assoc($check); if($regUsername == $assoc["username"]){ $msg3 .= "That username has been taken, pick another."; }else{ $regPassword = sha1($regPassword); $id = uniqid(); $register = insert("members", "name, email, username, password, user_level, id, ban", "'$regName', '$email', '$regUsername', '$regPassword', 1, '$id', 0"); if($register){ $newsTitle = "New member registered!"; $cont = $regUsername." has just joined Fusion Forums!<br>"; $cont.= "Check out his/her profile:<br><br>"; $cont.= "View Profile"; $newsCont = $cont; $newMem = insert("news", "news_title, news_content, username", "'$newsTitle', '$newsCont', '$regUsername'"); if($newMem){ $to = $email; $subject = "Fusion Forums - Account Confirmation"; $message = "Hello! You have recently registered to Fusion Forum's.<br><br>"; $message.= "This is a confirmation email, below you will find your account details along with a Unique ID.<br><br>"; $message.= "In order to activate your account you must first enter the ID into the text field that follows the link at the end of this email. Your details are as follows:<br><br>"; $message.= "<table>"; $message.= "<tr>"; $message.= "<td><strong>Name:</strong></td>"; $message.= "<td></td>"; $message.= "<td>".$regName."</td>"; $message.= "</tr>"; $message.= "<tr>"; $message.= "<td><strong>Email:</strong></td>"; $message.= "<td></td>"; $message.= "<td>".$email."</td>"; $message.= "</tr>"; $message.= "<tr>"; $message.= "<td><strong>Username:</strong></td>"; $message.= "<td></td>"; $message.= "<td>".$regUsername."</td>"; $message.= "<tr>"; $message.= "<tr>"; $message.= "<td><strong>Unique ID:</strong></td>"; $message.= "<td></td>"; $message.= "<td>".$id."</td>"; $message.= "<tr>"; $message.= "</table><br><br>"; $message.= "Please follow this link in order to activate your account (opens in your default browser):<br>"; $message.= "<a href='http://www.janedealsart.co.uk/activate.php?id=".$id."'>Activate Account</a>"; $from = "noreply@janedealsart.co.uk"; $headers = 'MIME-Version: 1.0' . "\r\n"; $headers .= 'Content-type: text/html; charset=iso-8859-1' . "\r\n"; $headers.= "From: ".$from; mail($to, $subject, $message, $headers); $done = "You have successfully registered to Fusion Fourm's.<br>"; $done.= "We have sent you an email with a confirmation code on it,"; $done.= " when you go to confirm your account you will need this code in order to be able to access the forum's."; $msg2 .= $done; }else{ $msg3 .= "Sorry, we could not register your account details, if this persists contact the webmaster. "; } }else{ $msg3 .= "Sorry, we could not register your account details, if this persists contact the webmaster. "; } } } } if($msg2){ echo '<div class="success">Success: '.$msg2.'</div>'; }else{ if($msg3){ echo '<div class="error">Error: '.$msg3.'</div>'; } echo '<form action="" method="POST">'; echo '<label>Full Name:</label>'; echo '<input type="text" class="field" name="name" />'; echo '<div class="clear"></div>'; echo '<label>Email:</label>'; echo '<input type="text" class="field" name="email" />'; echo '<div class="clear"></div>'; echo '<label>Username:</label>'; echo '<input type="text" class="field" name="username" />'; echo '<div class="clear"></div>'; echo '<label>Password:</label>'; echo '<input type="password" class="field" name="password" />'; echo '<div class="clear"></div>'; echo '<label>Again:</label>'; echo '<input type="password" class="field" name="rePassword" />'; echo '<div class="clear"></div>'; echo '<input type="submit" class="button" name="submitReg" value="Register" />'; echo '<div class="clear"></div>'; echo '</form>'; } ?> Here is the error: Parse error: syntax error, unexpected $end in /home/sites/janedealsart.co.uk/public_html/inc/regCheck.php on line 135 I am slightly confused as i did a search for the variable $end and it does not exist within the code... However this is line 135: ?> Any ideas as to why this is would be appreciated. (I'm not posting here to find out how vulnerable this form is to some types of injection, i just want to sort this error out.) Edited November 6, 2012 by White_Lily Quote Link to comment Share on other sites More sharing options...
snowdog Posted November 6, 2012 Share Posted November 6, 2012 Looks like to me there is an error in your nesting. You have double end braces. I nest differently then you do but I changed it over and you have a double end there in red and missing one below at bottom. I will add my nested code in also. }else{ $msg3 .= "Sorry, we could not register your account details, if this persists contact the webmaster. "; [color=#ff0000]} }[/color]else{ $msg3 .= "Sorry, we could not register your account details, if this persists contact the webmaster. "; } } } } <?php if($_POST["submitReg"]) { $validateArray = array("name" => "text", "email" => "email"); foreach($validateArray as $key => $value) { if($value == "text") { $key = htmlspecialchars(mysql_real_escape_string($_POST[$key])); } else { $key = mysql_real_escape_string($_POST[$key]); } if(empty($key)) { $msg3 = "Please fill in all fields of this form."; } } $regName = htmlspecialchars(mysql_real_escape_string($_POST["name"])); $regUsername = htmlspecialchars(mysql_real_escape_string($_POST["username"])); $regPassword = htmlspecialchars(mysql_real_escape_string($_POST["password"])); $regREpassword = htmlspecialchars(mysql_real_escape_string($_POST["rePassword"])); $email = mysql_real_escape_string($_POST["email"]); if(preg_match("^[a-zA-Z0-9_.-]+@[a-zA-Z0-9-]+.[a-zA-Z0-9-.]+$", $_POST["email"]) === 0) { $msg3 .= "Please enter a valid email."; } else { if($regPassword != $regREpassword || $regREpassword != $regPassword) { $msg3 .= "The password fields didn't match."; } else { $check = select("*", "members", "username = '$regUsername'"); $assoc = mysql_fetch_assoc($check); if($regUsername == $assoc["username"]) { $msg3 .= "That username has been taken, pick another."; } else { $regPassword = sha1($regPassword); $id = uniqid(); $register = insert("members", "name, email, username, password, user_level, id, ban", "'$regName', '$email', '$regUsername', '$regPassword', 1, '$id', 0"); if($register) { $newsTitle = "New member registered!"; $cont = $regUsername." has just joined Fusion Forums!<br>"; $cont.= "Check out his/her profile:<br><br>"; $cont.= "View Profile"; $newsCont = $cont; $newMem = insert("news", "news_title, news_content, username", "'$newsTitle', '$newsCont', '$regUsername'"); if($newMem) { $to = $email; $subject = "Fusion Forums - Account Confirmation"; $message = "Hello! You have recently registered to Fusion Forum's.<br><br>"; $message.= "This is a confirmation email, below you will find your account details along with a Unique ID.<br><br>"; $message.= "In order to activate your account you must first enter the ID into the text field that follows the link at the end of this email. Your details are as follows:<br><br>"; $message.= "<table>"; $message.= "<tr>"; $message.= "<td><strong>Name:</strong></td>"; $message.= "<td></td>"; $message.= "<td>".$regName."</td>"; $message.= "</tr>"; $message.= "<tr>"; $message.= "<td><strong>Email:</strong></td>"; $message.= "<td></td>"; $message.= "<td>".$email."</td>"; $message.= "</tr>"; $message.= "<tr>"; $message.= "<td><strong>Username:</strong></td>"; $message.= "<td></td>"; $message.= "<td>".$regUsername."</td>"; $message.= "<tr>"; $message.= "<tr>"; $message.= "<td><strong>Unique ID:</strong></td>"; $message.= "<td></td>"; $message.= "<td>".$id."</td>"; $message.= "<tr>"; $message.= "</table><br><br>"; $message.= "Please follow this link in order to activate your account (opens in your default browser):<br>"; $message.= "<a href='http://www.janedealsart.co.uk/activate.php?id=".$id."'>Activate Account</a>"; $from = "noreply@janedealsart.co.uk"; $headers = 'MIME-Version: 1.0' . "\r\n"; $headers .= 'Content-type: text/html; charset=iso-8859-1' . "\r\n"; $headers.= "From: ".$from; mail($to, $subject, $message, $headers); $done = "You have successfully registered to Fusion Fourm's.<br>"; $done.= "We have sent you an email with a confirmation code on it,"; $done.= " when you go to confirm your account you will need this code in order to be able to access the forum's."; $msg2 .= $done; } else { $msg3 .= "Sorry, we could not register your account details, if this persists contact the webmaster. "; } else { $msg3 .= "Sorry, we could not register your account details, if this persists contact the webmaster. "; } } } } if($msg2) { echo '<div class="success">Success: '.$msg2.'</div>'; } else { if($msg3) { echo '<div class="error">Error: '.$msg3.'</div>'; } } echo '<form action="" method="POST">'; echo '<label>Full Name:</label>'; echo '<input type="text" class="field" name="name" />'; echo '<div class="clear"></div>'; echo '<label>Email:</label>'; echo '<input type="text" class="field" name="email" />'; echo '<div class="clear"></div>'; echo '<label>Username:</label>'; echo '<input type="text" class="field" name="username" />'; echo '<div class="clear"></div>'; echo '<label>Password:</label>'; echo '<input type="password" class="field" name="password" />'; echo '<div class="clear"></div>'; echo '<label>Again:</label>'; echo '<input type="password" class="field" name="rePassword" />'; echo '<div class="clear"></div>'; echo '<input type="submit" class="button" name="submitReg" value="Register" />'; echo '<div class="clear"></div>'; echo '</form>'; } ?> Quote Link to comment Share on other sites More sharing options...
MDCode Posted November 6, 2012 Share Posted November 6, 2012 Anytime it says unexpected $end it means you're missing one or more closing brackets Quote Link to comment Share on other sites More sharing options...
Psycho Posted November 6, 2012 Share Posted November 6, 2012 (edited) @White_Lilly: The new forum software doesn't do a good job of keeping the formatting for code when posting. So, I don't know if what you posted is representative if the formatting you actually had. But, as snowdog posted you would do yourself a great favor by adding formatting (especially indenting) to provide a visual representation of the logical flow of your code. However, I would also suggest that if you have a very deep nest of logic that you should probably take a second look to see if you can reduce how deep that nesting goes. I do see an error in the code snowdog posted where another ending brace was removed that shouldn't have. That might not have happened with simpler structure. But there are other problems as well: You are running all of the input through htmlspecialchars() and mysql_real_escape_string() BEFORE you validate the data. You should validate the data first THEN transform the data as needed. Using the method you have, valid data could fail the validation. Plus, if I am reading the coed right yu are running the POST values through that same logic twice - once in the foreach loop and again right after that. Also, I would advise not using htmlspecialchars() on the data you save to the database. You should make that change as you echo the content to the page. If you were to ever use the data for something other than directly outputting onto an HTML page the data would be messed up. if($regPassword != $regREpassword || $regREpassword != $regPassword) Huh? Regarding the nested logic, here is an example of something that can be simplified. Instead of this if(preg_match("^[a-zA-Z0-9_.-]+@[a-zA-Z0-9-]+.[a-zA-Z0-9-.]+$", $_POST["email"]) === 0) { $msg3 .= "Please enter a valid email."; } else { if($regPassword != $regREpassword || $regREpassword != $regPassword) { $msg3 .= "The password fields didn't match."; } else { You could have if(preg_match("^[a-zA-Z0-9_.-]+@[a-zA-Z0-9-]+.[a-zA-Z0-9-.]+$", $_POST["email"]) === 0) { $msg3 .= "Please enter a valid email."; } elseif($regPassword != $regREpassword || $regREpassword != $regPassword) { $msg3 .= "The password fields didn't match."; } else { But, I don't like logic where the script stops on the first error. It should check for ALL errors then provide the user a list of what failed. Edited November 6, 2012 by Psycho Quote Link to comment Share on other sites More sharing options...
White_Lily Posted November 7, 2012 Author Share Posted November 7, 2012 Sorry for the code being non-indented(?) For some reason this forums text-editor removes indenting when you click submit :/ Thanks Psycho I will look at that list of things to do and see what happens Quote Link to comment Share on other sites More sharing options...
Psycho Posted November 7, 2012 Share Posted November 7, 2012 (edited) Here's a rewrite in what I think is a more logical workflow that isn't as complicated: <?php //Set error message for form $error_msg = ''; //Set variable for populating form field values //Used to make fields sticky in case of errors $nameHTML = ''; $usernameHTML = ''; $emailHTML = ''; if($_POST["submitReg"]) { //Preprocess the inputs $regName = trim($_POST["name"]); $regUsername = trim($_POST["username"]); $regPassword = trim($_POST["password"]); $regREpassword = trim($_POST["rePassword"]); $regEmail = trim($_POST["email"]); //Run field validations $errors = array(); //Array to hold errors //Validate name if($regName == '') { $errors[] = "Name is required."; } else { $regUsernameSQL = mysql_real_escape_string($regUsername); $check = select("COUNT(*)", "members", "username = '$regUsernameSQL'"); if(mysql_result($check, 0) !== 0) { $errors[] = "That username has been taken, pick another."; } } //Validate email if($regEmail == '') { $errors[] = "Email is required."; } elseif(!preg_match("^[a-zA-Z0-9_.-]+@[a-zA-Z0-9-]+.[a-zA-Z0-9-.]+$", $regEmail)) { $errors[] = "Email is not valid."; } //Validate passwords if($regPassword == '' || $regREpassword == '') { $errors[] = "Password and confirmation are required."; } elseif($regPassword != $regREpassword) { $errors[] = "The password fields didn't match."; } //If validations passed, attempt to create records if(!count($errors)) { //Validations passed $regNameSQL = mysql_real_escape_string($regName); $emailSQL = mysql_real_escape_string($email); $regPasswordSQL = sha1($regPassword); $id = uniqid(); $register = insert("members", "name, email, username, password, user_level, id, ban", "'$regNameSQL', '$emailSQL', '$regUsernameSQL', '$regPasswordSQL', 1, '$id', 0"); if(!$register) { $errors[] = "Sorry, we could not register your account details, if this persists contact the webmaster."; } } //If member was created, attempt to send email if(!count($errors)) { $to = $email; $subject = "Fusion Forums - Account Confirmation"; $message = "Hello! You have recently registered to Fusion Forum's.<br><br>"; $message.= "This is a confirmation email, below you will find your account details along with a Unique ID.<br><br>"; $message.= "In order to activate your account you must first enter the ID into the text field that follows the link at the end of this email. details are as follows:<br><br>"; $message.= "<table>\n"; $message.= "<tr><td><strong>Name:</strong></td><td>{$regName}</td></tr>\n"; $message.= "<tr><td><strong>Email:</strong></td><td>{$email}</td></tr>\n"; $message.= "<tr><td><strong>Username:</strong></td><td>{$regUsername}</td><tr>\n"; $message.= "<tr><td><strong>Unique ID:</strong></td><td>{$id}</td><tr>\n"; $message.= "</table><br><br>"; $message.= "Please follow this link in order to activate your account (opens in your default browser):<br>"; $message.= "<a href='http://www.janedealsart.co.uk/activate.php?id=".$id."'>Activate Account</a>"; $from = "noreply@janedealsart.co.uk"; $headers = 'MIME-Version: 1.0' . "\r\n"; $headers .= 'Content-type: text/html; charset=iso-8859-1' . "\r\n"; $headers.= "From: {$from}"; if(!mail($to, $subject, $message, $headers)) { $errors[] = "There was a problem sending your confirmation email."; } } //If user registration passed, add forum message if(!count($errors)) { $newsTitle = "New member registered!"; $cont = "{$regUsername} has just joined Fusion Forums!<br>"; $cont.= "Check out his/her profile:<br><br>"; $cont.= "View Profile"; $newsCont = $cont; $newMem = insert("news", "news_title, news_content, username", "'$newsTitle', '$newsCont', '$regUsername'"); //If this fails the user registration still completed. No need to provide error to user. } //If there were errors, create error message for form if(count($errors)) { $error_msg = "<div class='error'>"; $error_msg .= "The following error(s) occured:\n"; $error_msg .= "<ul>\n"; foreach($errors as $error) { $error_msg .= "<li>$error</li>\n"; } $error_msg .= "</ul>\n"; $error_msg .= "</div>"; //Set the form field values to be repopulated $nameHTML = htmlentities($name); $usernameHTML = htmlentities($username); $emailHTML = htmlentities($email); } else { //Include page with register success message include('register_success.php'); //$done = "You have successfully registered to Fusion Fourm's.<br>"; //$done .= "We have sent you an email with a confirmation code on it,"; //$done .= " when you go to confirm your account you will need this code in order to be able to access the forum's."; exit(); } } ?> <html> <head><title>Registration Form</title></head> <body> <?php echo $error_msg; ?> <form action="" method="POST"> <label>Full Name:</label> <input type="text" class="field" name="name" value="<?php echo $nameHTML; ?>" /> <div class="clear"></div> <label>Email:</label> <input type="text" class="field" name="email" value="<?php echo $emailHTML; ?>" /> <div class="clear"></div> <label>Username:</label> <input type="text" class="field" name="username" value="<?php echo $usernameHTML; ?>" /> <div class="clear"></div> <label>Password:</label> <input type="password" class="field" name="password" /> <div class="clear"></div> <label>Again:</label> <input type="password" class="field" name="rePassword" /> <div class="clear"></div> <input type="submit" class="button" name="submitReg" value="Register" /> <div class="clear"></div> </form> </BODY> </HTML> Edited November 7, 2012 by Psycho Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.