PHP OOP with MySQL

Question

ok Guys have a big trouble and cann't anderstand what is going on. Please help.

first of all I have a DB connection class:

    private $localhost = DB_LOCALHOST;
    private $db = DB_DATABASE;
    private $user = DB_USER;
    private $password = DB_PASSWORD;
    protected $link ;


    function __construct()
      {

       $this->link = mysql_connect($this->localhost,$this->user,$this->password) or die(mysql_error()."<br/>".mysql_errno());
       mysql_query("SET NAMES ".DB_CHARSET , $this->link) or die(mysql_error()."<br/>".mysql_errno());
       mysql_select_db($this->db, $this->link)or die(mysql_error()."<br/>".mysql_errno());
           if($this->link===false)
           die("Didn't connect to DB");            

       }

       function sql_query($q)
       {    
          $r = mysql_query($q, $this->link) or die ("DATABASE ERROR! Please, contact the administrator.".mysql_error().);
          return $r;
       }

        function __destruct()
        {
           mysql_close($this->link);
        }

      }

then I use it in another one in two different functions:

     function del_meal($id_meal)
     {
         $db_s = new DBConnect();
     $db_s->sql_query("DELETE FROM `pictures` WHERE `id_target` = '{$id_meal}' AND `type` = 'meal';");
     $db_s->sql_query("DELETE FROM `meal_ingredient` WHERE `id_meal` = '{$id_meal}';"); 
     $db_s->sql_query("DELETE FROM `meal` WHERE `id_meal` = '{$id_meal}';"); 
     unset($db_s);
     $pictures = $this->select_pictures($id_meal,'meal');
         if(count($pictures)>0)
         {
           foreach($pictures as $pic)
           {
            if(file_exists("./pictures/meal/{$pic}"))
             {
               unlink("./pictures/meal/{$pic}");
               unlink("./pictures/meal/thumb/{$pic}");
             }
           }
         }
     }

    function del_category($id_category)
    {
       $db = new DBConnect();
       $id_category = $db->safe_var($id_category);
       $q = $db->sql_query("SELECT * FROM `meal` WHERE `id_category` ='{$id_category}' ");
    while($res = mysql_fetch_assoc($q))
    {
        $this->del_meal($res['id_meal']);
    }
    $db->sql_query("DELETE FROM `meal_category` WHERE `id_category` = '{$id_category}' ");
    unset($db);
    }

ok - here is safe_var()

   function safe_var($var)
  {
      $var = stripslashes($var);
      $var = trim($var);
      $var = strip_tags($var);
      $var = mysql_real_escape_string($var);
      $var = htmlspecialchars($var);
      $var = nl2br($var);

      return $var;
  }

So - when I use just del meal function - everything is fine - but when I use del category - I get "Warning: mysql_query(): 7 is not a valid MySQL-Link resource in DBConnect.php on line 33 DATABASE ERROR! Please, contact the administrator."

What can be the reason. Can somebody help me?

You checked out mysqli()? OOP MySQL out of the box - php.net/mysqli — Jason McCreary
– Jason McCreary, Commented May 12, 2011 at 19:55
Have you checked that your constructor is even executing? If your host is still on PHP4, auto-execution of __construct is not supported — Marc B
– Marc B, Commented May 12, 2011 at 20:06
@Marc B: If he was running this on PHP4, it would parse error on the private/public member declarations. — webbiedave
– webbiedave, Commented May 12, 2011 at 20:08
well, it's obvious that somewhere $this->link being overwritten by an integer(probably id_category). I also spotted that $link is declared with a public scope. Is this necessary for anything? Also applying a try-catch with a print-out of the backtrace would be very helpful for you. — Bryan
– Bryan, Commented May 12, 2011 at 21:06

Xeoncross · Accepted Answer · 2011-05-12 20:29:14Z

2

Like SkippyChalmers said, you need a single Database instance.

function db($config)
{
    static $db = NULL;
    if($db === NULL)
    {
        $db = new DBConnect($config);
    }
    return $db;
}

// db()->sql_query(...);

However, I recommend that you switch to PDO.

answered May 12, 2011 at 20:29

Xeoncross

57.6k84 gold badges272 silver badges371 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

