Php class not allowing class-wide scoping

Question 1

The way you have written your code, if the query fails, you'll never know. It just silently returns from your add() function even if it didn't do anything.

You should always do something to report the error if your prepare() or execute() fails. These functions return false if there's a problem, and if that happens you should output the error (or at least log the error even if you want to show only a friendlier "we are experiencing technical difficulties" screen to users).

Here's an example:

public function add($page, $element_id, $text){
    if(($this->query = $this->sql->prepare($this->addquery)) === false){
        trigger_error($this->sql->error, E_USER_ERROR);
    }
    $this->query->bind_param("sss", $page, $element_id, $text);
    if ($this->query->execute() === false)) {
        trigger_error($this->query->error, E_USER_ERROR);
    }
    $this->query->close();
}

Do something similar for both prepare() and execute() in your grab() function too, and every time you prepare or execute an SQL statement. It's tedious, but necessary.

Mysqli is supposed to have a capability to report errors by throwing exceptions, but this isn't enabled by default. You can read more about it here: http://www.php.net/manual/en/mysqli-driver.report-mode.php

Question 2

As I see it, you are trying to access $addquery and $grabquery in your methods, which are not class members but local variables to your constructor.

So because PHP doesn't require you to declare variables, they are evaluated to empty strings.

Just put them into your class member list.

e.g. protected $grabquery and access them by using the $this pointer $this->$grabquery

Question 3

Try lowercasing the table name "content" and it should work. I renamed this->sql to this->db for sanity and removed grab method for brevity.

class ContentManager{
    protected $db;

    protected $host     = "localhost";
    protected $username = "";
    protected $password = "";
    protected $dbname   = "";

    protected $query;
    protected $addquery = "INSERT INTO content (Page, ElementID, Text) VALUES (?, ?, ?)";
    protected $grabquery    = "SELECT ElementID, Text FROM content WHERE Page = ?";



    public function __construct(){

        $this->db = new mysqli($this->host, $this->username, $this->password, $this->dbname);

        if(mysqli_connect_errno())
            die("Connection to DB failed, please contact web administrator at: michaelwiley@gmx.com");
    }


    public function add($page, $element_id, $text) {

        if (! ($stmt = $this->db->prepare($this->addquery)))
        {
            echo "Prepare failed: (" . $this->db->errno . ") " .$this->db->error;
        } else {

            $stmt->bind_param("sss", $page, $element_id, $text);
            $stmt->execute();
            $stmt->close();        
        }
    }
}


//test :
$content = new ContentManager();
$content->add('testcrm', 'divcontainer','<h1>test</h1>');

Question 4

I think you're using the wrong class. You are binding params using bind_params and the mysql object doesn't have this:
- http://nl3.php.net/manual/en/class.mysqli.php

The class you are looking for is probably:
- http://nl3.php.net/manual/en/class.mysqli-stmt.php

I have no experience with this as I have always used ORM after I learned mysql(i)_query and the mysqli_object. I would also suggest to turn on error_report to E_ALL, because if I'm right, you were supposed to get a fatal error.

Question 5

I quote :

$addquery   = "INSERT INTO Content (Page, ElementID, Text) VALUES (?, ?, ?)";
$grabquery  = "SELECT ElementID, Text FROM Content WHERE Page = ?";

if you want to use these variable in instance methods you need to declare them as instance variables

$this->addquery   =  ...
$this->grabquery  = ...

and use them as such in instance methods

