0
\$\begingroup\$

I'm still new in web development and I am trying to develop a website with free access to doctors listing with option of booking. This is a personal project that my local community can benefit.

I want to get an opinion or recommendation from you experts whether the logic for my register functionality is correct.

Before I ask any question on this platform, please note that I have done extensive research and trials on web development.

Let me just explain the logic I implemented point wise.

  1. Call DB Connection file
  2. Check for DB Connection errors
  3. Assign form input to variables
  4. Use prepared statements to retrieve current user accounts info
  5. Check if account exists
  6. Insert user details in database using prepared statement
  7. Direct to confirmation page + Send email to activate account
  8. Close connection
<?php
require_once("dblogin.php");

// Connection error to database
if ($conn->connect_error) {
trigger_error('No database connection', E_USER_WARNING);;
}

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

  $firstname = mysqli_real_escape_string($conn, $_POST['firstname']);
  $lastname = mysqli_real_escape_string($conn, $_POST['lastname']);
  $email = mysqli_real_escape_string($conn, $_POST['email']);
  $password1 = password_hash(mysqli_real_escape_string($conn, 
$_POST['password1']),PASSWORD_BCRYPT);
  $terms_flag = 'Approved';
  $user_type = 'user';
  $status = 'Active';
  $account_verified = '0';

  $str = 
  'qwertyuiopasdfghjklzxcvbnmQWERTYUIOPASDFGHJKLZXCVBNM!@#$%&*';
  $otp = '0123456789';
  $str = str_shuffle($str);
  $otp = str_shuffle($otp);
  $str = substr($str,0,10);
  $otp = substr($otp,0,6);  

  $user_check_query = $conn -> prepare("SELECT count(email) as 
  count_num FROM user_account WHERE username=? LIMIT 1");
  $user_check_query -> bind_param("ss", $email);
  $user_check_query->execute();
  $user_check_query->store_result();  

  if($user_check_query->execute() == true) {  
    $user_check_query -> bind_result($count_num);
    $user_check_query -> fetch(); 
    $user_check_query->close();     
  if ($count_num) { // if user exists

  $message = "Username already exists. Please use another Email 
  Address or Phone Number";

 }elseif (strlen($email) > 0) {  

    $stmt = $conn->prepare("INSERT INTO user_account 
  (first_name,last_name,email,password1,account_verified,token) 
   VALUES (?,?,?,?,?,?,?)");
    $stmt->bind_param("sssssssssssssss",      

    $firstname,$lastname,$email,
    $email,$password1,$account_verified,$str);          
    $stmt->execute();
    $stmt->close();         

        $to = 'test@gmail.com';
        $subject = "Activate your account - Find A Doctor";
        //* ========== Start Activate Account Email Layour ======== 
        *//
        $message .= '<html><body>';         
        $message .= 'Please Activate your account';
        $message .= '<p align="center" style="font-size:25px; 
        color:#3f4079;">Username: <strong>'.$email.'</strong></p> 
        <br>';
        $message .= '<p align="center" style="font-size:25px; 
       color:#3f4079;">Kindly<a href="http://www.test.com/login.php? 
         confirmemail='.$email.'&token='.$str.'"><strong> 
        confirm</strong></a> your registration.</p>';
        $message .= "</body></html>";           
        //* ========== Start Activate Account Email Layout ======== 
       *//

        // Always set content-type when sending HTML email
        $headers = "MIME-Version: 1.0" . "\r\n";
        $headers .= "Content-type:text/html;charset=UTF-8" . "\r\n";

        // More headers
        $headers .= 'From: <test@gmail.com>' . "\r\n";

        mail($to,$subject,$message,$headers);           

        $_SESSION['username'] = $username;

        header('location: confirm-register.php? 
        confirmemail='.$email.'&p='.$phonenumber);  
        exit(); 
          }
        }
       }
        $conn->close();
       ?>
\$\endgroup\$

closed as off-topic by Your Common Sense, rolfl Jun 12 at 18:24

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Your Common Sense, rolfl
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ There seems to be some disputes here. In order to give me time to understand the situation, I am locking this post for one hour. Please hold on. \$\endgroup\$ – Simon Forsberg Jun 12 at 14:46
  • 3
    \$\begingroup\$ Alright, here's what we do, the answer will be invalidated if we edit the question, so let's close this question off as broken, not working (it is broken, and not working), and, @PhilStamp - open up a new question with the working code (you can edit this question to get the raw content - and pasted it in to the new question, and then use the correct code for the new question). This will clear up any confustion and start the new question with a clear slate. \$\endgroup\$ – rolfl Jun 12 at 18:24
1
\$\begingroup\$

After a cursory read, this jumped out at me:

$user_check_query -> bind_param("ss", $email);

You’re binding 1 string parameter, but have two s codes.

You do it again here:

$stmt->bind_param("sssssssssssssss",      

$firstname,$lastname,$email,
$email,$password1,$account_verified,$str);          

15 s’s but only 7 parameters.


It appears you are storing the password as clear text in the database. Never do that. Don’t even store an encrypted password; use a one-way hash.


Your confirmation email is not being sent to $email. It is sent unconditionally to test@gmail.com. Your newly registered user will be waiting a long time for their verification code.

\$\endgroup\$
  • \$\begingroup\$ My apologies. You are perfectly right. While editing the code, I missed updating the bind parameter. \$\endgroup\$ – Phil Stamp Jun 12 at 13:45
  • 4
    \$\begingroup\$ Remove the checkmark, please. The question is 17 minutes old, and I’ve looked at only 2 lines of code. There is plenty more to review. \$\endgroup\$ – AJNeufeld Jun 12 at 13:46
  • \$\begingroup\$ @PhilStamp this is not a code review. If this is all you were looking for it means your question is off topic. \$\endgroup\$ – Your Common Sense Jun 12 at 13:46
  • \$\begingroup\$ @YourCommonSense What does a code review mean? I explained clearly the logic for development and I provided my script for reference. I am just looking for recommendation or things I missed so that the functionality works fine. How is it off topic as StackExchange is for code review. \$\endgroup\$ – Phil Stamp Jun 12 at 14:03
  • \$\begingroup\$ @PhilStamp if your present code doesn't work, it means it is off topic for this site. Only a working code is accepted for the review \$\endgroup\$ – Your Common Sense Jun 12 at 14:05

Not the answer you're looking for? Browse other questions tagged or ask your own question.