1
\$\begingroup\$

I've already posted this PHP class few days ago when I started writing it. Now that all the methods and all the controller logics are ready, I will be ask here for some suggestions about how to improve functions and security. Here is the original code. The code I'm posting is the final work almost ready for the deployment. Since I think to be a newbie in PHP OOP, any fix for issues will be appreciated a lot.
Class.php

<?php

class market{

private $db;
private static $stmt;    
public $data = array();
public $results;    
public $id;    

public function __construct(){
//$opt = array(PDO::ATTR_PERSISTENT => true, PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION);
//    try {    
if($this->db === null){    
    $this->db = new PDO('mysql:host=localhost;dbname=marymarket','root','root'); 
        }
    //} catch (Exception $e) {
    //echo $e->getMessage();
    //}
}    

/* articles management methods */    

public function insertArticle(array $data){

$stmt = $this->db->prepare('INSERT INTO products (cod_articolo,codice_barre,pezzi_disponibili, marca, nome_articolo, prezzo, tipologia_articolo,in_promozione,note) VALUES (?,?,?,?,?,?,?,?,?)');  

if($stmt->execute(array($data['code'],$data['barcode'],$data['quantity'],$data['brand'],$data['article_name'],$data['price'],$data['article_type'],$data['promo_stats'],$data['note']))){
    return true;
    } else {   
    return false;
    }       
}

public function selectArticle($id){

$stmt = $this->db->prepare('SELECT * FROM products WHERE id=:id');    
$stmt->bindParam(':id',$id,PDO::PARAM_INT);
$stmt->execute();    
$result = $stmt->fetch(PDO::FETCH_ASSOC);
return json_encode($result);    
} 

public function editArticle(array $data){

$stmt = $this->db->prepare('UPDATE products SET cod_articolo = ? ,codice_barre = ? ,pezzi_disponibili = ?, marca = ?, nome_articolo = ? , prezzo = ?, tipologia_articolo = ? ,in_promozione = ? ,note = ? WHERE id = ?');    

if($stmt->execute(array($data['cod_articolo'],$data['codice_barre'],$data['qty'],$data['brand'],$data['article_name'],$data['price'],$data['article_type'],$data['promo_stats'],$data['note'],$data['id']))){
    return true;
    } else {
    return false;
    }   
}    

public function deleteArticle($id){

$stmt = $this->db->prepare('DELETE FROM products WHERE id=:id');    
$stmt->bindParam(':id',$id,PDO::PARAM_INT);
if($stmt->execute()){
    return true;
    } else {
    return false;
    }
}

public function listArticles(){

$stmt = $this->db->query('SELECT * FROM products'); 
$results = $stmt->fetchAll();    
return json_encode($results); 
}    

/* suppliers management methods */


public function insertSupplier(array $data){

$stmt = $this->database()->prepare('INSERT INTO suppliers (codice_interno,nome_fornitore,partita_iva,telefono,fax,email,indirizzo,citta,cap,provincia) VALUES (?,?,?,?,?,?,?,?,?,?)'); 
if($stmt->execute(array($data['cod_interno'],$data['nome_fornitore'],$data['p_iva'],$data['tel'],$data['fax'],$data['email'],$data['indirizzo'],$data['citta'],$data['cap'],$data['provincia']))){
    return true;
    } else {
    return false;
    }      
}

public function selectSupplier($id){

$stmt = $this->db->prepare('SELECT * FROM suppliers WHERE id = :id');
$stmt->bindParam(':id',$id,PDO::PARAM_INT);    
$stmt->execute();
$result = $stmt->fetch(PDO::FETCH_ASSOC);   
return json_encode($result);    
}   

public function editSupplier(array $data){
$stmt = $this->db->prepare('UPDATE suppliers SET codice_interno = ? ,nome_fornitore = ? ,partita_iva = ? ,telefono = ? ,fax = ? ,email = ? ,indirizzo = ? ,citta = ?,cap = ?,provincia = ? WHERE id = ?');    
if($stmt->execute($data['cod_interno'],$data['nome_fornitore'],$data['p_iva'],$data['tel'],$data['fax'],$data['email'],$data['indirizzo'],$data['citta'],$data['cap'],$data['provincia'],$data['id'])){
    return true;
    } else {
    return false;
    }
}    

public function deleteSupplier($id){

$stmt = $this->db->prepare('DELETE FROM suppliers WHERE id = :id');
$stmt->bindParam(':id',$id,PDO::PARAM_INT);
if($stmt->execute()){
    return true;    
    } else {
    return false;
    }   
}     

public function listSuppliers(){

$stmt = $this->db->query('SELECT * FROM suppliers');    
$stmt->execute();
$results = $stmt->fatchAll();
return json_encode($results);    
}

/* brands management methods */    

public function insertBrand(array $data){

$stmt = $this->db->prepare('INSERT INTO brands (codice_interno,marca,tipologia_prodotti) VALUES (?,?,?)');       
if($stmt->execute($data['codice_interno'],$data['marca'],$data['tipologia_prodotti'])){
    return true;
    } else {
    return false;
    }   
}    

public function selectBrand($id){

$stmt = $this->db->prepare('SELECT * FROM brands WHERE id = :id');
$stmt->bindParam(':id',$id,PDO::PARAM_INT);    
$stmt->execute();
$result = $stmt->fetch(PDO::FETCH_ASSOC);    
return json_encode($result);    
}    

public function editBrand(array $data){

$stmt = $this->db->prepare('UPDATE brands SET codice_interno = ? , marca = ?, tipologia_prodotti = ? WHERE id = ?');    
$stmt->execute(array($data['codice_interno'],$data['marca'],$data['tipologia_prodotti'],$data['id'])){
    return true;
    } else {
    return false;
    }   
}    

public function deleteBrand($id){

$stmt = $this->db->prepare('DELETE FROM brands WHERE id = :id');
$stmt->bindParam(':id',$id,PDO::PARAM_INT);
if($stmt->execute()){
    return true;
    } else {
    return false;
    }    
}    

public function listBrands(){

$stmt = $this->db->query('SELECT * FROM brands');    
$results = $stmt->fetchAll();
return json_encode($results);
}    

/* search */    

public function search($q){

$stmt = $this->db->prepare('SELECT * FROM products WHERE nome_articolo LIKE :q OR marca LIKE :q OR cod_articolo LIKE :q');    
$stmt->bindParam(':q',$q,PDO::PARAM_STR);    
$stmt->execute();    
if($stmt->rowCount() > 0){
$results = $stmt->fetchAll();
return json_encode($results);    
    } else {
return false;    
    }       
} 

}

