PHP OOP code works well. Is it correct practice? Will some future release break it?

Question

Code snippet for class lnemail_fetch

<?php  Part of heritage_classes.php
// Declare classes
class lnemail_fetch {
// return string in format "title | factoid" 
    public  $result;
    public function get_ln_info() 
    {
    include ("./includes/LOheritage-config.php");
    mysql_connect("$dbhost", "$dbuser", "$dbpass") or die(mysql_error());
    mysql_select_db("$dbname") or die(mysql_error());
      $query = "SELECT * FROM lnemail";
     $result = mysql_query($query);
         $this->result = $result;
    }
}
?>

Code Snippet from larger program It lists a MySQL table

    require_once('./includes/heritage_classes.php'); 

    $newlnemail_fetch = new  lnemail_fetch;
     $newlnemail_fetch->get_ln_info();
     $newresult  = $newlnemail_fetch->result;
     echo  "lnemail File display  <br />"; 

      while($row = mysql_fetch_array($newresult))
        {
         echo $row['ln_email']. "  |  " . $row['ln_date'] . "  |  " . $row['ln_week'] ;
          echo "<br />";

        }

Is this use of PHP OOP considered good practice even though it works nicely for now?

Answer 1

I would say no, it's no good use of OOP.

Areas for improvement:

Separate the db connection and query stuff. Separate the db result handling. Implementing a result object that is iterable will be a good idea. Not using the mysql extension and switching to mysqli is a very good idea. It will also give you an OOP interface to MySQL for free.

Probably aspects of escaping input inside SQL strings should be considered, but this is undecidable because no such code has been shown.

Answer 2

Will some future release break it?

Yes, because you are using the old and (now) deprecated mysql_* functions.

Code snippet for class lnemail_fetch

The name lnemail is not really a good name for a class, because when I look at it I have no idea what ln means. Also class names are often UpperCamelCased and methods camelCased.

Now to actually look at your code:

When looking at your class it is just a class and currently has nothing to do with OOP. What I would have done is make the $result property private, because currently your is simply some container for data. Also I would introduce another class which will be reponsible for accessing the data from the database (or whatever storage you have). I would also introduce another class to represent a single email and an factory class to build these mail objects. This would look something like the following:

// not sure whether inbox is the correct name, because I don't really have a good idea of what the class represents

class Inbox
{
    private $storage;

    private $mailFactory;

    public function __construct($storage, $mailFactory)
    {
        $this->storage     = $storage;
        $this->mailFactory = $mailFactory;
    }

    public function fetchAllMails()
    {
        $mailRecordset = $this->storage->fetchAll();

        $mails = array();
        foreach ($mailRecordset as $mailRecord) {
            $mails[] = $this->mailFactory->create($mailRecord);
        }

        return $mails;
    }
}

class InboxStorage
{
    private $dbConnection;

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

    public function fetchAll()
    {
        $stmt = $this->dbConnection->query('SELECT * FROM lnemail');

        return $stmt->fetchAll(\PDO::FETCH_ASSOC);
    }
}

class Email
{
    private $email;

    private $date;

    private $week;

    public function __construct($email, $date, $week)
    {
        $this->email = $email;
        $this->date = $date;
        $this->week = $week;
    }

    public function getEmail()
    {
        return $this->email;
    }

    public function getDate()
    {
        return $this->date;
    }

    public function getWeek()
    {
        return $this->week;
    }
}

class EmailFactory
{
    public function create($record)
    {
        return new Email($record['email'], $record['date'], $record['week']);
    }
}

And you can run it like following:

// initialize all the objects we are going to need
$emailFactory = new EmailFactory();
$dbConnection = new \PDO('mysql:dbname=dbtest;host=127.0.0.1;charset=utf8', 'user', 'pass');
$dbConnection->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
$dbConnection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$inboxStorage = new InboxStorage($dbConnection);
$inbox = new Inbox($inboxStorage, $mailFactory);

// run the code
foreach($inbox->fetchAllMails() as $email) {
    echo $mail->email . ' | ' . $mail->date . ' | ' . $mail->week . '<br>';
}

Answer 3

It's not really a true class because lnemail_fetch isn't an object. All you are doing is making a container and having to make the container merely to call a function that could be static and return the result rather than assign it.

A better class might include the newer mysqli rather than the dated mysql and work as follows. It makes the rows into objects with the columns being properties (variables;

<?php
class lnemail {
    public $ln_emai;
    public $ln_date;
    public $ln_week;

    public static function Fetch($dbhost,$dbuser,$dbpass,$dbname) {
      $db = new mysqli($dbhost, $dbuser, $dbpass,$dbname) or die(mysql_error());
      $query = "SELECT * FROM lnemail";
      $result = $db->query($query);
      $returnArr = array();
      while($obj = $result->fetch_object('lnemail') {
         $returnArr[] = $obj;     
      }
      return $returnArr;
    }
}

Then

 <?php
 require_once("./includes/LOheritage-config.php");
 require_once('./includes/heritage_classes.php');
 $lnemails = lnemail::Fetch($dbhost,$dbuser,$dbpass,$dbname);
 echo  "lnemail File display  <br />";
 foreach($obj as $lnemail) {
      echo $obj->ln_email. "  |  " . $obj->ln_date . "  |  " . $obj->ln_week;
      echo "<br />";
 }