Trying a New Login script Does it seem optimized?

[cooldude832]

I'm trying this out for login and I think its okay any ideas

<?php
function login(){
connectSQL(); //Function that connects and opens DB if its not already there
$username = mysql_escape_string(trim($_POST['username']));
$password = md5(trim($_POST['password']));
// Create query
$fields = array("UserID, Other Fields");
$q = "SELECT '$fields' FROM `users` WHERE `username`='$username' && `password`='$password' LIMIT 1";
// Run query
$r = mysql_query($q) or die(mysql_error());
if(mysql_num_rows($r)){
	$row = mysql_fetch_assoc($r);
	foreach($fields as $value){
		$_SESSION[$value] = $row[$value];
	}
	// Redirect to member page
	header("Location: after_login.php");
}	
else{
  // Login not successful
die("Sorry, could not log you in. Wrong login information.<br/>");
include("login.html");
}
}//End of login
?>

[pocobueno1388]

$fields = array("UserID, Other Fields");
$q = "SELECT '$fields' FROM `users` WHERE `username`='$username' && `password`='$password' LIMIT 1";

 

Are you sure by putting "SELECT '$fields'" it will actually put each individual field in your query without any looping having to be done? I'm thinking it won't...

 

Also on these lines:

die("Sorry, could not log you in. Wrong login information.<br/>");
include("login.html");

 

The include will never be executed because you have already told the script to die...that completely exits the script.

 

have you even tested this script out to see if it even works? I would suggest you do that first before asking if it looks okay...

 

 

[cooldude832]

thanks for the feedback, yeah its untested, and i think i need to do something like

$sqlfields = implode(","$fields);

 

and yeah i need to kill that die to an echo, thanks for the help

[pocobueno1388]

Why are you storing the field names in an array anyways? I doubt it will make the query any more secure...if thats what your going for.

[cooldude832]

so that i can reuse them as the session names. and the row results, also lets me adjust hte query very easily

[pocobueno1388]

How many fields could you possibly need, same thing for sessions? I think it would be faster to just register each session line by line rather than having to loop through the array for the field names And to register the session.

 

also lets me adjust hte query very easily

Is it not easy enough to simply insert/delete/modify  a field name straight from the query? I really don't see how much easier it could get...sounds like your just creating more work for yourself if you ask me.

 

I would also change this line

if(mysql_num_rows($r)){

 

To:

if(mysql_num_rows($r) > 0){

 

Obviously there is going to be some amount of rows, even if it is zero...so you would think that the way you had it would always return true...I could be wrong about that though, but it wouldn't hurt to change it just in case.

 

[cooldude832]

if will only execute if

the parameters do not return: NULL, FALSE, or a String or Integer with the value 0 so saying >0 is not needed

secondly I can modify this for any table i have users in simply by changing the array to what ever fields I name, and my logins only pull what they need for sessions so the loop makes complete sense to me.