?>

Here is the controller code who is called using Jquery AJAX. I've implemented a static variable called "action" when the $_POST and $_GET requests are made,this to manage all the different functions of the view that is an html file.

controller.php

<?php
require_once 'core.php';

$core = new market();


/* articles $_POST requests */

if(isset($_POST['action']) && $_POST['action'] === 'insertArticle'){ 
$code = filter_var($_POST['code'],FILTER_SANITIZE_STRING); 
$barcode = filter_var($_POST['barcode'],FILTER_SANITIZE_NUMBER_INT); 
$qty = filter_var($_POST['quantity'],FILTER_SANITIZE_STRING);     
$brand = filter_var($_POST['brand'],FILTER_SANITIZE_STRING);
$article_name = filter_var($_POST['artName'],FILTER_SANITIZE_STRING);  
$article_type = filter_var($_POST['artType'],FILTER_SANITIZE_STRING);
$price = filter_var($_POST['price'],FILTER_SANITIZE_NUMBER_INT);
$promo_stats = filter_var($_POST['promoStats']);    
$note = filter_var($_POST['note'],FILTER_SANITIZE_STRING);    

$data = array('code'=>$code,
              'barcode'=>$barcode,
              'quantity'=>$qty,
              'brand'=>$brand,
              'article_name'=>$article_name,
              'article_type'=>$article_type,
              'price'=>$price,
              'promo_stats'=>$promo_stats,
              'note'=>$note
);    

$save = $core->insertArticle($data);  

}

