OOP - Contact Form PHP Backend - Procedural to OOP - Dynamic MySQL Prepared Statement PHP

Question

I just converted procedural code to OOP code. Is there any performance or security issue with this code or what should I consider further?

interface

interface IContactform {
    
    public function validate();
    public  function addcontact();
    public function  getcontact();
}

Implementing class

class customercontact implements IContactform {
    
    protected $dbconnection;
    protected $customerDbTable;
    protected $formvalues;
    protected $statusTracker=[];
    protected $collectedFormData=[];    
    protected $customerformrequestingKeys=["customername","customeremail","customerphone","messageBox"];//requestind form attribute names
    protected $processedContactDetails=[];
    protected $databasecolomns=["c_name","c_email","c_contact","c_message"];

    public function __construct($dbcon, $formvalues, $tbtable){
        //var_dump($tbtable);
        $this->customerDbTable=$tbtable;
        $this->formvalues=$formvalues;
        $this->dbconnection=$dbcon;
    }
    
    public function addcontact() {
        if($this->validate()){            
            if($this->preparesqlstatemen()){
                array_push($this->statusTracker,["success"=>"Received and we will contact you soon"]);
                return $this->statusTracker;                
            }            
            array_push($this->statusTracker,["error"=>"Something gone wrong"]);
                return $this->statusTracker;                
        }else{
            //var_dump($this->statusTracker);
            return $this->statusTracker;
        }        
    }

    public function validate() {
        foreach ($this->formvalues as $key => $value){
            $santizedinput=$this->sanitizeAndEmptyValueValidate($key,$value);                   
            if($santizedinput ){
                for ($i=0; $i < count($this->customerformrequestingKeys);$i++){
                    if($key==$this->customerformrequestingKeys[$i]){
                        array_push($this->collectedFormData,[$this->customerformrequestingKeys[$i]=>$santizedinput]);
                    }                    
                }                
            }
            else
            {              
               return false;                
            }  
        }
        return true;//return true if all values are validated and filled
        
    }
    
    //sanitize and empty value check userinput
    public function sanitizeAndEmptyValueValidate($key,$value){
         $valuea=(filter_var($value,FILTER_SANITIZE_STRING)); 
        if(!empty($valuea)){
            return $valuea;
        }else{
            array_push($this->statusTracker,["error"=>"".$key." is Empty"]);
            return false;
        }        
    }
    
    //prepare sqlinsert statement dynamically
    private function preparesqlstatemen(){        
        $preparedStatementPlaceHolders=[];
        $sqlexpectedTypes=[];
        $sqlexpectedValues1=[];
        $sqlexpectedValues2=[];
        
        for($k=0; $k<COUNT($this->collectedFormData); $k++){
            array_push($preparedStatementPlaceHolders,"?");
            array_push($sqlexpectedTypes,"s");
            array_push($sqlexpectedValues1,implode(",",$this->collectedFormData[$k]));
        }       
       
        for($k=0; $k<COUNT($sqlexpectedValues1); $k++){ 
            $rt=($sqlexpectedValues1[$k]);
            array_push($sqlexpectedValues2,''.$rt.'');            
        }
        
        $sql= $this->dbconnection->conn->prepare("INSERT INTO ".$this->customerDbTable."(".implode(",",$this->databasecolomns).") VALUES(".implode(",",$preparedStatementPlaceHolders).")");       
        $da=implode('',$sqlexpectedTypes);
        $sql->bind_param($da,...$sqlexpectedValues2);

       if($sql->execute()=== TRUE){           
           return true;
       }else{
           return $sql->error;
       }
  
    }
    
    public function getcontact() {
        
    }

}

DB Class

class dbclass {
    //put your code here
    
    private $servername = "localhost";
    private $username = "root";
    private $password = "";
    private $dbname = "contact_form";
    
    public function __construct(){
        $this->conn=new mysqli($this->servername, $this->username, $this->password, $this->dbname);
        if ($this->conn->connect_error) {
            die("Connection failed: " . ($this->conn->connect_error));
        }
        //return $this->conn;
    }
}

index.php

<?php 

    include_once 'customercontact.php';
    include_once 'dbclass.php';

?>

<!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8">
        <title></title>
    </head>
    <body>
        <?php
            if ($_SERVER['REQUEST_METHOD'] === 'POST') {
                    $formvalues=json_decode(file_get_contents('php://input'), true);    

                    $dbcon=new dbclass(); //passing db connection from db class
                    $tbtable="contact_master"; //table name     
                    $contcatClass=new customercontact($dbcon, $formvalues[0], $tbtable);    
                    
                    echo json_encode($contcatClass->addcontact());

            }
        ?>
    </body>
</html>

Answer 1

• Rather than passing the db connection into your constructor, I think it would be cleaner for your constructor to call a method in your DB class which first checks for an existing connection and only creates one if not found. This connection could be fetched by any other method if/when needed. It is best practice to create just one db connection and keep reusing it. Furthermore, not passing in the connection as a parameter will reduce the line length of the class instantiating code -- this is what I mean by "cleaner".

• Please do not print the actual mysql error directly to the user. While testing this is not so bad, but in production this is a major security vulnerability.

• As a matter of code styling, please adopt all recommendations from the PSR-12 guidelines. You have inconsistent spacing at language constructs, concatenations, assignments, array elements, and returns.

