1
\$\begingroup\$

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'] . " <noreply@emailsmsmarketing.com>\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;
    }
    }
}
?>
\$\endgroup\$
5
  • \$\begingroup\$ 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. \$\endgroup\$ Sep 19 '11 at 16:13
  • \$\begingroup\$ 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. \$\endgroup\$ Sep 19 '11 at 16:17
  • \$\begingroup\$ I have replaced the global with a constructor. I don`t understand what you mean by your first comment. \$\endgroup\$ Sep 20 '11 at 9:23
  • \$\begingroup\$ 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. \$\endgroup\$ Sep 30 '11 at 18:09
  • \$\begingroup\$ Any script that tries to include this file without having _VALID defined will fail to do so. I see your point though. \$\endgroup\$ Oct 3 '11 at 10:40
1
\$\begingroup\$

Not a PHP expert, but things like

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

should be

return $conn->affected_rows() == 1;
\$\endgroup\$
2
  • \$\begingroup\$ Would this return false in the event $conn->affected_rows() is not 1? \$\endgroup\$ Sep 19 '11 at 11:57
  • 2
    \$\begingroup\$ Yes. But also if its not true. Better to check $conn->affected_rows() === 1 \$\endgroup\$
    – mAu
    Sep 19 '11 at 12:17
0
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ The values that are not escaped are generated by my script so I saw no use in escaping them. \$\endgroup\$ Sep 20 '11 at 9:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.