Aaria Carter-Weir Over a year ago

+1 agreed. Although a wrapper class rather than wrapper function? Or use of static property (accessed by a static method).

Xeoncross Over a year ago

@SkippyChalmers, a wrapper class would also be a bad idea as both of these methods break Dependency Injection. However, given the level of code he is producing I opted for a simple wrapper function that seems to match the level of code he is producing. Singleton is still bad though and should only be used on very small applications where the overhead of DI isn't worth it.

Aaria Carter-Weir Over a year ago

@Xeoncross, interesting thoughts there. I see what you mean, and yes - actually agree. For the level and purposes of this application a simple wrapper function makes sense. I wasn't meaning actually implementing the singleton pattern though, I meant using a static property and then calling a static getter method on it from within an instantiated object. It's late and I'm doubting myself, but would that not ensure the persistence of the connection across multiple instantiations?

Xeoncross Over a year ago

@SkippyChalmers, you could do a static method or an individual function. However, I find db()->query() nicer to type than DB::get_instance()->query(). Plus, if you extend the DB class you would have to go back through your application and replace all the DB::get_instance() with My_DB::get_instance() (or alter the DB::get_instance() method to return a My_DB object).

Aaria Carter-Weir Over a year ago

Too right :) I think I got mixed up and wanted to do $db = new db(); and then store the link resource as a static property which could be accessed via a static method, using the self:: operator.

|

Aaria Carter-Weir · Accepted Answer · 2011-05-31 17:40:48Z

2

if($link===false)

Should be:

if($this->link===false)

Should it not?

Also, you can do better than this! Design it so the DB connection is persistent across all uses of the class. One new connection for every query is not good practice. Good start though. As Jason McCreary said, use mysqli, or PDO or something out of the box. Lot's of third party libraries out there too. Have you considered a data-mapper solution? This is a good start though!

EDIT: I'm going to recommend using a dependency injection container to properly persist the DB connection.

edited May 31, 2011 at 17:40

answered May 12, 2011 at 20:23

Aaria Carter-Weir

1,67912 silver badges17 bronze badges

3 Comments

devworkstation Over a year ago

Yes - Ofcourse should be $this->link. And also You are right about a new connection for each query - I will look in this side.

Aaria Carter-Weir Over a year ago

Ensure error reporting is on. I might need to take a closer look but from this far away, it looks like you're not actually connecting successfully.

Aaria Carter-Weir Over a year ago

Okay... sorry @Gavryshuk, I've made a bit of a mess of this by not actually reading your code properly. Can you modify the sql_query method to do a var_dump of $this->link? Can you also remove as much uncessary code from your example as possible? (After testing that the remaining code still produces the error)

score 0 · Accepted Answer · 2011-05-12 20:24:05Z

0

I council you, to Extend database class, or make DB class instance 1 time in construct.

Example with Extend:

class Anything Extends Database {
///and here you can use DB methods as $this->sql_query...
}

Second way is:

class Anything {

   private $db;

   public function __construct() {
      $this->db = new DBConnect();
      //then use db as $this->db->sql_query in the class with this same instance.
   }
}

I think these are right ways.

OR Try to edit your constructor. Try this:

function __construct()
      {

       $this->link = mysql_connect($this->localhost,$this->user,$this->password) or die(mysql_error()."<br/>".mysql_errno());
       mysql_select_db($this->db, $this->link)or die(mysql_error()."<br/>".mysql_errno());
       mysql_query("SET NAMES ".DB_CHARSET , $this->link) or die(mysql_error()."<br/>".mysql_errno());
           if($this->link===false)
           die("Didn't connect to DB");            

       }

edited May 12, 2011 at 20:24

answered May 12, 2011 at 20:15

user744116

2 Comments

devworkstation Over a year ago

Yes - should $this->link - but after I fixed it doesn't change anything

Aaria Carter-Weir Over a year ago

Sorry, don't see how you've actually achieved anything by extending a db class or implementing a wrapper class, so -1 for now. You've not actually made use of a persistent connection in your wrapper class either.

Collectives™ on Stack Overflow

PHP OOP with MySQL

3 Answers 3

7 Comments

3 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

7 Comments

3 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related