jackhammer013
jackhammer013

Reputation: 2297

PDO MySQL on Class functions

I have a class called Form. and I have a add_form() method which I call to save record to the database. add_form() basically adds the record to the database and returns rows affected. When I am using this function without the use of Classes or OOP Approach, it works fine. But now I get error says exec() is an undefined function. and variable count not defined? I use exec() because it returns rows affected instead of execute() which only executes the query.

class Forms
{
  private $database;
  private $count;

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

  function add_form($lastname, $firstname, $island, $region, $province, $baranggay, $city, $address, $gender, $birthdate)
  {
    $sql  = "INSERT INTO forms (lastname, firstname, island, region, province, baranggay, city, address, gender, birthdate) VALUES (:lastname, :firstname, :island, :region, :province, :baranggay, :city, :address, :gender, :birthdate)";
    $stmt = $this->database->prepare($sql);
    $count->exec(array(
                  ':lastname'=>$lastname,
                  ':firstname'=>$firstname,
                  ':island'=>$island,
                  ':region'=>$region,
                  ':province'=>$province,
                  ':baranggay'=>$baranggay,
                  ':city'=>$city,
                  ':address'=>$address,
                  ':gender'=>$gender,
                  ':birthdate'=>$birthdate
                  ));
    return $count;
   }

}

Upvotes: 0

Views: 106

Answers (1)

Elias Van Ootegem
Elias Van Ootegem

Reputation: 76395

A couple of things, first: the method you need to call is called PDOStatement::execute, not exec. The exec method is a method of the PDO class as the manual clearly shows. PDO::exec, however, isn't very safe, because it does not use prepared statements, so it's good of you to use a PDOStatement, but then use the appropriate methods. It's quite easy to get the number of affected rows from a prepared statement, too...
Next, you assign the prepared statement (PDOStatement instance) to a variable called $stmt, but are calling exec on an undefined variable: $count:

   $stmt = $this->database->prepare($sql);
   $count->exec(array( //<--- should be $stmt->execute(array());
                  ':lastname'=>$lastname,
                  ':firstname'=>$firstname,
                  ':island'=>$island,
                  ':region'=>$region,
                  ':province'=>$province,
                  ':baranggay'=>$baranggay,
                  ':city'=>$city,
                  ':address'=>$address,
                  ':gender'=>$gender,
                  ':birthdate'=>$birthdate
));

Then, to get the number of affected rows, you need to call the rowCount method:

return $stmt->rowCount();

Put these issues together, and applied to your code, you should change your code to:

$stmt->execute(array());
$count = $stmt->rowCount();
//optional:
$stmt->closeCursor();
return $count;

For completeness, here's an unsafe version using exec:

$vals = array(
    'firstname' => 'Joe',
    'lastname'  => 'Goodboy'
);
$count = $pdo->exec(
    'INSERT INTO hackable (firstname, lastname)
    VALUES ("'.$vals['firstname'].'", "'.$vals['lastname'].'")'
);//all is well
//BUT:
$vals = array(
    'firstname' => 'Evil", /*', 
    'lastname'  => '*/ (SELECT CONCAT_WS(",", id, username, password)
        FROM users
        WHERE isAdmin = 1
        LIMIT 1
    )); --'
$count = $pdo->exec(
    'INSERT INTO hackable (firstname, lastname)
    VALUES ("'.$vals['firstname'].'", "'.$vals['lastname'].'")'
);//OUCH!!

The latter input results in the query:

INSERT INTO hackable (firstname, lastname)
VALUES ("Evil", /*, "*/
    (SELECT CONCAT_WS(",", id, username, password)
     FROM users
     WHERE isAdmin = 1
    )); -- ", "")

Which just goes to show that executing a query with user input is just a bad idea...

As an asside, some code-review:
Please, make it a habit to always write the access modifiers, and subscribe to the coding standards most major players subscribe to. That means methods should be camalCased: add_form should be addForm. Also do yourself a favour, and add doc-blocks:

/**
 * Inserts data provided by $bind into forms table
 * @param array $bind
 * @return int
 * @throw PDOException (if PDO::ERRMODE is set to PDO::ERRMODE_EXCEPTION)
 */
public function addForm(array $bind)
{//PUBLIC + camelCased name
    $stmt = $this->database->prepare($queryString);
    $stmt->execute($bind);
    return $stmt->rowCount();
}

But on the whole, this form has a fixed layout, and a given number of values is expected. In larger projects, you'll probably end up defining a data class for this:

class From extends DataModel
{//where DataModel is an abstract class
    protected $lastname = null;
    protected $firstname = null;
    //all properties hiere
    public function __construct(array $data)
    {//use assoc array in constructor
        foreach ($data as $key => $value)
        {
            $setter = 'set'.ucfirst($key);
            if (method_exists($this, $setter))//define setters for properties!
                $this->{$setter}($value);
        }
    }
    /**
     * method to turn instance into bind array
     * Optionally omit null values for WHERE clauses
     * @param bool $includeNulls = true
     * @return array
     */
    public function toBind ($includeNulls = true)
    {
        $bind = array(
            ':firstname' => $this->firstname,
            ':lastname'  => $this->lastname,
            //do this for all properties
        );
        if ($includeNulls === false)
            return array_filter($bind);//unset null values
        return $bind;
    }
}

If you want to know why I urge you to use setters and getters, instead of magic __get and __set calls, check out some of my answers on codereview.stackexchange. I've explained it all before in detail

Upvotes: 2

Related Questions