• Please only use array_push() when you need to add two or more indexed elements to an array. In all other scenarios, square brace syntax is less verbose, more performant, and I find it easier to read.

• Instead of using for loops, use foreach(), this avoids declaring and incrementing a counter.

• !empty() is not the most appropriate technique when "truthy" checking a variable which is guaranteed to exist. Because $valuea (a poor variable name, by the way) is guaranteed to exist, if(!empty($valuea)){ should become if ($valuea) { -- they are equivalent.

• I see a couple of places where you are concatenating a zero-width string to a variable. I do not like this technique and it is often a signature that indicates the development has been done in a geographical culture which doesn't always have a reputation for high code quality. Instead, when you NEED to ensure that a variable is string-typed, then cast it as a string for clarity ((string)$variable). That said, if the variable is guaranteed to be string-typed, don't bloat your code with any additional syntax.

• implode()'s default glue is an empty string, this parameter can be omitted when using a zero-width glue string.

• autoloading seems like a topic worth a look. https://stackoverflow.com/q/12445006/2943403

Answer 2

There are a lot of things wrong with your code, but none of them should cause performance or security issues.

The main thing you are doing is you are using prepared statements. Of course, I would highly encourage you to make the switch to PDO as it is easier to use.

Some main pain points:

1. Never expose MySQL errors to the user! This is potentially a huge security risk, although you never really expose that error in your code, but more importantly you are shooting yourself in the leg by doing this. You should enable mysqli error reporting and log the errors on the server. Only show the information to the user about a general system issue, i.e. code 500. How to get the error message in MySQLi?

2. $this->customerDbTable should be a constant. Your SQL should not allow any variable input. For this reason, I would strongly advise avoiding writing smart classes that abstract SQL in this way. Instead, design your data models and have each model perform its operations using constant SQL expressions. (placeholder generation is an exception). The same applies to $this->databasecolomns which should have just been hardcoded SQL.

3. Never use FILTER_SANITIZE_STRING. This filter will be deprecated in PHP 8.1 as it is very harmful and will destroy your data. It does not offer any protection from any attack if that is what you expected.

4. Don't use !empty if you know the variable exists. This is a useless function call.

5. Use array append operator [] instead of array_push().

6. Each of your methods should have only one responsibility. A method should either return value or produce side effects, but not both.

7. Use strict comparison whenever possible.

8. Don't use redundant and unnecessary syntax. Things like ''.$rt.'' and $rt=($sqlexpectedValues1[$k]); use syntax that does absolutely nothing.

9. What exactly is going on inside preparesqlstatemen() method? The whole logic there looks like it can be safely simplified.

   Just take a look at how much cleaner the class becomes when we do some basic refactoring:

   class customercontact implements IContactform
   {
       protected $dbconnection;
   
       protected $statusTracker = [];
   
       protected $collectedFormData = [];
   
       protected $customerformrequestingKeys = ["customername", "customeremail", "customerphone", "messageBox"]; //requestind form attribute names
   
       public function __construct($dbcon)
       {
           $this->dbconnection = $dbcon;
       }
   
       public function addcontact(array $formvalues)
       {
           if ($this->validate($formvalues)) {
               $this->insertIntoDatabase();
               $this->statusTracker[] = ["success" => "Received and we will contact you soon"];
           }
           return $this->statusTracker;
       }
   
       private function validate(array $formvalues): bool
       {
           foreach ($this->customerformrequestingKeys as $reqKey) {
               if (!empty($formvalues[$reqKey])) {
                   $this->collectedFormData[$reqKey] = $formvalues[$reqKey];
               } else {
                   $this->statusTracker[] = ["error" => "$reqKey is Empty"];
                   return false;
               }
           }
           return true; //return true if all values are validated and filled
       }
   
       //prepare sqlinsert statement dynamically
       private function insertIntoDatabase(): void
       {
           foreach ($this->collectedFormData as $data) {
               $preparedStatementPlaceHolders[] = "?";
               $sqlexpectedTypes[] = "s";
               $sqlexpectedValues[] = $data;
           }
   
           $sql = $this->conn->prepare("INSERT INTO contact_master(c_name, c_email, c_contact, c_message) VALUES(".implode(",", $preparedStatementPlaceHolders).")");
           $sql->bind_param(implode('', $sqlexpectedTypes), ...$sqlexpectedValues);
           $sql->execute();
       }
   
       public function getcontact()
       {
       }
   }
   

10. The dbclass is absolutely useless. You could enhance it with useful abstraction methods, or you can remove it completely. Either way, you should fix your mysqli connection code. The mysqli connection should take no more no less than 3 lines:

    /* This is a useless class that provides no abstraction */
    class dbclass
    {
        public $conn;
    
        public function __construct()
        {
            mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
            $this->conn = new mysqli('localhost', 'root', '', 'contact_form');
            $this->conn->set_charset('utf8mb4'); // always set the charset
        }
    }
    

11. You are doing well by using dependency injection, but it makes no sense to provide everything in the constructor. Instead, do it like this:

    $contcatClass = new customercontact($dbcon);
    
    echo json_encode($contcatClass->addcontact($formvalues[0]));
    

12. Implement and stick to PSR-12 coding standard. It will make your code much cleaner. You can use your IDE build-in formatter or use PHP-CS-fixer.