From 022f9f309efb0a67c7cd3b232623658982c201d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacobo=20Aragunde=20P=C3=A9rez?= Date: Mon, 27 Sep 2021 13:39:07 +0200 Subject: [PATCH 1/6] Rewrite getter methods in PostgreSQLTemplateDAO to use PDO. This begins the work to replace pg_* functions for database access with the modern PDO interface. We first migrate PostgreSQLTemplateDAO::GetById and GetByUserId methods. We will progressively extend PDO usage to the rest of DAOs, refactoring code as needed along the way. --- .../dao/TemplateDAO/PostgreSQLTemplateDAO.php | 91 ++++++++++++++----- 1 file changed, 70 insertions(+), 21 deletions(-) diff --git a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php index d837a006d..8a00dce57 100644 --- a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php +++ b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php @@ -40,15 +40,49 @@ */ class PostgreSQLTemplateDAO extends TemplateDAO{ + /** The connection to DB. + * + * PDO object with an open connection to the database, initialized in the + * class constructor. + * + * @var resource + * @see __construct() + */ + protected PDO $pdo; + /** Template DAO for PostgreSQL constructor. * - * This is the constructor of the implementation for PostgreSQL of {@link TemplateDAO}, and it just calls its parent's constructor. + * This is the constructor of the implementation for PostgreSQL of + * {@link TemplateDAO}. It sets up everything for database connection, using + * the parameters read from {@link config.php} and saving the open + * connection in {@link $pdo}. + * Notice this DAO connects to the DB through PDO, unlike the rest of the + * application. * * @throws {@link DBConnectionErrorException} - * @see TemplateDAO::__construct() */ function __construct() { + // Call parent to initialize non-PDO database access, while we don't + // migrate all the methods here. parent::__construct(); + + // TODO: EXTRA_DB_CONNECTION_PARAMETERS used to expect pg_connect + // parameters, which were space-separated, but PDO requires semicolons + $connectionString = sprintf("pgsql:host=%s;port=%d;user=%s;dbname=%s;password=%s;%s", + ConfigurationParametersManager::getParameter('DB_HOST'), + ConfigurationParametersManager::getParameter('DB_PORT'), + ConfigurationParametersManager::getParameter('DB_USER'), + ConfigurationParametersManager::getParameter('DB_NAME'), + ConfigurationParametersManager::getParameter('DB_PASSWORD'), + ConfigurationParametersManager::getParameter('EXTRA_DB_CONNECTION_PARAMETERS')); + + try { + $this->pdo = new PDO($connectionString); + $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); + } catch (PDOException $e) { + error_log('Connection failed: ' . $e->getMessage()); + throw new DBConnectionErrorException($connectionString); + } } /** Template value object constructor for PostgreSQL. @@ -65,15 +99,8 @@ protected function setValues($row) { $templateVO->setId($row['id']); $templateVO->setName($row['name']); $templateVO->setStory($row['story']); - $templateVO->setStory($row['story']); - if (strtolower($row['telework']) == "t") - $templateVO->setTelework(True); - elseif (strtolower($row['telework']) == "f") - $templateVO->setTelework(False); - if (strtolower($row['onsite']) == "t") - $templateVO->setOnsite(True); - elseif (strtolower($row['onsite']) == "f") - $templateVO->setOnsite(False); + $templateVO->setTelework($row['telework']); + $templateVO->setOnsite($row['onsite']); $templateVO->setText($row['text']); $templateVO->setTtype($row['ttype']); $templateVO->setUserId($row['usrid']); @@ -97,9 +124,19 @@ protected function setValues($row) { public function getById($templateId) { if (!is_numeric($templateId)) throw new SQLIncorrectTypeException($templateId); - $sql = "SELECT * FROM template WHERE id=".$templateId; - $result = $this->execute($sql); - return $result[0] ?? NULL; + try { + $statement = $this->pdo->prepare( + "SELECT * FROM template WHERE id=:id"); + $statement->execute([ + ':id' => $templateId + ]); + $row = $statement->fetch(PDO::FETCH_ASSOC); + return $row ? $this->setValues($row) : NULL; + + } catch (PDOException $e) { + error_log('Query failed: ' . $e->getMessage()); + throw new SQLQueryErrorException($e->getMessage()); + } } /** Templates retriever by User id for PostgreSQL. @@ -115,9 +152,23 @@ public function getById($templateId) { public function getByUserId($userId) { if (!is_numeric($userId)) throw new SQLIncorrectTypeException($userId); - $sql = "SELECT * FROM template WHERE usrid=$userId"; - $result = $this->execute($sql); - return $result; + try { + $statement = $this->pdo->prepare( + "SELECT * FROM template WHERE usrid=:usrid"); + $statement->execute([ + ':usrid' => $userId + ]); + + $VO = array(); + foreach($statement->fetchAll(PDO::FETCH_ASSOC) as $row) { + $VO[] = $this->setValues($row); + } + return $VO; + + } catch (PDOException $e) { + error_log('Query failed: ' . $e->getMessage()); + throw new SQLQueryErrorException($e->getMessage()); + } } /** Template creator for PostgreSQL. @@ -226,8 +277,6 @@ public function batchDelete($templates){ * @throws SQLQueryErrorException */ public function getUserTemplates($userId) { - $sql = "SELECT * FROM template where usrid=$userId"; - - return $this->execute($sql); + return $this->getByUserId($userId); } -} \ No newline at end of file +} From 0b87033bac86ee2975622901391f638fc4fdc06f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacobo=20Aragunde=20P=C3=A9rez?= Date: Wed, 10 Nov 2021 12:45:10 +0100 Subject: [PATCH 2/6] Refactor common code into runSelectQuery. Refactor common code in PostgreSQLTemplateDAO::GetById and GetByUserId to the function runSelectQuery. This leaves those functions looking similar to what they looked like before, when they used the execute function from the non-PDO code paths. --- .../dao/TemplateDAO/PostgreSQLTemplateDAO.php | 53 ++++++++----------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php index 8a00dce57..929963d34 100644 --- a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php +++ b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php @@ -112,6 +112,21 @@ protected function setValues($row) { return $templateVO; } + protected function runSelectQuery(string $statement, array $data) { + try { + $statement = $this->pdo->prepare($statement); + $statement->execute($data); + $VOs = array(); + foreach($statement->fetchAll(PDO::FETCH_ASSOC) as $row) { + $VOs[] = $this->setValues($row); + } + return $VOs; + } catch (PDOException $e) { + error_log('Query failed: ' . $e->getMessage()); + throw new SQLQueryErrorException($e->getMessage()); + } + } + /** Template retriever by id for PostgreSQL. * * This function retrieves the row from Template table with the id $templateId and creates a {@link TemplateVO} with its data. @@ -124,19 +139,10 @@ protected function setValues($row) { public function getById($templateId) { if (!is_numeric($templateId)) throw new SQLIncorrectTypeException($templateId); - try { - $statement = $this->pdo->prepare( - "SELECT * FROM template WHERE id=:id"); - $statement->execute([ - ':id' => $templateId - ]); - $row = $statement->fetch(PDO::FETCH_ASSOC); - return $row ? $this->setValues($row) : NULL; - - } catch (PDOException $e) { - error_log('Query failed: ' . $e->getMessage()); - throw new SQLQueryErrorException($e->getMessage()); - } + $result = $this->runSelectQuery( + "SELECT * FROM template WHERE id=:id", + [':id' => $templateId]); + return $result[0] ?? NULL; } /** Templates retriever by User id for PostgreSQL. @@ -152,23 +158,10 @@ public function getById($templateId) { public function getByUserId($userId) { if (!is_numeric($userId)) throw new SQLIncorrectTypeException($userId); - try { - $statement = $this->pdo->prepare( - "SELECT * FROM template WHERE usrid=:usrid"); - $statement->execute([ - ':usrid' => $userId - ]); - - $VO = array(); - foreach($statement->fetchAll(PDO::FETCH_ASSOC) as $row) { - $VO[] = $this->setValues($row); - } - return $VO; - - } catch (PDOException $e) { - error_log('Query failed: ' . $e->getMessage()); - throw new SQLQueryErrorException($e->getMessage()); - } + $result = $this->runSelectQuery( + "SELECT * FROM template WHERE usrid=:usrid", + [':usrid' => $userId]); + return $result; } /** Template creator for PostgreSQL. From a4ea9bd50f5d39fb12f9eb2350785a0a3668f5a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacobo=20Aragunde=20P=C3=A9rez?= Date: Tue, 23 Nov 2021 17:12:14 +0100 Subject: [PATCH 3/6] Port PostgreSQLTemplateDAO::create() to PDO. Rewrite method to use PDO prepared statements, typed binding and last id retrieval. --- .../dao/TemplateDAO/PostgreSQLTemplateDAO.php | 48 ++++++++++--------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php index 929963d34..144223d8f 100644 --- a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php +++ b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php @@ -176,30 +176,34 @@ public function getByUserId($userId) { public function create(TemplateVO $templateVO) { $affectedRows = 0; - $sql = "INSERT INTO template (name, story, telework, onsite, text, ttype, usrid, projectid, init_time, end_time, task_storyid) VALUES(" . - DBPostgres::checkStringNull($templateVO->getName()) . ", " . - DBPostgres::checkStringNull($templateVO->getStory()) . ", " . - DBPostgres::boolToString($templateVO->isTelework()) . ", " . - DBPostgres::boolToString($templateVO->isOnsite()) . ", " . - DBPostgres::checkStringNull($templateVO->getText()) . ", " . - DBPostgres::checkStringNull($templateVO->getTtype()) . ", " . - DBPostgres::checkNull($templateVO->getUserId()) . ", " . - DBPostgres::checkNull($templateVO->getProjectId()) . ", " . - DBPostgres::checkNull($templateVO->getInitTime()) . ", " . - DBPostgres::checkNull($templateVO->getEndTime()) . ", " . - DBPostgres::checkNull($templateVO->getTaskStoryId()) .")"; - - $res = pg_query($this->connect, $sql); - - if ($res == NULL) - throw new SQLQueryErrorException(pg_last_error()); - - $templateVO->setId(DBPostgres::getId($this->connect, "template_id_seq")); - - $affectedRows = pg_affected_rows($res); + $sql = "INSERT INTO template (name, story, telework, onsite, text, " . + "ttype, usrid, projectid, init_time, end_time, task_storyid) " . + "VALUES(:name, :story, :telework, :onsite, :text, :ttype, " . + ":usrid, :projectid, :init_time, :end_time, :task_storyid)"; + try { + $statement = $this->pdo->prepare($sql); + $statement->bindValue(":name", $templateVO->getName(), PDO::PARAM_STR); + $statement->bindValue(":story", $templateVO->getStory(), PDO::PARAM_STR); + $statement->bindValue(":telework", $templateVO->isTelework(), PDO::PARAM_BOOL); + $statement->bindValue(":onsite", $templateVO->isOnsite(), PDO::PARAM_BOOL); + $statement->bindValue(":text", $templateVO->getText(), PDO::PARAM_STR); + $statement->bindValue(":ttype", $templateVO->getTtype(), PDO::PARAM_STR); + $statement->bindValue(":usrid", $templateVO->getUserId(), PDO::PARAM_INT); + $statement->bindValue(":projectid", $templateVO->getProjectId(), PDO::PARAM_INT); + $statement->bindValue(":init_time", $templateVO->getInitTime(), PDO::PARAM_INT); + $statement->bindValue(":end_time", $templateVO->getEndTime(), PDO::PARAM_INT); + $statement->bindValue(":task_storyid", $templateVO->getTaskStoryId(), PDO::PARAM_INT); + $statement->execute(); + + $templateVO->setId($this->pdo->lastInsertId('template_id_seq')); + + $affectedRows = $statement->rowCount(); + } catch (PDOException $e) { + error_log('Query failed: ' . $e->getMessage()); + throw new SQLQueryErrorException($e->getMessage()); + } return $affectedRows; - } /** From 01afc795aca2d2abd0bf35bf243dfcacadaf1fef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacobo=20Aragunde=20P=C3=A9rez?= Date: Tue, 23 Nov 2021 17:27:07 +0100 Subject: [PATCH 4/6] Port PostgreSQLTemplateDAO::delete() to PDO. The check that looked up the template in the database before removing it was deleted because it was useless. The query will run, returning zero affected rows, if the template doesn't exist. From the outside, there is no change of behavior in the delete operation. --- .../dao/TemplateDAO/PostgreSQLTemplateDAO.php | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php index 144223d8f..5bd2e9f42 100644 --- a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php +++ b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php @@ -234,20 +234,16 @@ public function batchCreate($templates) { public function delete(TemplateVO $templateVO) { $affectedRows = 0; - // Check for a task ID. - if($templateVO->getId() >= 0) { - $currTaskVO = $this->getById($templateVO->getId()); - } - - // Otherwise delete a task. - if($currTaskVO) { - $sql = "DELETE FROM template WHERE id=".$currTaskVO->getId(); + $sql = "DELETE FROM template WHERE id=:id"; - $res = pg_query($this->connect, $sql); - if ($res == NULL) throw new SQLQueryErrorException(pg_last_error()); - $affectedRows = pg_affected_rows($res); + try { + $statement = $this->pdo->prepare($sql); + $statement->execute([':id' => $templateVO->getId()]); + $affectedRows = $statement->rowCount(); + } catch (PDOException $e) { + error_log('Query failed: ' . $e->getMessage()); + throw new SQLQueryErrorException($e->getMessage()); } - return $affectedRows; } From f74cf305a353ff49e054e84e21f5c915f75520f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacobo=20Aragunde=20P=C3=A9rez?= Date: Tue, 23 Nov 2021 17:49:09 +0100 Subject: [PATCH 5/6] Docs: Add PDO to system requirements. The missing PDO package in Debian is not a typo, it seems to be provided by another package. I'm removing the RHEL line because I don't have a RHEL system where I can test things, and I don't want to provide unverified commands. --- docs/admin/installation.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/admin/installation.rst b/docs/admin/installation.rst index 33665417e..3cd6b34e3 100644 --- a/docs/admin/installation.rst +++ b/docs/admin/installation.rst @@ -12,7 +12,7 @@ To install PhpReport in your system, you will need the following software: * PHP 7.3 or higher - * Support for PostgreSQL + * Support for PDO and PostgreSQL * Web server (tested with Apache 2.x) @@ -23,11 +23,11 @@ Installing dependencies on selected GNU/Linux distros Run the following command with root privileges: -* Debian, Ubuntu: ``apt-get install postgresql apache2 php php-pgsql php-xml`` +* Ubuntu: ``apt install postgresql apache2 php php-pgsql php-xml php-pdo`` -* Fedora: ``dnf install postgresql-server httpd php php-pgsql php-xml`` +* Debian: ``apt install postgresql apache2 php php-pgsql php-xml`` -* RHEL: ``yum install postgresql-server httpd php php-pgsql php-xml`` +* Fedora: ``dnf install postgresql-server httpd php php-pgsql php-xml php-pdo`` Install composer to manage the project dependencies. Follow the official docs for the instructions: https://getcomposer.org/doc/00-intro.md#installation-linux-unix-macos From 9460159d1132b3636f504e73655334556a1a1fbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacobo=20Aragunde=20P=C3=A9rez?= Date: Wed, 24 Nov 2021 10:51:56 +0100 Subject: [PATCH 6/6] Use PDO::FETCH_CLASS to retrieve TemplateVO objects. We need to modify the properties in the VO class to match column names. Besides, the TemplateVO::setValues() function cannot be removed because it's declared abstract in a parent class. This should be temporary, until all DAOs use PDO. --- .../dao/TemplateDAO/PostgreSQLTemplateDAO.php | 33 +++----------- model/vo/TemplateVO.php | 45 ++++++++++--------- 2 files changed, 29 insertions(+), 49 deletions(-) diff --git a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php index 5bd2e9f42..73c7bd7c0 100644 --- a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php +++ b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php @@ -85,42 +85,19 @@ function __construct() { } } - /** Template value object constructor for PostgreSQL. - * - * This function creates a new {@link TemplateVO} with data retrieved from database. - * - * @param array $row an array with the Task values from a row. - * @return TemplateVO a {@link TemplateVO} with its properties set to the values from $row. - * @see TemplateVO + /** + * This method is declared to fulfill TemplateVO as non-abstract, but it should not be used. + * PDO::FETCH_CLASS now takes care of transforming DB rows into VO objects. */ protected function setValues($row) { - $templateVO = new TemplateVO(); - - $templateVO->setId($row['id']); - $templateVO->setName($row['name']); - $templateVO->setStory($row['story']); - $templateVO->setTelework($row['telework']); - $templateVO->setOnsite($row['onsite']); - $templateVO->setText($row['text']); - $templateVO->setTtype($row['ttype']); - $templateVO->setUserId($row['usrid']); - $templateVO->setProjectId($row['projectid']); - $templateVO->setTaskStoryId($row['task_storyid']); - $templateVO->setInitTime($row['init_time']); - $templateVO->setEndTime($row['end_time']); - - return $templateVO; + error_log("Unused TemplateVO::setValues() called"); } protected function runSelectQuery(string $statement, array $data) { try { $statement = $this->pdo->prepare($statement); $statement->execute($data); - $VOs = array(); - foreach($statement->fetchAll(PDO::FETCH_ASSOC) as $row) { - $VOs[] = $this->setValues($row); - } - return $VOs; + return $statement->fetchAll(PDO::FETCH_CLASS, 'TemplateVO'); } catch (PDOException $e) { error_log('Query failed: ' . $e->getMessage()); throw new SQLQueryErrorException($e->getMessage()); diff --git a/model/vo/TemplateVO.php b/model/vo/TemplateVO.php index 9fefcbe4b..00d75d0be 100644 --- a/model/vo/TemplateVO.php +++ b/model/vo/TemplateVO.php @@ -33,6 +33,9 @@ * * This class just stores Templates data. * + * NOTICE: properties must match column names in the DB, for PDO::FETCH_CLASS + * to work properly. + * * @property int $id database internal identifier. * @property string $name name of the template * @property string $story story of this Task. @@ -40,9 +43,9 @@ * @property boolean $onsite says if this Task was made onsite. * @property string $text text describing this Task. * @property string $ttype type of this Task. - * @property int $userId database internal identifier of the associated User. - * @property int $projectId database internal identifier of the associated Project. - * @property int $taskStoryId database internal identifier of the associated Task Story. + * @property int $usrid database internal identifier of the associated User. + * @property int $projectid database internal identifier of the associated Project. + * @property int $task_storyid database internal identifier of the associated Task Story. */ class TemplateVO { @@ -56,11 +59,11 @@ class TemplateVO { protected $onsite = NULL; protected $text = NULL; protected $ttype = NULL; - protected $userId = NULL; - protected $projectId = NULL; - protected $taskStoryId = NULL; - protected $initTime = NULL; - protected $endTime = NULL; + protected $usrid = NULL; + protected $projectid = NULL; + protected $task_storyid = NULL; + protected $init_time = NULL; + protected $end_time = NULL; protected $initTimeRaw = NULL; protected $endTimeRaw = NULL; @@ -166,50 +169,50 @@ public function setTtype($ttype) { * @return int */ public function getUserId() { - return $this->userId; + return $this->usrid; } /** * @param int $userId */ public function setUserId($userId) { - $this->userId = $userId; + $this->usrid = $userId; } /** * @return int */ public function getProjectId() { - return $this->projectId; + return $this->projectid; } /** * @param int $projectId */ public function setProjectId($projectId) { - $this->projectId = $projectId; + $this->projectid = $projectId; } /** * @return int */ public function getTaskStoryId() { - return $this->taskStoryId; + return $this->task_storyid; } /** * @param int $taskStoryId */ public function setTaskStoryId($taskStoryId) { - $this->taskStoryId = $taskStoryId; + $this->task_storyid = $taskStoryId; } public function getInitTime(): ?int { - return $this->initTime; + return $this->init_time; } public function setInitTime(?int $initTime) { - $this->initTime = $initTime; + $this->init_time = $initTime; } public function setInitTimeRaw(?string $initTime) { @@ -217,11 +220,11 @@ public function setInitTimeRaw(?string $initTime) { } public function getEndTime(): ?int { - return $this->endTime; + return $this->end_time; } public function setEndTime(?int $endTime) { - $this->endTime = $endTime; + $this->end_time = $endTime; } public function setEndTimeRaw(?string $endTime) { @@ -241,9 +244,9 @@ public function toXml() { $string .= "{$this->onsite}"; $string .= "{$this->text}"; $string .= "{$this->ttype}"; - $string .= "{$this->userId}"; - $string .= "{$this->projectId}"; - $string .= "{$this->taskStoryId}"; + $string .= "{$this->usrid}"; + $string .= "{$this->projectid}"; + $string .= "{$this->task_storyid}"; $string .= "{$this->initTimeRaw}"; $string .= "{$this->endTimeRaw}"; $string .= "";