Having trouble deleting multiple queries (PHP, MYSQL)

Question

I'm having trouble making WHERE IN query work, I was only able to make it work as a foreach loop.

My payload looks like this:

sku[]: BOOK0002
sku[]: BOOK0003

My API file code is as follows:

    public function deleteProduct($prdt_sku)
    {
        try {
            $this->db->query("DELETE FROM products WHERE product_sku IN :sku");
            $this->db->bind(":sku", $prdt_sku);

            if ($this->db->execute()) {
                return true;
            } else {
                return false;
            }
        } catch (Throwable $e) {
            echo 'Error deleting product from database. Reirecting...';
            header("HTTP/1.1 406 Error deleting product from database");
            header("refresh:3;url=https://metwesh.github.io/scandiweb-product-page/add-product");
        }
    }

My actual form destination:

$response = array();

if (isset($_POST['sku'])) {

     foreach ($sku as $deletables) {
         $result = $api->deleteProduct($deletables);
     }

    if ($result) {
        header("HTTP/1.1 200 OK");
        header("Location: https://metwesh.github.io/scandiweb-product-page/add-product");
        exit();
    } else {
        header("HTTP/1.1 406 Error deleting product");
    }
} else {
    header("HTTP/1.1 499 Required parameters missing");
    echo 'Please select one or more of the checkboxes. Reirecting...';
    header("refresh:3;url=https://metwesh.github.io/scandiweb-product-page/");
}

I need to use the WHERE IN clause so I can have only one query per form & reduce the unnecessary amount of queries I'm using right now. Any help would be greatly appreciated, thanks.

Update My PDO:

<?php

class Database
{
    private $host = DB_SERVER;
    private $user = DB_USERNAME;
    private $pass = DB_PASSWORD;
    private $dbname = DB_NAME;

    private $dbh;
    private $stmt;
    private $error;

    public function __construct()
    {
        $dsn = 'mysql:host=' . $this->host . ';dbname=' . $this->dbname;
        $options = array(
            PDO::ATTR_PERSISTENT => true,
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
        );

        try {
            $this->dbh = new PDO($dsn, $this->user, $this->pass, $options);
        } catch (PDOException $e) {
            $this->error = $e->getMessage();
            echo $this->error;
        }
    }

    public function query($sql)
    {
        $this->stmt = $this->dbh->prepare($sql);
    }

    public function bind($param, $value, $type = null)
    {
        if (is_null($type)) {
            switch (true) {
                case is_int($value):
                    $type = PDO::PARAM_INT;
                    break;
                case is_bool($value):
                    $type = PDO::PARAM_BOOL;
                    break;
                case is_null($value):
                    $type = PDO::PARAM_NULL;
                    break;
                default:
                    $type = PDO::PARAM_STR;
            }
        }

        $this->stmt->bindValue($param, $value, $type);
    }

    public function execute()
    {
        return $this->stmt->execute();
    }

    public function resultSet()
    {
        $this->execute();
        return $this->stmt->fetchAll(PDO::FETCH_OBJ);
    }

    public function single()
    {
        $this->execute();
        return $this->stmt->fetch(PDO::FETCH_OBJ);
    }
}
```

Answer 1

Yeah, this shows the limitations of creating a PDO wrapper. It makes it difficult to get rid of the foreach loop. Your wrapper is quite simple though, so I'll ignore it for now and use plain PDO.

The problem here is that you need a variable amount of placeholders in your query depending on how many entries there are in $prdt_sku. I solved this by using ? placeholders, like this:

public function deleteProducts(PDO $pdoHandle, array $products_sku)
{
    try {
        // put the right amount of placeholders in the query
        $query = "DELETE FROM products WHERE product_sku IN (" . 
                  trim(str_repeat(",?", count($products_sku)), ",") . ")";
        $statement = $pdoHandle->prepare($query);
        return $statement->execute($products_sku);
    } catch (Throwable $e) {
        echo 'Error deleting products from database. Redirecting...';
        header("HTTP/1.1 406 Error deleting product from database");
        header("refresh:3;url=https://metwesh.github.io/scandiweb-product-page/add-product");
        exit;
    }
}

Note that this function now deletes multiple products. Here the assumption is that your "payload" is:

$products_sku = ["BOOK0002", "BOOK0003"];

It is actually not so difficult to use your wrapper for this. All that is needed is a small change to your execute() method. Like this:

public function execute($parameters = null)
{
    return $this->stmt->execute($parameters);
}

Then the deleteProducts() method can become this:

public function deleteProducts(array $products_sku)
{
    try {
        // put the right amount of placeholders in the query
        $query = "DELETE FROM products WHERE product_sku IN (" . 
                  trim(str_repeat(",?", count($products_sku)), ",") . ")";
        $this->db->query($query);
        return $this->db->execute($products_sku);
    } catch (Throwable $e) {
        echo 'Error deleting product from database. Reirecting...';
        header("HTTP/1.1 406 Error deleting product from database");
        header("refresh:3;url=https://metwesh.github.io/scandiweb-product-page/add-product");
        exit;
    }
}

Some notes:

I really don't like variable names like $prdt_sku. Try to pronounce that! Most people would know what a "sku" is, so that's an acceptable abbreviation, but why do you use "prdt"? It must stand for "product", but would $product_sku be so bad? Please don't abbreviate unnecessarily. It's not faster, you don't safe memory space, all you do is make your code harder to read. Do you want that?

I cringe a bit at the way you handle the exception. You print an error and then jump out of the method and load another URL after 3 seconds. That's quite inflexible. You can never handle this exception in any other way. The worst thing is that you don't actually die or exit after the headers. That means code execution continues as if nothing happened. The method also doesn't return anything. Why not do:

public function deleteProducts(array $products_sku)
{
    try {
        // put the right amount of placeholders in the query
        $query = "DELETE FROM products WHERE product_sku IN (" . 
                  trim(str_repeat(",?", count($products_sku)), ",") . ")";
        $this->db->query($query);
        return $this->db->execute($products_sku);
    } catch (Throwable $e) {
        <.... store reason of exception ....>
        return false;
    }
}

Then the calling code can still decide what to do with any failures. I would also store the reason for the exception somewhere in the class, so you can actually tell what happened. Note that the PDO statement does something similar with PDOStatement::errorInfo().

Answer 2

For the case when the POST data does not contain sku, the following code occurs:

header("HTTP/1.1 499 Required parameters missing");
echo 'Please select one or more of the checkboxes. Reirecting...';
header("refresh:3;url=https://metwesh.github.io/scandiweb-product-page/");

HTTP Status code 499 is:

A non-standard status code introduced by nginx for the case when a client closes the connection while nginx is processing the request. ¹

A more appropriate status code for such a case would be 400 or 422.

Also note the documentation for header() states:

The second special case is the "Location:" header. Not only does it send this header back to the browser, but it also returns a REDIRECT (302) status code to the browser unless the 201 or a 3xx status code has already been set. ²

Thus the status code other than 201 or 3xx likely isn't being respected.

Also, hopefully the code is running on PHP 5.4 (and given LTS - hopefully it is running on PHP 7.4 or later) and presuming this, consider using http_response_code() instead of using header() to set the status code. For more context on this, refer to answers to this Stack Overflow question.