I come back to this at the end, but if this answer is tltr, basically, I end up suggesting not to write your own DB abstraction layer, It's been done before, way better than you're doing it now.
That doesn't mean the rest of this answer doesn't contain valuable tips on how to improve your code quality in general, so please do read through it:
For a start, when you're using an object to contain data, that object is responsible for that data's integrety, not the user of the object. By that I mean: if I am to use your object, you must be able to ensure that, regardless of how I use your object (I may forget the strip_tags call, for example) the input won't contain any markup.
The only way to do that is to define setter methods that take care of this:
class DataObjects_User
{
private $name = null;
public function getName()
{
return $this->name;
}
public function setName($name = null)
{//setting null ~= unsetting
if ($name !== null && !is_string($name))
{
throw new InvalidArgumentException('Name must be string');//don't hush-up errors
}
$this->name = is_null($name) ? $name : strip_tags($name);
return $this;
}
}
Your code appears to require me to set a creation date manualy. Why? Why not create a field in the table that has NOW() as a default value? or an ON UPDATE CURDATE clause? wouldn't that be easier?
even so, you could omit this, by simply adding a constructor to the object above, too:
class DataObjects_User
{
private $name = null;
private $created = null;
public function __construct()
{
$this->created = new \DateTime;//I prefer to keep everything OO
}
public function getCreated($format = 'Y-m-j H:i:s')
{//use class constants for the format
if ($format === false)
{//allow user to get the object itself?
return $this->created;
}
return $this->created->format($format);
}
public function setCreated(DateTime $obj = null)
{//again, null to unset
$this->created = $obj;
return $this;
}
public function getName()
{
return $this->name;
}
public function setName($name = null)
{//setting null ~= unsetting
if ($name !== null && !is_string($name))
{
throw new InvalidArgumentException('Name must be string');//don't hush-up errors
}
$this->name = is_null($name) ? $name : strip_tags($name);
return $this;
}
}
You may be wondering why I keep adding the option of setting the properties to null... well, that's perfectly simple: If these objects represent data in the DB, it's not unlikely you'll find yourself using these objects to build queries. If some of these properties have default values, or values that are irrelevant to a query, you can uset them, by passing null (or nothing, since it's the default value) to their setters.
$user = new DataObjects_User();//constructor sets created date!
$user->setName('Bobby');
->setCreated();//unset created
Now you can pass $user to a generic createBind() method. Besides, if you have a couple of these objects, just create an interface/abstract class if only for typehinting, and extend all dataobjects from that:
class DataObjects_User extends DataObjects_Abstract{}
//the createBind method then could look like this:
public function createBind(DataObjects_Abstract $model)
{
$getters = get_class_methods($model);
$bind = array();
foreach($getters as $getter)
{
if (substr($getter,0,3) === 'get' && method_exists('s'.substr($getter,1), $model))
{//getter and setter exist, this is a property
if ($model->{$getter}() !== null)
{//avoid null properties
$bind[':'.substr($getter,3)] = $model->{$getter}();
}
}
}
return $bind;
}
//when applied to $user
var_dump(
$queryObject->createBind(
$user->setName('Bobby')
->setCreated()
)
);
That should yield:
array(':name' => 'Bobby');
Which makes life a lot easier when using prepared statements, doesn't it?
since I'm on the subject: When using PDO, you don't have to bother with all those deprecated mysql_real_escape_string calls anymore... just use prepared statements. If you can't, then learn to use them. It's not that hard, and an awful lot safer at any rate than what you have now.
Basically, with the $user object, being passed through the createBind method, all you need to do is:
$bind = $queryObject->createBind(
$user->setName('Bobby')
->setCreated()
);
$fields = array_keys($bind);
$query = 'SELECT * FROM tbl WHERE ';
foreach($fields as $field)
{
$query .= substr($field,1) .' = '.$field;
}
$statement = $pdoInstance->prepare($query);
$statement->execute($bind);
This is just as secure as your code, if not more secure. The downsides:
- the properties of your objects have to be the same as the field-names in your db
- data and logic are too tightly coupled here...
Lastly, I'd like to point that, even though it's not something that'll leave a gaping hole in your app's security, storing passwords the way you are doing is not done. Common practice is to store password-hashes, not encrypted passwords. Encryption, as the word implies, is a two way deal: anything that is encrypted can be decrypted. A hash-function, by its very nature is a one-way algorythm: A hash can't be de-hashed (well, it can be brute-forced, but even then, collisions, salt and time-complexity make that less viable to would-be hackers). Instead of encrypting the pass, try this, inside the user model:
private $passHash = null;
const PASS_SALT = '$0m3_r@Nd0M_5tr1n9';
public function setPassHash($password)
{
$this->passHash = hash('sha256', self::PASS_SALT.$password);
return $this;
}
But that's just on the data model. Any abstraction layer you're writing to query the DB isn't going to cut it as your app grows. As I've stated previously, in other answers:
On the whole, I'd suggest you use Doctrine entities, though. ORM's aren't very inviting at first, but once you get the hang of how easy it is to distil tables from objects and objects from tables (yes, it works both ways), you haven't got anything to worry about.
