Skip to main content
added 290 characters in body
Source Link
mickmackusa
  • 8.8k
  • 1
  • 17
  • 31

A late note... if the friends_array is a collection of user ids/usernames, then a First Normal Form (1NF) rule should be implemented. Ditch the friends_array column. Create a new table called friends with separate rows for each relationship. The functional benefits are many.


A late note... if the friends_array is a collection of user ids/usernames, then a First Normal Form (1NF) rule should be implemented. Ditch the friends_array column. Create a new table called friends with separate rows for each relationship. The functional benefits are many.

added 261 characters in body
Source Link
mickmackusa
  • 8.8k
  • 1
  • 17
  • 31
  1. Regarding setting up your database connection and error reporting, please refer to this recent post from YCS.
  2. I'm noticing that you are unconditionally mutating several of the user submitted values. If you have any concerns that someone might legitimately access this page without submitting these variables, then you should have a condition to determine if the script should be used as a signup attempt or merely bounced. Otherwise, if the only reason that a legitimate user will be accessing this page is via a signup attempt, then I would say you should remove the empty() calls and use a function-less falsey check (e.g. if (!$_POST["first_name"]) {) because if the variables are guaranteed to be set, then empty() is doing unnecessary work.
  3. Be sure that you are calling the right encoding function for the right reason and at the right time. In short, you should not be htmencoding values before they go into the db, you should only be encoding strings just before printing them to screen. Please read: htmlentities() vs. htmlspecialchars() and PHP & mySQL: When exactly to use htmlentities?
  4. I don't see any reason to declare these empty strings as variables: $pw = ''; and $pw2 = '';. I also think it is poor naming convention to declare string data as a variable including the word array such as $friend_array = ',';.
  5. I think if ($_POST['pw'] !== $_POST['pw2']) { should be moved up the script with the other validators. You don't want to do any db fetching until the user input has clear all basic hurdles.
  6. The secondary condition in if ($row && $row['email'] == $_POST['email']) { is nonsense. You have already required this expression to be true in your sql. There is no benefit in the redundant check in php. In fact, because you only need to check if the username exists in the db, you should probably just write a COUNT() query. See this suggestion: Return row count from mysqli prepared statement
  7. I would not be bloating my $_SESSION array with the markup that you are piling into $_SESSION['error']. Trim all the fat and just keep the "white meat": $_SESSION['error'] = 'E-mail exists'. You should be moving all of your styling to an external stylesheet anyhow. Drop that <b> tag and just use styling. Never use more html markup than you need to.
  8. I don't like the idea of saving a default value of a comma in the friend_array column. If they have no friends, then the value should be NULL (or an empty string if you must). That said, if you are unconditionally hardcoding a value to be INSERTed into every row, then that argues that you should be modifying the table structure and declaring the default value for friends_array as ','. This way you don't even need to declare the $friends_array variable or bloat your prepared statement / binding with the extra syntax.
  9. "You can now login." is not an error, so it is inappropriate to write that data into $_SESSION['error']. The next developer is going to be scratching their head at your decision to write conflicting data into certain SESSION elements. Create better naming convention and/or change your SESSION structure.
  10. I don't like that you are printing the $errors then redirecting the user. Please read this: Redirect a user after the headers have been sent Not only will header() prompt problems after you have printed to screen, it doesn't make a lot of sense if you aren't going to let them see the errors that are printed. I'd say it makes better sense to push all of the errors into $_SESSION['errors'], then after the redirect you should echo implode("<br>\n", $_SESSION['errors']); in whatever format will be attractive to the user.
  11. I do like that you are sanitizing the input heavily and that you are using $_POST to transfer the form data to a database writing process.
  1. Regarding setting up your database connection and error reporting, please refer to this recent post from YCS.
  2. I'm noticing that you are unconditionally mutating several of the user submitted values. If you have any concerns that someone might legitimately access this page without submitting these variables, then you should have a condition to determine if the script should be used as a signup attempt or merely bounced. Otherwise, if the only reason that a legitimate user will be accessing this page is via a signup attempt, then I would say you should remove the empty() calls and use a function-less falsey check (e.g. if (!$_POST["first_name"]) {) because if the variables are guaranteed to be set, then empty() is doing unnecessary work.
  3. Be sure that you are calling the right encoding function for the right reason and at the right time. In short, you should not be htmencoding values before they go into the db, you should only be encoding strings just before printing them to screen. Please read: htmlentities() vs. htmlspecialchars() and PHP & mySQL: When exactly to use htmlentities?
  4. I don't see any reason to declare these empty strings as variables: $pw = ''; and $pw2 = '';. I also think it is poor naming convention to declare string data as a variable including the word array such as $friend_array = ',';.
  5. I think if ($_POST['pw'] !== $_POST['pw2']) { should be moved up the script with the other validators. You don't want to do any db fetching until the user input has clear all basic hurdles.
  6. The secondary condition in if ($row && $row['email'] == $_POST['email']) { is nonsense. You have already required this expression to be true in your sql. There is no benefit in the redundant check in php.
  7. I would not be bloating my $_SESSION array with the markup that you are piling into $_SESSION['error']. Trim all the fat and just keep the "white meat": $_SESSION['error'] = 'E-mail exists'. You should be moving all of your styling to an external stylesheet anyhow. Drop that <b> tag and just use styling. Never use more html markup than you need to.
  8. I don't like the idea of saving a default value of a comma in the friend_array column. If they have no friends, then the value should be NULL (or an empty string if you must). That said, if you are unconditionally hardcoding a value to be INSERTed into every row, then that argues that you should be modifying the table structure and declaring the default value for friends_array as ','. This way you don't even need to declare the $friends_array variable or bloat your prepared statement / binding with the extra syntax.
  9. "You can now login." is not an error, so it is inappropriate to write that data into $_SESSION['error']. The next developer is going to be scratching their head at your decision to write conflicting data into certain SESSION elements. Create better naming convention and/or change your SESSION structure.
  10. I don't like that you are printing the $errors then redirecting the user. Please read this: Redirect a user after the headers have been sent Not only will header() prompt problems after you have printed to screen, it doesn't make a lot of sense if you aren't going to let them see the errors that are printed. I'd say it makes better sense to push all of the errors into $_SESSION['errors'], then after the redirect you should echo implode("<br>\n", $_SESSION['errors']); in whatever format will be attractive to the user.
  11. I do like that you are sanitizing the input heavily and that you are using $_POST to transfer the form data to a database writing process.
  1. Regarding setting up your database connection and error reporting, please refer to this recent post from YCS.
  2. I'm noticing that you are unconditionally mutating several of the user submitted values. If you have any concerns that someone might legitimately access this page without submitting these variables, then you should have a condition to determine if the script should be used as a signup attempt or merely bounced. Otherwise, if the only reason that a legitimate user will be accessing this page is via a signup attempt, then I would say you should remove the empty() calls and use a function-less falsey check (e.g. if (!$_POST["first_name"]) {) because if the variables are guaranteed to be set, then empty() is doing unnecessary work.
  3. Be sure that you are calling the right encoding function for the right reason and at the right time. In short, you should not be htmencoding values before they go into the db, you should only be encoding strings just before printing them to screen. Please read: htmlentities() vs. htmlspecialchars() and PHP & mySQL: When exactly to use htmlentities?
  4. I don't see any reason to declare these empty strings as variables: $pw = ''; and $pw2 = '';. I also think it is poor naming convention to declare string data as a variable including the word array such as $friend_array = ',';.
  5. I think if ($_POST['pw'] !== $_POST['pw2']) { should be moved up the script with the other validators. You don't want to do any db fetching until the user input has clear all basic hurdles.
  6. The secondary condition in if ($row && $row['email'] == $_POST['email']) { is nonsense. You have already required this expression to be true in your sql. There is no benefit in the redundant check in php. In fact, because you only need to check if the username exists in the db, you should probably just write a COUNT() query. See this suggestion: Return row count from mysqli prepared statement
  7. I would not be bloating my $_SESSION array with the markup that you are piling into $_SESSION['error']. Trim all the fat and just keep the "white meat": $_SESSION['error'] = 'E-mail exists'. You should be moving all of your styling to an external stylesheet anyhow. Drop that <b> tag and just use styling. Never use more html markup than you need to.
  8. I don't like the idea of saving a default value of a comma in the friend_array column. If they have no friends, then the value should be NULL (or an empty string if you must). That said, if you are unconditionally hardcoding a value to be INSERTed into every row, then that argues that you should be modifying the table structure and declaring the default value for friends_array as ','. This way you don't even need to declare the $friends_array variable or bloat your prepared statement / binding with the extra syntax.
  9. "You can now login." is not an error, so it is inappropriate to write that data into $_SESSION['error']. The next developer is going to be scratching their head at your decision to write conflicting data into certain SESSION elements. Create better naming convention and/or change your SESSION structure.
  10. I don't like that you are printing the $errors then redirecting the user. Please read this: Redirect a user after the headers have been sent Not only will header() prompt problems after you have printed to screen, it doesn't make a lot of sense if you aren't going to let them see the errors that are printed. I'd say it makes better sense to push all of the errors into $_SESSION['errors'], then after the redirect you should echo implode("<br>\n", $_SESSION['errors']); in whatever format will be attractive to the user.
  11. I do like that you are sanitizing the input heavily and that you are using $_POST to transfer the form data to a database writing process.
Source Link
mickmackusa
  • 8.8k
  • 1
  • 17
  • 31

  1. Regarding setting up your database connection and error reporting, please refer to this recent post from YCS.
  2. I'm noticing that you are unconditionally mutating several of the user submitted values. If you have any concerns that someone might legitimately access this page without submitting these variables, then you should have a condition to determine if the script should be used as a signup attempt or merely bounced. Otherwise, if the only reason that a legitimate user will be accessing this page is via a signup attempt, then I would say you should remove the empty() calls and use a function-less falsey check (e.g. if (!$_POST["first_name"]) {) because if the variables are guaranteed to be set, then empty() is doing unnecessary work.
  3. Be sure that you are calling the right encoding function for the right reason and at the right time. In short, you should not be htmencoding values before they go into the db, you should only be encoding strings just before printing them to screen. Please read: htmlentities() vs. htmlspecialchars() and PHP & mySQL: When exactly to use htmlentities?
  4. I don't see any reason to declare these empty strings as variables: $pw = ''; and $pw2 = '';. I also think it is poor naming convention to declare string data as a variable including the word array such as $friend_array = ',';.
  5. I think if ($_POST['pw'] !== $_POST['pw2']) { should be moved up the script with the other validators. You don't want to do any db fetching until the user input has clear all basic hurdles.
  6. The secondary condition in if ($row && $row['email'] == $_POST['email']) { is nonsense. You have already required this expression to be true in your sql. There is no benefit in the redundant check in php.
  7. I would not be bloating my $_SESSION array with the markup that you are piling into $_SESSION['error']. Trim all the fat and just keep the "white meat": $_SESSION['error'] = 'E-mail exists'. You should be moving all of your styling to an external stylesheet anyhow. Drop that <b> tag and just use styling. Never use more html markup than you need to.
  8. I don't like the idea of saving a default value of a comma in the friend_array column. If they have no friends, then the value should be NULL (or an empty string if you must). That said, if you are unconditionally hardcoding a value to be INSERTed into every row, then that argues that you should be modifying the table structure and declaring the default value for friends_array as ','. This way you don't even need to declare the $friends_array variable or bloat your prepared statement / binding with the extra syntax.
  9. "You can now login." is not an error, so it is inappropriate to write that data into $_SESSION['error']. The next developer is going to be scratching their head at your decision to write conflicting data into certain SESSION elements. Create better naming convention and/or change your SESSION structure.
  10. I don't like that you are printing the $errors then redirecting the user. Please read this: Redirect a user after the headers have been sent Not only will header() prompt problems after you have printed to screen, it doesn't make a lot of sense if you aren't going to let them see the errors that are printed. I'd say it makes better sense to push all of the errors into $_SESSION['errors'], then after the redirect you should echo implode("<br>\n", $_SESSION['errors']); in whatever format will be attractive to the user.
  11. I do like that you are sanitizing the input heavily and that you are using $_POST to transfer the form data to a database writing process.