Page 1 of 1

Basic Fetch Database Records PHP Script

Posted: Wed Jan 17, 2007 5:03 pm
by Christopher
I wrote this really quickly for another thread and Daedalus- noted that it might be good for Snippets. I figured that a round here in Critique would probably make it more useful and see if it was worthy of Snippets.

The script was my attempt to write what a minimum a PHP database script ought to look like. Some goals were: no die(), clear logic, thorough error checking, separation of connection and response (reporting an error is also a response), and basic security. I should also show good programming habits and practices.

I have some questions:
- should it be in three parts: connection, query/fetch, and display data? (so we could show fetching one or may rows for example)
- should it be database independent in some way or does that complicate it too much?
- should things be put in functions so they are more reusable (e.g. connect() and query(), fetch())

Code: Select all

<?php
// initialize variables
$errmsg = '';
$row = array();

// Connect to server
$con = mysql_connect("localhost","don't_post_usernames","don't_post_passwords");

if (mysql_errno()) {
     $errmsg = 'Could not connect:' . mysql_error();
} else {

     // Select database.
     mysql_select_db("database_name");

     if (mysql_errno()) {
          $errmsg = 'Could not select:' . mysql_error();
     } else {
          // Setup and do query

          // filter untrusted post variable
          $username = preg_replace('/[^a-zA-Z0-9]/', '', $_POST['username']);

          // escape untrusted post variable
          $username = mysql_real_escape_string($username);     // changed - thanks Mordred

          $sql1 = "SELECT * FROM user WHERE username='$username'";
          $result1 = mysql_query($sql1);

          if (mysql_errno()) {
               $errmsg = 'Query failed:' . mysql_error();
          } else {
               $row = mysql_fetch_assoc($result);
          }
     }
}

// now the response
if ($errmsg) {
     // error goes here
     echo "Error: $errmsg<br/>";
} else {
     // success goes here
     dump($row);
}

// handy for debugging
function dump($value) {
     echo '<pre>' . print_r($value, 1) . '</pre>';
}

Posted: Wed Jan 17, 2007 5:18 pm
by daedalus__
New users normally only deal with MySQL, as far as I know, so I do not think it needs db abstraction. I do think that it should be chopped up into functions, though. Perhaps connect(), printError(), selectDb(), query(), disconnect() with a simple filter function or two to encourage security.



+1 for snippets!

EDIT:

I did bits and pieces of this through-out the course of the day. It might not even work rofl, I'll clean it up a bit when I get home.

Code: Select all

$link = '';
$res = '';
$err_msg = '';

function connect($host = 'localhost', $user = 'root', $pass = '')
{
	$link = mysql_connect($host, $user, $pass);
	if (! $link)
	{
		$err_msg = mysql_error();
		return false;
	} else return $link;
}

function disconnect()
{
	if (is_resource($link))
	{
		mysql_close($link);
	}
	else
	{
		mysql_close();
	}
}

function select_db($name)
{
	$bool = mysql_select_db($name);
	if (! $bool)
	{
		$err_msg = mysql_error();
		return false;
	} else return true;
}

function query($sql)
{
	$res = mysql_query($sql);
	if ($res != false)
	{
		if (is_resource($res))
		{
			if (rows($res) > 0)
			{
				return $res;
			}
			else
			{
				return false;
			}
		}
		else
		{
			return true;
		}
	}
	else
	{
		$err_msg = mysql_error();
		return false;
	}
}

function rows($res)
{
	$num = mysql_num_rows($res);
	if ($num != false)
	{
		return $num;
	}
	else
	{
		return mysql_affected_rows($res);
	}
}

function report_error($value)
{
	echo $err_msg.'<br />';
	echo '<pre>' . var_dump($value, 1) . '</pre>';
}

function filter_alphanumeric($string)
{
	return preg_replace('/[^a-zA-Z0-9]/', '', $string);
}


// connect
$link = connect('localhost', 'user', 'pass');
select_db('test');
$res = query('SELECT * FROM testycles');
disconnect();
if ($res != false)
{
	// do your thang, baby
}
else
{
	report_error();
}

Posted: Wed Jan 17, 2007 5:28 pm
by Nathaniel
In Code Complete, Steve Mcconnell says, if I remember correctly, that programming logic should have the wanted action first, and unexpected actions in the elses.

So that would make...

Code: Select all

if (mysql_errno()) {
     $errmsg = 'Could not connect:' . mysql_error();
} else { 
into

Code: Select all

if ( !mysql_errno() ) {
    // connection succeeded
} else { 
    $errmsg = 'Could not connect:' . mysql_error();
}
Perhaps a negligible critique, but as far as understanding the code goes, the latter method makes for an easy read of how the logic works in a successful case.

Posted: Thu Jan 18, 2007 5:37 am
by Mordred
The 'filter' step is meaningless, because the 'escape' step uses $_POST again.

You neglect the case where the SQL query succeeds, but returns zero rows. I would also add backticks to the query. Another good practice is to SELECT only the required fields, and to LIMIT 1, since you only use the first row from the result.

I don't think that outputting mysql_error() to the user is a good practice. It belongs to the error log, not to the output.

mysql_free_result() might be nice, although it's generally not needed, this is supposed to presend good practices ;)

Dividing the logical steps in separate funcitons is a good idea. Just don't turn them into a pseudo-db-abstraction-layer.

Also, it would be better if the sample is not about usernames, as not to tempt newbies into using it for their login scripts (which usually require one or two additional checks and features).

var_dump() may give more useful info than print_r(), so I'd use it instead.

Posted: Thu Jan 18, 2007 1:14 pm
by daedalus__
Mordred wrote:I don't think that outputting mysql_error() to the user is a good practice. It belongs to the error log, not to the output.
I feel that most newbies are going to be outputting this information, with their mind far, far away from an error log, making it acceptable to use this method in the example.
Mordred wrote:mysql_free_result() might be nice, although it's generally not needed, this is supposed to presend good practices ;)
I'm still thinking about the target group for this script. I do not think that anyone who uses this is going to have a site that is big enough to need to free the results although I do agree it presents an example of good practice. +1 for a free() function.
Mordred wrote:Also, it would be better if the sample is not about usernames, as not to tempt newbies into using it for their login scripts (which usually require one or two additional checks and features).
I disagree, it is likely that this code will be better than whatever the newbie has written, at least it will by the time it is finished.
Mordred wrote:var_dump() may give more useful info than print_r(), so I'd use it instead.
+1
Nathaniel wrote:

Code: Select all

PHP (Brief):
if ( !mysql_errno() ) {
    // connection succeeded
} else {
    $errmsg = 'Could not connect:' . mysql_error();
}
+1


arborint, if you would be okay with it, I would like to help with this. Would you like to collaborate through pm?

Posted: Thu Jan 18, 2007 2:26 pm
by Christopher
Daedalus- wrote:arborint, if you would be okay with it, I would like to help with this. Would you like to collaborate through pm?
I agree with your comments and those above. I am pretty busy right now ... why don't you have a go at taking the code to the next step as discussed. My only input would be that if it gets longer that maybe twice what I posted then it is too complicated.

Posted: Thu Jan 18, 2007 3:29 pm
by daedalus__
I think it needs to be kept as simple as possible, as well.