Function to insert a tag into a database

Question

I'm developing a MVC structured RESTful API project (thanks to the help of Mārtiņš Tereško for explaining it).

Almost all of my functions look similar to this:

 /**
 * Insert Tag
 */
 public function insertTag(Tag\Tag $tag)
 { 
 try{
 //Names
 $name = new Common\Name();
 //Insert into main table
 $sql = "INSERT INTO tag VALUES(null)";
 $statement = $this->connection->prepare($sql);
 $statement->execute();
 //Get main table id
 $tagId = $this->connection->lastInsertId();
 //Insert into audit tag table
 $sql = "INSERT INTO tag_audit(behaviour,action_peformed,state,tag_id) VALUES(:behaviour,'insert','P',:tag_id)";
 $statement = $this->connection->prepare($sql);
 $statement->bindValue(':behaviour',$tag->getBehaviour(),PDO::PARAM_STR);
 $statement->bindValue(':tag_id',$tagId,PDO::PARAM_INT);
 $statement->execute();
 //Insert into names
 $name->setNames($tag->getNames()[0]);
 $values = $name->getBulkValue(
 $name->getValue(),
 ["'insert'","'P'",$tagId]
 ); 
 $sql = "INSERT INTO tag_name_audit(name,language,action_peformed,state,tag) VALUES".$values;
 $statement = $this->connection->prepare($sql);
 $statement->execute();
 $response = ['status'=>200,'Message'=>'Successfully inserted.'];
 }catch(\Exception $e){
 $sql = "DELETE FROM tag WHERE id_tag = :id_tag";
 $statement = $this->connection->prepare($sql);
 $statement->bindValue(":id_tag",$tagId,PDO::PARAM_INT);
 $statement->execute();
 $sql = "DELETE FROM tag_audit WHERE tag_id = :id_tag";
 $statement = $this->connection->prepare($sql);
 $statement->bindValue(":id_tag",$tagId,PDO::PARAM_INT);
 $statement->execute();
 $response = ['status'=>409,'Message'=>'Error '.$e->getCode()];
 }
 $tag->setResponse($response);
 }

I don't like how much code there is and i tought of having some mapper helper classes which will handle at least a portion of it.But still i don't think that it's the right solution.

Also since i parse the bulk value over an array i know that it's not secure ....how could i solve that part?

So the question is how could i improve it?

Answer 1

• Use a transaction, so you won't need all the manual delete queries
• Use shorter PDO syntax.
• Log errors
• Use prepared statements

In the end it should be like

public function insertTag(Tag\Tag $tag)
{ 
 try{
 //Names
 $name = new Common\Name();
 $pdo->beginTransaction();
 //Insert into main table
 $this->connection->query("INSERT INTO tag VALUES(null)");
 //Get main table id
 $tagId = $this->connection->lastInsertId();
 //Insert into audit tag table
 $sql = "INSERT INTO tag_audit(behaviour,action_peformed,state,tag_id) VALUES(?,'insert','P',?)";
 $this->connection->prepare($sql)->execute([$tag->getBehaviour(), $tagId]);
 //Insert into names
 // you should be using prepared statements here
 $sql = "INSERT INTO tag_name_audit(name,language,action_peformed,state,tag) VALUES (?,?,?,?,?)";
 $statement = $this->connection->prepare($sql);
 foreach (whatever function supplies the name as $name) {
 $statement->execute([$name]);
 }
 $response = ['status'=>200,'Message'=>'Successfully inserted.'];
 $pdo->commit();
 }catch(\Exception $e){
 $pdo->rollback();
 error_log($e);
 $response = ['status'=>409,'Message'=>'Error '.$e->getCode()];
 }
 $tag->setResponse($response);
}

• DaAmidza: Thanks a lot for the transactions it will help me a lot!!!
• DaAmidza: Isn't bulk import better than to execute the same function several times?
• Your Common Sense: Nope it's no better. Prepared statements are optimized for multiple inserts. just prepare once and then execute in the loop
• DaAmidza: Didn't know that!!I taught that bulk import is better.Thanks a lot sir :D
• Your Common Sense: I'd say both methods are good. You can use either, but given prepared statements are safer, it makes the choice obvious.