if(isset($_POST['action']) && $_POST['action'] === 'editArticle'){
$id = filter_var($_POST['id'],FILTER_SANITIZE_NUMBER_INT);
$code = filter_var($_POST['code'],FILTER_SANITIZE_STRING); 
$barcode = filter_var($_POST['barcode'],FILTER_SANITIZE_NUMBER_INT); 
$qty = filter_var($_POST['quantity'],FILTER_SANITIZE_STRING);     
$brand = filter_var($_POST['brand'],FILTER_SANITIZE_STRING);
$article_name = filter_var($_POST['artName'],FILTER_SANITIZE_STRING);  
$article_type = filter_var($_POST['artType'],FILTER_SANITIZE_STRING);
$price = filter_var($_POST['price'],FILTER_SANITIZE_NUMBER_INT);
$promo_stats = filter_var($_POST['promoStats']);    
$note = filter_var($_POST['note'],FILTER_SANITIZE_STRING);    

$data = array('id'=>$id,
              'code'=>$code,
              'barcode'=>$barcode,
              'quantity'=>$qty,
              'brand'=>$brand,
              'article_name'=>$article_name,
              'article_type'=>$article_type,
              'price'=>$price,
              'promo_stats'=>$promo_stats,
              'note'=>$note
             );  
$update = $core->editArticle($data);    
if($update === true){
    echo 'ok';
    } else {
    echo 'error';
    }    
}

if(isset($_POST['action']) && $_POST['action'] === 'deleteArticle'){
$id = filter_var($_POST['id'],FILTER_SANITIZE_NUMBER_INT);    
$del = $core->deleteArticle($id);
echo $del;    
}

/* suppliers $_POST requests */

if(isset($_POST['action']) && $_POST['action'] === 'insertSupplier' ){

$code = filter_var($_POST['codice_interno'],FILTER_SANITIZE_STRING); 
$name = filter_var($_POST['nome_fornitore'],FILTER_SANITIZE_STRING); 
$piva = filter_var($_POST['p_iva'],FILTER_SANITIZE_NUMBER_INT); 
$tel = filter_var($_POST['tel'],FILTER_SANITIZE_NUMBER_INT);
$fax = filter_var($_POST['fax'],FILTER_SANITIZE_NUMBER_INT); $email = filter_var($_POST['email'],FILTER_SANITIZE_STRING);
$indirizzo = filter_var($_POST['indirizzo'],FILTER_SANITIZE_STRING);
$citta = filter_var($_POST['citta'],FILTER_SANITIZE_STRING);    
$cap = filter_var($_POST['cap'],FILTER_SANITIZE_NUMBER_INT);   $provincia = filter_var($_POST['provincia'],FILTER_SANITIZE_STRING);

$data = array('cod_interno'=>$code,
              'nome_fornitore'=>$name,
              'p_iva'=>$piva,
              'tel'=>$tel,
              'fax'=>$fax,
              'email'=>$email,
              'indirizzo'=>$indirizzo,
              'citta'=>$citta,
              'cap'=>$cap
              'provincia'=>$provincia
);     

$save = $core->insertSupplier($data);

}

