Class for sending emails

Question

I'm 100% sure improvements can be made. Any help is greatly appreciated. I should add that I am new to OOP and am not sure if I'm using this the right way.

<?php
defined('_VALID') or die('Restricted Access!');
class email
{
public function addtemplate($html, $public)
{
 global $conn;
 $sql = "INSERT INTO emailtemplate SET id = '', parentid = '" . $_SESSION['uid'] . "', html = '" . mysql_escape_string($html) . "', public = '" . $public . "', added = '" . time() . "'";
 $conn->execute($sql);
 if ($conn->affected_rows() == 1) {
 return true;
 } else {
 return false;
 }
}
public function addtext($subject, $text)
{
 global $conn;
 $sql = "INSERT INTO emailtext SET id = '', parentid = '" . $_SESSION['uid'] . "', subject = '" . mysql_escape_string($subject) . "', text = '" . mysql_escape_string($text) . "', added = '" . time() . "'";
 $conn->execute($sql);
 if ($conn->affected_rows() == 1) {
 return $conn->insert_id();
 } else {
 return false;
 }
}
public function addqueue($to, $text, $template)
{
 global $conn;
 $sql = "INSERT INTO emailqueue SET eid = '', parentid = '" . $_SESSION['uid'] . "', eto = '" . $to . "', etext = '" . $text . "', etemplate = '" . $template . "', added = '" . time() . "', status = 'pending'";
 $conn->execute($sql);
 if ($conn->affected_rows() == 1) {
 return true;
 } else {
 return false;
 }
}
public function delete($eid)
{
 global $conn;
 $sql = "DELETE FROM emailqueue WHERE eid = '" . $eid . "' AND parentid = '" . $_SESSION['uid'] . "' AND status = 'pending'";
 $conn->execute($sql);
 if ($conn->affected_rows() == 1) {
 return true;
 } else {
 return false;
 }
}
public function generate($eid)
{
 global $conn;
 $sql = "SELECT q.*, x.*, t.*, c.id, c.fname, c.email, p.UID, p.company, p.hash FROM emailqueue AS q, emailtext AS x, emailtemplate AS t, clients AS c, 
 signup AS p WHERE q.eid = '" . $eid . "' AND q.etemplate = t.id AND q.etext = x.id AND c.id = q.eto AND p.UID = q.parentid";
 $rs = $conn->execute($sql);
 $email = $rs->getrows();
 $email = $email[0];
 $find = array(
 '{name}',
 '{email}',
 '{company}',
 '{hash}'
 );
 $replace = array(
 $email['fname'],
 $email['email'],
 $email['company'],
 $email['hash']
 );
 $body = str_replace('{bodytext}', $email['text'], stripslashes($email['html']));
 $final = str_replace($find, $replace, $body);
 $array['body'] = stripslashes($final);
 $array['subject'] = $email['subject'];
 $array['to'] = $email['email'];
 return $array;
}
public function from($eid)
{
 global $conn;
 $sql = "SELECT q.*, s.UID, s.email, s.company FROM emailqueue AS q, signup AS s WHERE q.eid = '" . $eid . "' AND q.parentid = s.UID";
 $rs = $conn->execute($sql);
 $email = $rs->getrows();
 $email = $email[0];
 $array['company'] = $email['company'];
 $array['email'] = $email['email'];
 return $array;
}
public function send($eid)
{
 $msg = $this->generate($eid);
 $from = $this->from($eid);
 $headers = "From: " . $from['company'] . " <[email protected]>\r\n";
 $headers .= "Reply-To: " . $from['email'] . "\r\n";
 $headers .= "MIME-Version: 1.0\r\n";
 $headers .= "Content-type: text/html; \r\n";
 if (mail($msg['to'], $msg['subject'], $msg['body'], $headers)) {
 return true;
 } else {
 return false;
 }
 }
}
?>

• bitsoflogic: defined('_VALID') or die('Restricted Access!'); // Restricting access is best done in an OOP way as well. Additionally, it would be better to have this closer to the code that is concerned with whether or not someone has access.
• bitsoflogic: General Rule: Globals are bad. Rather than a global $conn, use Dependency Injection. If most methods require it, pass it in via the constructor (or factory method) otherwise pass it in to the specific method(s) that require it.
• Meisam Mulla: I have replaced the global with a constructor. I don`t understand what you mean by your first comment.
• bitsoflogic: It may simply be that I don't understand your business case for having the defined('_VALID') check. Whatever the reason, it seems misplaced. Class files are best left with nothing more than the class itself. Only your app will be loading the classes defined, so you should manage access requirements elsewhere.
• Meisam Mulla: Any script that tries to include this file without having _VALID defined will fail to do so. I see your point though.

Answer 1

Not a PHP expert, but things like

if ($conn->affected_rows() == 1) {
 return true;
} else {
 return false;
}

should be

return $conn->affected_rows() == 1;

• Meisam Mulla: Would this return false in the event $conn->affected_rows() is not 1?
• mAu: Yes. But also if its not true. Better to check $conn->affected_rows() === 1

Answer 2

See this page on SQL injection and why you shouldn't do it. You run a security risk that allows a user with a little maliciousness and knowledge of SQL to purposefully bypass certain checks in order to cause your server to possibly crash. I see you escape certain values but not all. You had better either be sure that the values which arrive are already escaped or you should add it in.

• Meisam Mulla: The values that are not escaped are generated by my script so I saw no use in escaping them.