MySQL within classes causing frustration

Question

I have this code:

<?php 

class guildData {

    public $email = NULL;
    public $hash_pw = NULL;
    public $user_id = NULL;
    public $clean_username = NULL;
    public $display_username = NULL;

    public function selectGuild($g_id)
    {
            global $db,$db_table_prefix;

            $this->g_id = $g_id;

            $sql = "SELECT
                            name
                            FROM
                            guild
                            WHERE
                            id = '".$g_id."'";

            $result = $db->sql_query($sql);

            $row = $db->sql_fetchrow($result);

            return ($row['name']);
    }
}
?>
<?php echo $guildData->selectGuild(1); ?>

I'm simply getting a 500 error and IDEone gave me this also:

Fatal error: Call to a member function selectGuild() on a non-object in /home/VT00Ds/prog.php on line 32

I can not see the error, can you help me?

Instead of using the `global` keyword you should inject all the needed services into the constructor of the class or into the methods that need it. (Not an answer to your question though) — PeeHaa, Aug 09 '12 at 18:57
@PeeHaa or define a DB class that has those variables as `static` member variables. — Matt, Aug 09 '12 at 19:04
Singletons?? Never ever ever use that word (nor that antipattern) again ;) — PeeHaa, Aug 09 '12 at 19:10
If you are using `static` calls in your code you are tightly coupling the database class to the class where you use it. It is hard to tell the database class is just when just looking at the class signature and besides the singleton antipattern is just another word for a `global`. I like your education about dropping `mysql_*` functions though :) — PeeHaa, Aug 09 '12 at 19:11
@PeeHaa I hadn't thought of it that way before. Sometimes I feel that SO is a better education than college was! — Matt, Aug 09 '12 at 19:19
@Matt I don't know how the educational system is over there so I can only talk for myself. But SO (especially [chat](http://chat.stackoverflow.com/rooms/11/php)) is waaaaay better than college :D — PeeHaa, Aug 09 '12 at 19:22
@PeeHaa I actually attended top Universities over here, but there's a difference between school (theory) and practical applications / industry practices. Technology changes too rapidly for any curriculum to properly teach current best-practices. — Matt, Aug 09 '12 at 19:27
@Matt , i guess you do not understand why global state is bad .. — tereško, Aug 09 '12 at 19:52
@tereško That's not the case. In fact, I never use `global` in any of my code. There are always ways around it, if need be, and I understand the importance of scope (I started out as a C++/Java developer). I just never thought of a singleton as a `global` before. — Matt, Aug 09 '12 at 19:55
@tereško I found an article [here](http://tomnomnom.com/posts/why-global-state-is-the-devil-and-how-to-avoid-using-it) explaining it in a non-condescending manner. Thanks for keeping things civil. — Matt, Aug 10 '12 at 12:51

score 4 · Answer 1 · edited May 23 '17 at 11:56

You are doing it wrong.

Get rid of the global variables. Instead , if the class needs DB access , then you should inject it in the constructor:

class GuildData
{
    //  ... snip 

    protected $connection;

    public function __construct( PDO $connection )
    {
        $this->connection = $connection;
    }

    //  ... snip 
}

Your code has potential for SQL injections. Instead of concatenating the query, you should use prepared statements:

$statement = $this->connection->prepare(
                'SELECT name FROM guild WHERE id = :id'
             );
$statement->bindParam( ':id', $this->g_id, PDO::PARAM_INT );
if ( $statement->execute() )
{
    $data = $statement->fetch( PDO::FETCH_ASSOC );
}

You have to instantiate object, before you can use them:

$pdo = new PDO('mysql:host=localhost;dbname=myMagicalDB;charset=UTF-8', 
               'username', 'password');
$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

$guild = new GuildData( $pdo );
$guild->selectGuild(42);

You might consider separating the part with deals with DB from the domain logic. Essentially letting some other class to handle the fetching and storing the data, while the Guild class manages the logic. You might find this and this answer relevant.
Do not use public variables. You are breaking the encapsulation of the object by directly exposing the internal data. Instead you should define them as protected or private.

You might also take a hard look at what you are actually keeping there. Why does GuildData need $hash_pw or $clean_username ?

score 3 · Accepted Answer · edited May 23 '17 at 11:56

You haven't instantiated $guildData. If you want to use this method without instantiating an object, the method should be static.

class guildData {
    public static function selectGuild($g_id) { ... }
}

Then you can call it from

echo guildData::selectGuild(1);

Otherwise, you need to instantiate an object

$guildData = new GuildData();

echo $guildData->selectGuild(1);

Also, you should have some sort of __construct() method in there, in order to set up your member variables.

UPDATE I also noticed an error in your selectGuild() method:

$this->g_id = $g_id;

Sets the value of g_id, which is not defined as a member variable in the class. You must declare g_id as a member variable in your class definition:

class guildData {
    public $email = NULL;
    public $hash_pw = NULL;
    public $user_id = NULL;
    public $clean_username = NULL;
    public $display_username = NULL;

    public $g_id = NULL;

    .
    .
    .

}

Finally, sql_query() is not a PHP method that I've ever heard of. Unless you're using a library that defines these methods, I think you mean mysql_query(). If this is the case, you should stop using mysql_* functions. They're being deprecated. Instead use PDO (supported as of PHP 5.1) or mysqli (supported as of PHP 4.1). If you're not sure which one to use, read this SO article.

Rookie mistake, fantastically fast answer, thank you Matt <3 — Solidarity, Aug 09 '12 at 18:57
On your update, I'm using a library which defines things like sql_query using PDO I do believe, thank you anyway :3. — Solidarity, Aug 09 '12 at 19:04

MySQL within classes causing frustration

2 Answers2