New to OOP and PDO

[AndrewPHP]

After writing procedural style PHP for many years I've decided to give OOP & PDO a go (and I'm hating it already).

 

I have written a simple function which validate a flag value, and another function which is later used to get the description of that value.

 

Am I on the right lines here and can any improvements be made?

<?php

// Connect to MySQL
$dsn = 'mysql:dbname=dev_pof;host=127.0.0.1';
$username = 'dev';
$password = 'dev.dev';

$pdo = new PDO($dsn, $username, $password,[PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);

// Function to validate the flag value
function CheckFlagValue($pdo, $FlagName, $FlagValue)
{
    $sql  = "SELECT 1 FROM flags WHERE flagname = ? AND flagvalue = ? LIMIT 1";
    $stmt = $pdo->prepare($sql);
    $stmt->execute([$FlagName, $FlagValue]);
    return $stmt->fetchColumn();
}

// Function to get the flag description
function GetFlagDescription($pdo, $FlagName, $FlagValue)
{
    $sql  = "SELECT flagdescription FROM flags WHERE flagname = ? AND flagvalue = ? LIMIT 1";
    $stmt = $pdo->prepare($sql);
    $stmt->execute([$FlagName, $FlagValue]);
    return $stmt->fetchColumn();
}

// Example of using the CheckFlagValue function
if (CheckFlagValue($pdo, 'intent', 2)) {
    echo "valid";
} else {
    echo "invalid";
}

?>

[requinix]

So, this isn't really OOP... It's still procedural code like before, except you're using functions instead of having to copy/paste code around.

 

An OOP version would be like

class Flag {

	private $FlagName = "";

	private $pdo = null;

	public function __construct($pdo, $FlagName) {
		$this->FlagName = $FlagName;
		$this->pdo = $pdo;
	}

	public function CheckValue($FlagValue) {
		$sql = "SELECT 1 FROM flags WHERE flagname = ? AND flagvalue = ? LIMIT 1";
		$stmt = $this->pdo->prepare($sql);
		$stmt->execute([$this->FlagName, $FlagValue]);
		return $stmt->fetchColumn();
	}

	public function GetDescription($FlagValue) {
		$sql = "SELECT flagdescription FROM flags WHERE flagname = ? AND flagvalue = ? LIMIT 1";
		$stmt = $this->pdo->prepare($sql);
		$stmt->execute([$this->FlagName, $FlagValue]);
		return $stmt->fetchColumn();
	}

}

$pdo = new PDO($dsn, $username, $password, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]);

$intent = new Flag($pdo, 'intent');
if ($intent->CheckValue(2)) {
	echo "valid";
} else {
	echo "invalid";
}

1. It could be that "a flag" is not just a name but a name/value pair, in which case you'd set up the name and value with the Flag class and then not have to pass the value into the CheckValue and GetDescription methods.

2. If you did that name/value pair thing, better than querying the database in each method would be to simply load up all data when the class gets constructed - then CheckValue and GetDescription could just return values already available.

[Jacques1]

The PDO code can also be improved a bit:

• Always set PDO::ATTR_EMULATE_PREPARES to false. Otherwise all prepared statements are silently replaced with a kind of client-side auto-escaping, which is a lot less secure.
• You should explicitly define the desired character encoding in the DSN string (e. g. charset=utf8mb4). Otherwise MySQL will fall back to a default encoding and may garble non-ASCII characters.
• Also consider setting the default fetch mode.
• PDO supports named parameters in prepared statements. Use them! It's a lot more readable than a bunch of question marks.
• When you connect to 127.0.0.1, you force MySQL to use the TCP protocol. It's generally more efficient to use a UNIX socket by connecting to localhost.

<?php

const DB_HOST = 'localhost';
const DB_USER = '...';
const DB_PASSWORD = '...';
const DB_NAME = '...';
const DB_CHARSET = 'utf8mb4';    // just an example; choose whatever you need



$dSN = 'mysql:host='.DB_HOST.';dbname='.DB_NAME.';charset='.DB_CHARSET;

$databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, [
    PDO::ATTR_EMULATE_PREPARES => false,
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
]);

[AndrewPHP]

Thanks for that, it looks like I should have stuck to the procedural side of things as this looks incredibly more complex.

 

I also need to create some other functions too but unsure how they would break down by class.

 

CheckIfEnabled - This function checks in the DB if this script is allowed to be run.

CheckIfTime - This function checks in the DB if this script is allowed to be run at this current minute.

CheckMessagesSent - This function checks in the DB to see how many messages have been sent already today

CheckMessageLimit - This function checks in the DB to see how many messages are allowed to be sent.

 

 

Would I create a new class for each function?

[Jacques1]

No, because then all you get is your old procedural code with a lot of useless OOP overhead.

 

OOP is about objects, not functions. Objects are actors which have attributes and can perform tasks. In your case, an obvious actor would be the current script: It has a path (or some other unique identifier) and a database connection, and it can tell you whether you're allowed to run it.

 

For example:

<?php

class RestrictedScript
{
    private $path;

    private $database;

    public function __construct($path, PDO $database)
    {
        $this->path = $path;
        $this->database = $database;
    }

    public function isEnabled()
    {
        // query the database
    }

    public function isRunnable()
    {
        // query the database
    }
}

<?php

$thisScript = new RestrictedScript(__FILE__, $database);

if (!$thisScript->isEnabled())
{
    echo 'Script is not enabled.';
    exit;
}

if (!$thisScript->isRunnable())
{
    echo 'Script may not be executed right now. Please try again later.';
    exit;
}

You should, of course, choose a more meaningful name than “RestrictedScript”.

[AndrewPHP]

Great stuff, I think I'm slowly taking it in (I hope), so how's this...

<?php

class ScriptControl
{

  private $pdo;
  private $ScriptName;
  
  public function __construct($pdo, $ScriptName)
  {
    $this->pdo = $pdo;
    $this->ScriptName = $ScriptName;
  }

  public function IsEnabled();
  {
    $sql = "SELECT 1 FROM config WHERE config_name = ? AND config_value = 1 LIMIT 1";
		$stmt = $this->pdo->prepare($sql);
		$stmt->execute([$this->ScriptName]);
		return $stmt->fetchColumn();
  }

  public function IsRunnable();
  {
    date_default_timezone_set("Europe/London");
    $time = date("Hi");
    $sql = "SELECT 1 FROM config WHERE config_name = ? AND config_value = ? LIMIT 1";
		$stmt = $this->pdo->prepare($sql);
		$stmt->execute([$this->ScriptName, $this->$time]);
		return $stmt->fetchColumn();
  }
  
}

?>

<?php

include_once('PDO.class.php');
include_once('ScriptControl.class.php');

$ThisScript = new ScriptControl('SendMessage');

if ( (!$ThisScript->isEnabled()) && (!isset($_POST['bypass'])) )
{
    echo "<p>This script is not enabled</p>\n";
    die();
}

if ( (!$ThisScript->isRunable()) && (!isset($_POST['bypass'])) )
{
  echo "<p>This script is not scheduled to run</p>\n";
  die();
}

?>

[Jacques1]

The OOP part is OK (except that you're not passing the PDO instance to the ScriptControl constructor in your example code).

 

The database layout looks odd. What's the type of config_value? Why does the column simultaneously contain integers(?) and time values? Also, you can get the current time in MySQL itself using CURTIME(). There's no need to do that in PHP (unless you somehow want different timezones in MySQL and PHP).