Reputation: 2297
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
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