[#513] Port Template DAO to use PDO.

jaragunde · web-flow · commit 09a5efa61cd5 · 2021-11-25T09:58:09.000+01:00
Merge pull request #534 from Igalia/i513-pdo-for-template-table
diff --git 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
diff --git a/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php b/model/dao/TemplateDAO/PostgreSQLTemplateDAO.php
@@ -40,49 +40,68 @@
  */
 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 <i>{@link config.php}</i> and saving the open
+     * connection in <var>{@link $pdo}</var>.
+     * 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.
-     *
-     * 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 <var>$row</var>.
-     * @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->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->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);
+            return $statement->fetchAll(PDO::FETCH_CLASS, 'TemplateVO');
+        } catch (PDOException $e) {
+            error_log('Query failed: ' . $e->getMessage());
+            throw new SQLQueryErrorException($e->getMessage());
+        }
     }
 
     /** Template retriever by id for PostgreSQL.
@@ -97,8 +116,9 @@ 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);
+        $result = $this->runSelectQuery(
+            "SELECT * FROM template WHERE id=:id",
+            [':id' => $templateId]);
         return $result[0] ?? NULL;
     }
 
@@ -115,8 +135,9 @@ 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);
+        $result = $this->runSelectQuery(
+            "SELECT * FROM template WHERE usrid=:usrid",
+            [':usrid' => $userId]);
         return $result;
     }
 
@@ -132,30 +153,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;
-
     }
 
     /**
@@ -186,20 +211,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;
     }
 
@@ -226,8 +247,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);
     }
-}
+}
diff --git a/model/vo/TemplateVO.php b/model/vo/TemplateVO.php
@@ -33,16 +33,19 @@
  *
  *  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.
  *  @property boolean $telework says if this Task was made by telework.
  *  @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,62 +169,62 @@ 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) {
         $this->initTimeRaw = $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 .= "<onsite>{$this->onsite}</onsite>";
         $string .= "<text>{$this->text}</text>";
         $string .= "<ttype>{$this->ttype}</ttype>";
-        $string .= "<userId>{$this->userId}</userId>";
-        $string .= "<projectId>{$this->projectId}</projectId>";
-        $string .= "<taskStoryId>{$this->taskStoryId}</taskStoryId>";
+        $string .= "<userId>{$this->usrid}</userId>";
+        $string .= "<projectId>{$this->projectid}</projectId>";
+        $string .= "<taskStoryId>{$this->task_storyid}</taskStoryId>";
         $string .= "<initTime>{$this->initTimeRaw}</initTime>";
         $string .= "<endTime>{$this->endTimeRaw}</endTime>";
         $string .= "</template>";