if(isset($_POST['action']) && $_POST['action'] === 'editSupplier' ){

$id = filter_var($_POST['id'],FILTER_SANITIZE_NUMBER_INT);    
$code = filter_var($_POST['codice_interno'],FILTER_SANITIZE_STRING); 
$name = filter_var($_POST['nome_fornitore'],FILTER_SANITIZE_STRING); 
$piva = filter_var($_POST['p_iva'],FILTER_SANITIZE_NUMBER_INT); 
$tel = filter_var($_POST['tel'],FILTER_SANITIZE_NUMBER_INT);
$fax = filter_var($_POST['fax'],FILTER_SANITIZE_NUMBER_INT); $email = filter_var($_POST['email'],FILTER_SANITIZE_STRING);
$indirizzo = filter_var($_POST['indirizzo'],FILTER_SANITIZE_STRING);
$citta = filter_var($_POST['citta'],FILTER_SANITIZE_STRING);    
$cap = filter_var($_POST['cap'],FILTER_SANITIZE_NUMBER_INT);   $provincia = filter_var($_POST['provincia'],FILTER_SANITIZE_STRING);

$data = array('id'=>$id,
              'cod_interno'=>$code,
              'nome_fornitore'=>$name,
              'p_iva'=>$piva,
              'tel'=>$tel,
              'fax'=>$fax,
              'email'=>$email,
              'indirizzo'=>$indirizzo,
              'citta'=>$citta,
              'cap'=>$cap
              'provincia'=>$provincia
);     

$update = $core->editSupplier($data);    

}

if(isset($_POST['action']) && $_POST['action'] === 'deleteSupplier' ){
$id = filter_var($_POST['id'],FILTER_SANITIZE_NUMBER_INT);    
$del = $core->deleteSupplier($id);
echo $del;        
}

/* brands $_POST requests */

if(isset($_POST['action']) && $_POST['action'] === 'insertBrand'){

$code = filter_var($_POST['codice_interno'],FILTER_SANITIZE_STRING);
$name = filter_var($_POST['marca'],FILTER_SANITIZE_STRING);
$type = filter_var($_POST['tipologia_prodotti'],FILTER_SANITIZE_STRING);    

$data = array('codice_interno'=>$code,
              'marca'=>$name,
              'tipologia_prodotti'=>$type
);    

$save = $core->insertBrand($data);

}

if(isset($_POST['action']) && $_POST['action'] === 'editBrand'){

$id = filter_var($_POST['id'],FILTER_SANITIZE_NUMBER_INT);    
$code = filter_var($_POST['codice_interno'],FILTER_SANITIZE_STRING);
$name = filter_var($_POST['marca'],FILTER_SANITIZE_STRING);
$type = filter_var($_POST['tipologia_prodotti'],FILTER_SANITIZE_STRING);    

$data = array('id' =>$id,
              'codice_interno'=>$code,
              'marca'=>$name,
              'tipologia_prodotti'=>$type
);    

$update = $core->editBrand($data);    

}

if(isset($_POST['action']) && $_POST['action'] === 'deleteBrand'){
$id = filter_var($_POST['id'],FILTER_SANITIZE_NUMBER_INT);    
$del = $core->deleteBrand($id);
//echo $del;    
}

/* $_GET requests */

if(isset($_GET['action']) && $_GET['action'] === 'productsList'){
$results = $core->listArticles(); 
echo $results;    
}

if(isset($_GET['action']) && $_GET['action'] === 'suppliersList' ){
$results = $core->listSuppliers();
echo $results;    
}

if(isset($_GET['action']) && $_GET['action'] === 'brandsList' ){
$results = $core->listBrands();
echo $results;    
}

if(isset($_GET['action']) && $_GET['action'] === 'selectArticle'){
$id = filter_var($_GET['id'],FILTER_SANITIZE_NUMBER_INT);    
$selected = $core->selectArticle($id);
echo $selected;    
}

if(isset($_GET['action']) && $_GET['action'] === 'selectSupplier'){    
$id = filter_var($_GET['id'],FILTER_SANITIZE_NUMBER_INT);     
$selected = $core->selectSupplier($id);
echo $selected;    
}

if(isset($_GET['action']) && $_GET['action'] === 'selectBrand'){
$id = filter_var($_GET['id'],FILTER_SANITIZE_NUMBER_INT);
$selected = $core->selectBrand($id); 
echo $selected;    
}

/* search */

if(isset($_POST['q'])){
$q = filter_var($_POST['q'],FILTER_SANITIZE_STRING);
$results = $core->search($q);    
echo $results;
}

?>

