FUDforum: PHP discussions » Script problem

Home » General » PHP discussions » Script problem (Scritpt doesn't update database in email validation)

Show: Today's Messages :: Unread Messages :: Show Polls :: Message Navigator
| Subscribe to topic | Bookmark topic

Script problem [message #165233]

Fri, 13 May 2011 08:40

anemic-cinema

Messages: 4
Registered: May 2011
Location: Paris

Karma: 0

Junior Member

add to buddy list
ignore all messages by this user

Hi everybody, I'm a newbie to this forum. My name is Andrea and I'm Italian.
So, to get to the point i want to learn php and I'm trying to write an email validation script but I don't understand where I make a mistake.
I've already created a page (inscription.php) where someone can register himself and then an email is sent to his email box.
Register page creates and store in mysql database an activation key that it is also sent to the user
The Url included in the message calls another php page (verify.php) that compares the activation key with that stored in mysql and actives user account.
It seems to run but when i check the database, it isn't updated.
Could anyone suggest me the way to solve it?
Thank you very much and sorry if my English isn't good.

<?php
mysql_connect("localhost", "emailval", "emailval") or die(mysql_error());

mysql_select_db("emailval") or die(mysql_error());

$queryString = $_SERVER['QUERY_STRING'];

if(isset ($queryString)) {

    	$query = "SELECT * FROM membre"; 

	$result = mysql_query($query) or die(mysql_error());
	
	while($row = mysql_fetch_array($result)) {

	if ($queryString == $row["activationkey"]){

	echo "Congratulations!" . $row["login"] . 
	" is now the proud new owner of an TEST.com account.";

       	$sql="UPDATE membre SET activationkey = '', 
	status='activated' WHERE (id = $row[id])";

	} else {

         echo "error";

  	}

}

}
    
?>

Report message to a moderator

Rate author: +1 -1

Re: Script problem [message #165238 is a reply to message #165233]

Sat, 14 May 2011 08:43

naudefj

Messages: 3688
Registered: December 2004

Karma: 19

Senior Member
Administrator
Core Developer

add to buddy list
ignore all messages by this user

You never executed the UPDATE statement.

Add something like this after the $sql = ... line:

mysql_query($sql);

Report message to a moderator

Rate author: +1 -1

Re: Script problem [message #165239 is a reply to message #165238]

Sat, 14 May 2011 09:07

anemic-cinema

Messages: 4
Registered: May 2011
Location: Paris

Karma: 0

Junior Member

add to buddy list
ignore all messages by this user

Hi thank you, I found a solution this morning. Smile

Could you give me your opinion about the script? How can i improve it?

<?php
$base = mysql_connect ('localhost', 'emailval', 'emailval');

mysql_select_db ('emailval', $base);

$queryString = ($_GET['code']);
	 
$result = mysql_query("SELECT * FROM membre") or die(mysql_error());

$row = mysql_fetch_array($result);

	if  ($row['activationkey'] == $queryString) {

	echo "Congratulations!" . $row["login"] . "";


 	$sql="UPDATE membre SET  status='activated' WHERE (id = $row[id])"; 
	$req = mysql_query($sql) or die('Erreur SQL !'.$sql.'<br />'.mysql_error());

	

	
  	}  else {

    	die('Error: ' . mysql_error());

	}
?>

Thank you very much!

Report message to a moderator

Rate author: +1 -1

Re: Script problem [message #165240 is a reply to message #165239]

Sun, 15 May 2011 12:50

naudefj

Messages: 3688
Registered: December 2004

Karma: 19

Senior Member
Administrator
Core Developer

add to buddy list
ignore all messages by this user

Not bad, but you need to pay attention to the following:

1. Proper code indentation - your code is difficult to read.
2. Use consistent white space around concatenations - this look like a cut and paste job.
2. Use singe quotes where possible and appropriate - you switch between singe and double quotes without apparent reason.
4. Why append an empty string in: echo "Congratulations!" . $row["login"] . "";?
5. Don't store 'activated' in the DB (too long). Shorten it to STATUS='A' or even better, an a bit value of an integer.
6. You may want to close the DB connection when you are done.

Best of luck!

Report message to a moderator

Rate author: +1 -1

Re: Script problem [message #165241 is a reply to message #165240]

Sun, 15 May 2011 16:23

anemic-cinema

Messages: 4
Registered: May 2011
Location: Paris

Karma: 0

Junior Member

add to buddy list
ignore all messages by this user

Hi, thank you! I found your advices very helpful and I tried to follow them.
Now the script is something like that:

<?php

// Connect to database
$base = mysql_connect ('localhost', 'emailval', 'emailval');
mysql_select_db ('emailval', $base);

// Get Activation Key in the email URL
$queryString = ($_GET['code']);
$result = mysql_query('SELECT * FROM membre') or die(mysql_error());
$row = mysql_fetch_array($result);

        // Compare sent activationKey with stored activationKey
        if  ($row['activationkey'] == $queryString) {
                
                // Update databse
                $sql = "UPDATE membre SET  status='1' WHERE (id = $row[id])";
                $req = mysql_query($sql) or die('Erreur SQL !'.$sql.'<br />'.mysql_error());
                
                // Congratulations on registration
                echo "Congratulations!." . $row['login'] ." You are a new membre of TEST.COM ";
                
                // Close connection
                mysql_close ($base);
                include("index.php");
        } else {

        // Get an error
        die('Error: ' . mysql_error());
        }

?>

What do you think about it? Is it right?

Thank you very much!

Report message to a moderator

Rate author: +1 -1

Re: Script problem [message #165249 is a reply to message #165241]

Mon, 16 May 2011 23:01

Ernesto

Messages: 413
Registered: August 2005

Karma: 0

Senior Member

add to buddy list
ignore all messages by this user

Now you are looping through you entire database until you find a match, it would make much mroe sense to not select anything at all.

First, you grab the $_GET, then you make sure it is clean.

$queryString = mysql_real_escape_string($_GET['code']);

(Read about SQL injections)

After that, you just
$sql = 'UPDATE membre SET status=1 WHERE activationkey = '.$queryString;

With your way you take ALL members from the database, loop though them in PHP until you find the match. That is not a good way, PHP is not made for looping that amount of data, however, that is JUST what an SQL server is made for, it can do it in a 100th of the time with a 100th of the effort. Imagine if you have a million users, it would take PHP days to loop it and it would take huge power to bring all the data from the database to the PHP layer.

Ginnunga Gaming

Report message to a moderator

Rate author: +1 -1