Jump to content

Unsubscribe script not deleting database entry for user


UAFWolfcry

Recommended Posts

I'm creating a newsletter and the unsubscribe isn't deleting the database entry like I'm asking it to. Everything else works fine, it even successfully says the user has been removed, but it doesn't actually delete the database entry.

I've spent  two days trying to figure out why. Here's the code:

Newsletter sign up:
 

<?php
//DB Connect Info
$servername = "";
$database = "";
$username = "";
$password = "";

// Create connection
$conn = mysqli_connect($servername, $username, $password, $database);

// Check connection
if ($conn->connect_error) {
die("Connection failed: " . $conn->connect_error);
}

/*$createTable = $conn->prepare ("CREATE TABLE IF NOT EXISTS email_user (
  id int(11) NOT NULL AUTO_INCREMENT,
  email varchar(200) NOT NULL,
  hash varchar(250) NOT NULL,
  PRIMARY KEY (id)
)");

$createTable->execute();
*/

function input_security($data)
{
    $data = trim($data);
    $data = stripslashes($data);
    $data = htmlspecialchars($data);
    return $data;
}

$email = input_security($_POST['email']);

$insertData = input_security($insertData);

if(isset($_POST['submit']))
{  
  extract($_POST);
  if($email!="") :
    $email=mysqli_real_escape_string($conn,$email);
    $emailval = '/^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,4})$/';    
    if(preg_match($emailval, $email)) :
      $db_check=$conn->query("SELECT * FROM email_user WHERE email='$email'");
      $count=mysqli_num_rows($db_check);
      if($count< 1) :         
         $hash=md5($email.time());
         $link = '/unsubscribe.php?key='.$hash;

        // Change your domain        
         $fetch=$conn->query("INSERT INTO email_user(email,hash) VALUES('$email','$hash')");
         $to="$email"; //change to ur mail address
         $strSubject="Maintenance Fee Relief, LLC | Email Subscription";
         $message = '<p>Thank you for subscribing with us.</p>' ;              
         $message .= '<p>Click here to unsubscribe your email : <a href="'.$link.'">unsubscribe</p>' ;              
         $headers = 'MIME-Version: 1.0'."\r\n";
         $headers .= 'Content-type: text/html; charset=iso-8859-1'."\r\n";
         $headers .= "From: info@";            
         $mail_sent=mail($to, $strSubject, $message, $headers);  
         $msg_sucess="Your request has been accepted!.";
      else :
        $msg="This $email email address is already subscribe with us.";
      endif;  
    else :
      $msg="Please enter your valid email id";
    endif;      
  else :
    $msg="Please fill all mandatory fields";
  endif;
}
?>

<div class="newsletter-sign-up-header-form">
<div id="logerror"><?php echo @$msg; ?><?php echo @$msg_sucess; ?></div>
<form  method="post">
<span><input type="email" name="email" placeholder="Email Address - Join Newsletter" class="newsletter-sign-up-header-email" required></span>
<span><button name="submit" value="submit" title="Submit" class="newsletter-sign-up-header-submit-button">Submit</button></span>
</form>
</div>
<?php
//DB Connect Info
$servername = "";
$database = "";
$username = "";
$password = "";

// Create connection
$conn = mysqli_connect($servername, $username, $password, $database);

// Check connection
if ($conn->connect_error) {
die("Connection failed: " . $conn->connect_error);
}
?>

<?php
if(@$_GET['key']!=""):
    $hash=mysqli_real_escape_string($conn,$_GET['key']);
    $fetch=$conn->query("SELECT * FROM email_user WHERE hash = '$hash'");
    $count=mysqli_num_rows($fetch);
    if($count==1) :
      $row=mysqli_fetch_array($fetch);      
        $conn->query("DELETE email_user WHERE id='$user_id'");
        $msg="Your email id unsubscribe with us";
    else :
      $msg="Please click valid link.";
    endif;
else :
    header("Location:404.php");
endif;
?>

<!doctype html>
<html lang="en">
<head>
   <title>Unsubscribe</title>
</head>
<body>
    <div align="center">
    <h2><?php echo $msg; ?></h2>
        <a href="https://www.--.com">--.com</a>                                                
    </div>
    

 

Quote

 

 

Link to comment
Share on other sites

As @gw1500se stated, the DELETE query is malformed as it does not include the "FROM" parameter, but it still won't work with that

$conn->query("DELETE email_user WHERE id='$user_id'");