My doubt about all the code is about the prepared statements, I'm using an array() inside the execute()function, this to avoid to write long SQLqueries, so I'm not sure if this can be a good practice for the security. Also I've a doubt about the controller and on how to echoa response to the view, because of this for now I've limited the response from the controller to an if(), else() that return true or false in case of errors in the query execution.

NB:
For now the database tables design is a draft, so every column name is temporary.

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Mutually exclusive conditionals

Take a closer look at this:

if(isset($_GET['action']) && $_GET['action'] === 'productsList'){
$results = $core->listArticles(); 
echo $results;    
}

if(isset($_GET['action']) && $_GET['action'] === 'suppliersList' ){
$results = $core->listSuppliers();
echo $results;    
}

if(isset($_GET['action']) && $_GET['action'] === 'brandsList' ){
$results = $core->listBrands();
echo $results;    
}

If the value of $_GET['action'] is productsList, then it's unnecessary to evaluate other conditionals on this value. When you work with mutually exclusive conditions like this, then chain them together with elseif.

In addition, it's also unnecessary to evaluate isset($_GET['action']) repeatedly. It's common in all the above conditions, so it will be better to write like this:

if (isset($_GET['action'])) {
    if ($_GET['action'] === 'productsList') {
        // ...
    } elseif ($_GET['action'] === '...') {
        // ...
    } elseif ($_GET['action'] === '...') {
        // ...
    }
}

Use boolean conditions directly

Instead of this:

if ($stmt->execute(...)) {
    return true;
} else {   
    return false;
}

You can use the value of boolean conditions directly:

return $stmt->execute(...));

Avoid wildcard selects

Instead of wildcard selects like SELECT * FROM products WHERE id=:id, it's better to list the specific columns you really need. That will have two practical benefits:

  • Reduce the amount of data transferred between your program and the database
  • Make your program agnostic of the ordering of the columns in the database

Don't repeat yourself

Many of the methods that operate on some tables are almost identical, except for the table names. It would be good to extract the common logic to helper methods with parameters, so that you can reduce the repetitive boilerplate code.

Improve this answer
\$\endgroup\$
7
  • \$\begingroup\$ Ok thank you for the suggestions. About the wildcards I was using them likeSELECT * FROM products WHERE id=:id because I want to display all the data that are in the database table. At the initial stage of the development i was using a select query who was displaying only some column form the tables, but the user of final application need to display all the stored info so I fall back on the wildcards. For the helper methods, I'm not sure on how to proceed, I want to avoid to insert a variable inside the code to specify what table needs to be displayed. Do you have suggestions? \$\endgroup\$
    – user9741470
    May 19, 2018 at 10:58
  • 1
    \$\begingroup\$ Even if you will use all columns of the tables, it's a good practice to list them explicitly, to protect the program in case the columns get reordered in the schema. As for the helper method, if you are concerned about SQL injection vulnerability when inserting table name variable as string, that's a valid point. But it's perfectly safe to do when the method is only called internally, and unreachable by unsanitized user input. \$\endgroup\$
    – janos
    May 19, 2018 at 11:05
  • \$\begingroup\$ The lists method views are loaded when the DOM if fully loaded, so every $_GET request is made after $(document).ready(). I will consider to write an helper methods, but my worries are about the fact that JS is a client side code and can be manipulated from someone who has the right knowledge about. For the deployment version I will take care to remove wildcards and specify all the columns that are needed by the single html tables who display the data. \$\endgroup\$
    – user9741470
    May 19, 2018 at 11:14
  • \$\begingroup\$ My suggestion is to change methods like listArticles and listSuppliers to call a helper method with parameters products and suppliers, respectively. These table names will be hardcoded in the listArticles and listSuppliers methods and cannot possibly be affected by client-side code. There is no vulnerability here. \$\endgroup\$
    – janos
    May 19, 2018 at 11:28
  • 1
    \$\begingroup\$ No, the table name must never come from an $_GET request. \$\endgroup\$
    – janos
    May 19, 2018 at 11:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.