PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Wed Jun 19, 2019 7:15 pm

All times are UTC - 5 hours




Post new topic Reply to topic  [ 7 posts ] 
Author Message
PostPosted: Wed Nov 23, 2011 3:41 am 
Offline
Forum Commoner

Joined: Mon Nov 21, 2011 1:16 am
Posts: 69
Quite often I need to pull just one specific value from a MySQL database so I wrote this function to do just that. I've been using it for a while and haven't had any problems with it at all. I was just wondering if there is anything wrong with it. Thanks to anyone who takes the time to examine my function and provide comments.

Syntax: [ Download ] [ Hide ]
<?php

include_once("sql_connect.php");



//Hermit TLs' my_get function v0.1

//This function pulls a single value from your MYSQL database

//

//Required Values

//      $value : Column name of requested value

//      $table : Table name of where column resides

//      $where : ie. username=someuser

//

//Optional Values

//      $error : Custom error to be displayed

//      $override : Use this MYSQL statment to query (if this value is specified all other values will be ignored)



function my_get($value, $table, $where, $error = NULL, $override = NULL){

session_start();

if (!$where){

        $where = $_SESSION['id'];

}

        if(!$override){$query = "SELECT " . $value . " FROM " . $table . " WHERE " . $where;}

                else{$query = $override;}

        $getdata = mysql_query($query) or die ('

        <table class="page">

                <tr><td class="error">Error returned from my_get function.</td></tr>

                <tr><td class="error">

                Error: '
.mysql_error() .

                '</td></tr>

                <tr><td class="error">'
.

                $query . '

                </td></tr>

                <tr><td class="error">

                '
.$error.'

                </td></tr>

        </table>

        '
);

        $results = mysql_fetch_array($getdata);

        $got = $results[$value];

                return ($got);

        unset($value, $table, $where, $error, $override);

}



?>


Top
 Profile  
 
PostPosted: Wed Nov 23, 2011 5:36 am 
Offline
Forum Regular
User avatar