$user_id is never defined!!! Aside from that, there are quite a few problems and poor construction. Here are some tips:

1. Put your DB connection logic in a separate file and include() it on the pages you need it. If you need identical code on multiple scripts, don't use copy/past coding. If you ever need to make a change in your DB connection you would only have to change it one place and not many.

2. Never user "SELECT *". There are performance and security problems with doing that. Always list the fields you want.

3. Don't suppress errors using the '@' symbol. You could have simply used an isset() check for the variable in your script.

if(isset($_GET['key']) && $_GET['key']!=""):

4. I would highly suggest not wrapping your error conditions in nested if/else conditions in this manner as it makes it difficult to easily "see" which results go with which error

if(!error_condition_1) {
    if(!error_condition_2) {
        if(!error_condition_3) {
            //Success result
        } else {
            //error result 3
        }
        //error result 2
    }
    //error result 1
}

This is MUCH more readable and maintainable IMO

if(error_condition_1) {
    //error result 1
} elseif(error_condition_2) {
    //error result 2
} elseif(error_condition_3) {
    //error result 3
} else {
    //Success result
}

5. You are switching back and forth between object oriented and procedural. Stick with one for consistency

    $fetch = $conn->query("SELECT * FROM email_user WHERE hash = '$hash'");
    //$count = mysqli_num_rows($fetch);
    $count = $fetch->num_rows;

6. Use prepared statements for your queries.

7. No need to use a SELECT statement before running the DELETE query. A race condition can create problematic results. You can just run the DELETE query and then check the number of affected rows. If 0, then there was no matching record, else if 1 then you can tell the user they were unsubscribed.

Link to comment
Share on other sites

I did realize the user_id variable was missing completely. I couldn't figure why they would omit that from their crappy tutorial and expect it to work.

What should the user_id variable look like so that I can delete the correct persons entry?

I just want to get it to work for the moment and then I can play around with the code, learn from it and make it better. I did realize they aren't using prepared statements and I was going to fix that after I got this stupid thing working.

Thanks.

Edited by UAFWolfcry
Link to comment
Share on other sites

New at this, but here's the screenshot of the tables.

I have them fill out the form, it associates their entry with a hash, then sends them an email with an unsubscribe link they can click on. They click on unsubscribe and it's supposed to remove their entries under email_user.

Screenshot - 12_14_2019 , 12_32_54 AM.png

Link to comment
Share on other sites

reproducing something you see on the web isn't learning. it's you functioning as a human photo-copier machine, where the quality of the output cannot be any better then the quality of the input, which in this case is filled with mistakes, bad and out of date practices.

ignoring for the moment that data is almost never actually deleted in real-life (its updated to indicate it is no longer being used), for the current operation of deleting a specific record in a database table, what inputs do you have or need, what processing are you going to do based on those inputs, and what result or output are you going to produce? defining the Inputs, Processing, and Output (IPO) is the most useful thing you can do before writing any code.

once you have defined these things, you can go about designing, writing, testing, and debugging the code needed to accomplish the steps it takes to perform this operation.

Link to comment
Share on other sites

So the action is from an email? PHP is probably not the best solution for processing an incoming email, IMO. In any case the hash is probably what should be used for the delete. Something like:

DELETE FROM email_user WHERE hash='$hash'

 

Edited by gw1500se
Link to comment
Share on other sites

Thank you! That worked. Thank you so much.

Now that it works.. I am going to tear it into pieces and learn from it, make it better and continue to teach myself better PHP/SQL.

I realize using someone elses code isn't ideal, I'm a web developer and I've been writing my own HTML/CSS code for years. I started off by studying other peoples code, figuring out how it worked and why and then went on writing my own code. That's how my brain works.

Link to comment
Share on other sites

2 minutes ago, UAFWolfcry said:

What language would you recommend for processing email?

that information was a miss-statement. this code is not processing an email. it is processing the result of clicking on a link that was sent in an email.

another issue with this code is it ONLY works because you are reading the email on the same computer where the web server is running. the URL being sent in the email is a relative URL.

to do this properly, the link would need to be an absolute, fully qualified URL, so that when it is clicked/copied to a browser's address bar, it will work regardless of what computer the email is opened on.

Link to comment
Share on other sites

your should x/* out information in posts, rather than to delete it, as that changes the meaning of what you post.

site, personal, sensitive, configuration information used in code should be defined in a separate .php configuration file and required into your main code. this will let you post any of your main code as is without needing to alter it.

Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.