public function grab($page){
        if($query = $this->sql->prepare($this->grabquery)){ 
        ...

Question 6

Your code is working in my local station ( PHP 5.4 ), i guess the problem should be with your database columns, please check if you Content table has the Page, ElementID and Text columns, exactly with this format.

Best Regards

Code example, it creates the Table if table doesn't exist, so try to delete the Table Content before using it.

Url used: ?func=add&page=testcrm&id=divcontainer&content=<h1>test</h1>



class ContentManager{
    protected $sql;
    protected $host     = "localhost";
    protected $username = "root";
    protected $password = "passsword";
    protected $dbname   = "tempdb";
    protected $query;
    protected $addquery = "INSERT INTO Content (Page, ElementID, Text) VALUES (?, ?, ?)";
    protected $grabquery    = "SELECT ElementID, Text FROM Content WHERE Page = ?";



    public function __construct(){

        $this->sql = new mysqli($this->host, $this->username, $this->password, $this->dbname);

        $test_query = "CREATE TABLE IF NOT EXISTS `Content` (
            `Page` varchar(255) NOT NULL,
            `ElementID` varchar(255) NOT NULL,
            `Text` varchar(255) NOT NULL
         ) ENGINE=InnoDB DEFAULT CHARSET=latin1;";

        $query = $this->sql->prepare($test_query);
        $query->execute();

        if(mysqli_connect_errno())
            die("Connection to DB failed, please contact web administrator at: michaelwiley@gmx.com");

    }

    public function add($page, $element_id, $text){
        if($this->query = $this->sql->prepare($this->addquery)){
            $this->query->bind_param("sss", $page, $element_id, $text);
            $this->query->execute();
            $this->query->close();
        }
    }

    public function grab($page){
        if($this->query = $this->sql->prepare($this->grabquery)){
            $this->query->bind_param("s", $page);
            $this->query->execute();
            $this->query->bind_results($elementID, $text);
            $results = array();
            while($row = $this->query->fetch_assoc()){
                $results[] = $row;
            }
        }
        return $results;
    }

    public function __destruct(){
        $this->sql->close();
    }
}

spl_autoload_register('classloader');

function classloader($className){
    include ("/classes/".$className.".php");
}

$content = new ContentManager();

$action = $_GET['func'];
if($action==='add'){
    $content->add($_GET['page'], $_GET['id'], $_GET['content']);
}

Question 7

More importantly than this specific class not working is the design you have implemented here is non-orthogonal. Good code should follow the DRY (don't repeat yourself) principle. In this class, the intent is to manage content. However, you are dealing with a bunch of database connection stuff. This means the class is doing two things: database connection management, as well as content management. This is non-orthogonal code.

To fix this, you should define a requirement for the construction of your ContentManager class. Namely, that it be passed a valid, connected database handle:

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

So the db handle needs to come from somewhere. This should be a DB connection factory method.

class DbFactory {

    public static function getConnection($host, $username, $password, $dbname) {
       //alternatively load this information from a configuration file for your site,
       //perhaps using a standard format/utility pair like json

        return new mysqli($host, $username, $password, $dbname);
    }

}

Now you can create a unit test to ensure that you are able to generate a valid connection from your db factory (and test it in the unit test). And a separate test to ensure that when given a valid db handle your connection manager will call the correct methods on it..

Since you have no other specific indicating an error message I can't outline how to create a test case to reproduce and then fix that error, but that is the next step.

Finally, you could take it one step further and instead of writing raw sql for every insert and select statement, just use the model/mapper pattern. This pattern uses the field names of your data class to generate the sql statements for corresponding inserts and updates. This frees you from having to write lots and lots of potential sql for common simple things like 1:1 mapping of data in your php app to data in tables in your database.

Code I write using this pattern ends up looking something like this:

class Content {

    protected $id;
    protected $pageName;
    protected $pageContent;

}

class ContentMapper{

    protected $tableName = 'content';

    public function __construct($conn) {
        $this->conn = $conn;
    }
    public function find($id) {
         $result = $this->conn->
             select()->
             from($this->tableName)->
             where('id', $id);
         if(isset($result[0])) {
             $returnContent = new Content();
             $returnContent->id = $result[0]['id'];
             $returnContent->pageName = $result[0]['pageName'];
             $returnContent->pageContent = $result[0]['pageContent'];
             return $returnContent;
         }
         return null;
    }

    public function save(Content $content) {
         $result = $this->conn->
             insertOrUpdate()->
             field('id', $content->id)
             field('pageName', $content->pageName)
             field('pageContent', $content->pageContent)
             where('id', $content->id);

    }
}

This is kind of rough code, but you get the general picture. You can pm me if you want full examples of how this works in practice, but it is very simple and clean.