Joined: Tue Sep 28, 2010 11:41 am
Posts: 984
Location: Columbus, Ohio
Calling session_start() from within a function that will be most likely called many times is not good, as it will produce notices (you may have them not displaying or logging, but IMO best for a script to never produce anything and a development environment should display ALL levels of error reporting, including Notices.

Also you will never be able to use this function once output is started without generating warnings. (But if you program like me, by the time you start outputting anything, all the info has been collected)

A better place to have this would be on something that gets called early on only once, or at the very least should do something like:
Syntax: [ Download ] [ Hide ]
if (!session_id) { session_start(); }


Make sure you do proper variable cleansing before passing them to this function (ie. using mysql_real_escape_string(). ANd if Field/Table can possibly be affected by user input, Change your query string to:
Syntax: [ Download ] [ Hide ]
$query = "SELECT `" . str_replace('`','',$value) . "` FROM `" . str_replace('`','',$table) . "` WHERE " . $where


You have not code in place for what if there were no rows returned.

The chunk to return the value could be changed to this :
Syntax: [ Download ] [ Hide ]
if (mysql_num_rows($getdata)==0) {
    return FALSE;
}
else {
    $return_val = mysql_result($getdata,$value);
    mysql_free_result($getdata);
    return FALSE;
}


The unset at the end, 1, no necessary as once the function is exited, unless they were declared static, won't exist anyhow, and 2. nothing in a function gets executed after a return statement is called.


Top
 Profile  
 
PostPosted: Wed Nov 23, 2011 7:17 am 
Offline
DevNet Master
User avatar

Joined: Sun Feb 15, 2009 12:08 pm
Posts: 2794
Location: .za
I would throw an exception instead of using die() to check for any failing on the script.

You don't say explicitly if you use it for production purposes but if you do you might want to use the @ sign to suppress potential warning messages which might result from your mysql functions.

There's also no checking of what type of data you receive so this is an area where you can improve (twinedev already touched on the issue). Another thing which might make no difference is to use FALSE instead of NULL but this is just a personal preference.

_________________
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering


Top
 Profile  
 
PostPosted: Wed Nov 23, 2011 3:45 pm 
Offline
Forum Commoner

Joined: Mon Nov 21, 2011 1:16 am
Posts: 69
Quote:
The chunk to return the value could be changed to this :
Syntax: [ Download ] [ Hide ]
if (mysql_num_rows($getdata)==0) {
    return FALSE;
}
else {
    $return_val = mysql_result($getdata,$value);
    mysql_free_result($getdata);
    return FALSE;
}


Wouldn't that return FALSE every time? Did you mean:
Syntax: [ Download ] [ Hide ]
if (mysql_num_rows($getdata)==0) {
    return FALSE;
}
else {
    $return_val = mysql_result($getdata,$value);
    mysql_free_result($getdata);
    return $return_val;
}

or am I missing something?

Quote:
Make sure you do proper variable cleansing before passing them to this function

Thanks; this is not done because all values are supplied by hard code (none from the user) when using this function.

Quote:
IMO best for a script to never produce anything and a development environment should display ALL levels of error reporting, including Notices

You're right, I did have problems with this in the past when moving from my dev machine to a live test environment. (I'll look into fixing that.)

Quote:
Also you will never be able to use this function once output is started without generating warnings. (But if you program like me, by the time you start outputting anything, all the info has been collected)

Yup. I never ever send output before all the input is collected. (That's good to know though.)

Quote:
Another thing which might make no difference is to use FALSE instead of NULL but this is just a personal preference.

The reason I prefer NULL over FALSE is that sometime a result of FALSE would be correct if I am trying to pull a TRUE/FALSE value from the database.

Quote:
There's also no checking of what type of data you receive so this is an area where you can improve

I wanted to use this function for any data types, so instead of having the function check that, I have code for that outside the function checking what it returned.

Quote:
I would throw an exception instead of using die() to check for any failing on the script.

How would I do that?

Quote:
You don't say explicitly if you use it for production purposes but if you do you might want to use the @ sign to suppress potential warning messages

It will eventually be used for production, but not for a while. Where do you put the @ sign to suppress warning messages? (I'm not going to add it until I think there are no warnings and have tested it on a production machine.)

The slightly (completely untested) modified function:
Syntax: [ Download ] [ Hide ]
<?php

//*********************************

//*********************************

//*** my_get.php by Hermit TL v0.2a

//***

//*** pulls a single existing value

//*** from a MySQL database
//***
//*** Required Values

//***   $value : Column name of requested value

//***   $table : Table name of where column resides

//***   $where : ie. username=someuser

//***

//*** Optional Values

//***   $error : Custom error to be displayed

//***   $override : Use this MYSQL statment to query (if this value is specified all other values will be ignored)

//*********************************

//*********************************





function my_get($value, $table, $where, $error = NULL, $override = NULL){



        if(!$override){
                $query = "SELECT " . $value . " FROM " . $table . " WHERE " . $where;
        }else{
                $query = $override;
        }

        $getdata = mysql_query($query) or die ('

        <table class="page">

                <tr><td class="error">Error returned from my_get function.</td></tr>

                <tr><td class="error">

                Error: '
.mysql_error() .

                '</td></tr>

                <tr><td class="error">'
.

                $query . '

                </td></tr>

                <tr><td class="error">

                '
.$error.'

                </td></tr>

        </table>

        '
);
        if (mysql_num_rows($getdata)==0) {
            return FALSE;
        }
        else {
            $return_val = mysql_result($getdata,$value);
            mysql_free_result($getdata);
            return FALSE;
        }

}



?>


Top
 Profile  
 
PostPosted: Wed Nov 23, 2011 4:48 pm 
Offline
DevNet Master
User avatar

Joined: Sun Feb 15, 2009 12:08 pm
Posts: 2794
Location: .za
Hermit TL wrote:
The reason I prefer NULL over FALSE is that sometime a result of FALSE would be correct if I am trying to pull a TRUE/FALSE value from the database.

I see
Syntax: [ Download ] [ Hide ]
<?php
 $getdata = mysql_query($query);
 if (!$getdata) {
     throw new Exception('Error message');
 }
 else {
     // continue processing
 }
//
# when using the function you would do something like this
try {
    my_get($value, $table, $where, $error = NULL, $override = NULL);
}
catch (Exception $e) {
    // echoes "Error Message"
    echo $e->getMessage();
}
 ?>

The Manual on Exceptions
http://php.net/manual/en/language.exceptions.php

Hermit TL wrote:
Where do you put the @ sign to suppress warning messages?

Before the mysql_query() function i.e @mysql_query($yourQuery); This means the warning message resulting from a failed query will not be echoed to the browser but lets you handle it yourself; like with an exception. More useful when the script is live, during production leave it so you can find errors / problems easier.

_________________
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering


Top
 Profile  
 
PostPosted: Thu Nov 24, 2011 2:54 am 
Offline
Forum Regular
User avatar

Joined: Tue Sep 28, 2010 11:41 am
Posts: 984
Location: Columbus, Ohio
Hermit TL wrote:
Did you mean:
Syntax: [ Download ] [ Hide ]
if (mysql_num_rows($getdata)==0) {
    return FALSE;
}
else {
    $return_val = mysql_result($getdata,$value);
    mysql_free_result($getdata);
    return $return_val;
}

or am I missing something?



Yes you are correct on the fix, I must have been tired when I wrote that.... Sorry, but glad you figured out what I meant.

-Greg


Top
 Profile  
 
PostPosted: Thu Nov 24, 2011 11:47 pm 
Offline
Forum Contributor

Joined: Sat Jan 03, 2009 4:27 pm
Posts: 148
Hermit TL wrote:
The reason I prefer NULL over FALSE is that sometime a result of FALSE would be correct if I am trying to pull a TRUE/FALSE value from the database.


I didn't think booleans could be returned from (or stored in) a database. Aren't the data types used one of string, integer, double, blob, or NULL?


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 7 posts ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: No registered users and 3 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
Powered by phpBB® Forum Software © phpBB Group