Using A Parent Associative Foreach Loop That Contains A Glob Function (to delete files) and a PDO Statement To Update A Database - PHP

Question

I have a form that outputs images and their related titles and tags via a while loop, and at the bottom of this form I have the option to delete an image.

Because the input elements inside the form produce an array of values, when deleting an image I need to run the PDO statements and PHP glob methods that delete the images from their folders inside a parent foreach loop.

I cannot get this to work though. Without the foreach loop I'm getting the PHP Array to string conversion error, which I understand/expect. This error throws on the line $stmt->bindParam(':image_id', $image_id); in the second section of code below.

I think I need to wrap the code from in-between the // ---- START FOREACH ? and // ---- END FOREACH ? comments in an associative foreach loop, but I can't work how to do this in terms of the key/value pairs?

Any help would be really appreciated.

Output onto page (the issue is in the block of code after this)

<?php
    isset($_REQUEST['username']) ? $username = $_REQUEST['username'] : header("Location: login.php");
    $user_id = $_SESSION['logged_in'] ?? header("Location: login.php");
?>

<form method="post" enctype="multipart/form-data">

    <?php
        $stmt = $connection->prepare("SELECT * FROM lj_imageposts WHERE user_id = :user_id");

        $stmt->execute([
            ':user_id' => $user_id
        ]); 

        while ($row = $stmt->fetch()) {

            $db_image_id = htmlspecialchars($row['image_id']);
            $db_image_filename = htmlspecialchars($row['filename']);
            $db_image_ext = htmlspecialchars($row['file_extension']);

    ?>
    
    <div class="upload-details-component">                
            <div>
                <img src="project/images-lib/image.jpg">
            </div>
            <div class="edit-zone">
                <div class="form-row">
                    <label for="upload-details-title">Image Title</label>
                    <input id="upload-details-title" type="text" name="image-title[]">
                </div>
                <div class="form-row upload-details-form-row">
                    <label for="upload-details-tags">Comma Separated Image Tags</label>
                    <textarea name="image-tags[]"></textarea>
                </div>
                <div class="form-row">
                    <input type="hidden" name="username" value="<?php echo $username;?>">
                    <input type="hidden" name="image-id[]" value="<?php echo $db_image_id;?>">
                    <button name="upload-details-delete[]">DELETE</button>
                </div>
            </div>
    </div>

    <?php } ?>

        <div class="form-row upload-details-submit-row">
            <button type="submit" name="upload-submit">COMPLETE UPLOAD</button>
        </div>
    </form>

Deleting An Image

<?php

    if(isset($_POST['upload-details-delete'])) {

        $loggedInUser = $user_id;
        $imagesLibrary = 'images-lib/';
        $imagesDownload = 'images-download/';
        $image_id = $_POST['image-id'];

    try {
        $sql = "DELETE FROM `lj_imageposts` WHERE image_id = :image_id AND user_id = :user_id";
        $stmt = $connection->prepare($sql);

        // ---- START FOREACH ?

        $stmt->bindParam(':image_id', $image_id);
        $stmt->bindParam(':user_id', $loggedInUser);

        $stmt->execute();
        
        // delete image files from 'images-lib' folder
        foreach(glob($imagesLibrary . $db_image_filename . '-{500,750,1000,1500}' . '.' . $db_image_ext, GLOB_BRACE) as $i) {
            unlink($i);
        }

        // delete files from 'images-download' folder
        foreach(glob($imagesDownload . $db_image_filename . '.' . $db_image_ext) as $i) {
            unlink($i);
        }

        // ---- END FOREACH ?
                
        header("Location: upload-details.php?username={$db_username}");

        exit;

        } catch (PDOException $e) {
            echo "Error: " . $e->getMessage();
        }
    }
?>

This could use some debug information... while you ignore that `glob()` may return `false`. It may be, that this alternation part of the pattern `{500,750,1000,1500}` just doesn't match any filename, because it's not alternating at all. — Martin Zeitler, Jan 01 '22 at 00:27
@MartinZeitler that isn't the issue. The image sizes those values represent are done via the Imagick image library on the initial upload, which then directs to this page. When I had the form set up differently that functionality worked fine (originally each image was inside its own form), but I've now changed it so all images can be submitted in one go after adding titles and tags. — pjk_ok, Jan 01 '22 at 00:47

score 1 · Answer 1 · answered Jan 04 '22 at 22:00

This is some messy code, and there's a lot you can do to clean it up. With regards to your core problem, the fix is simple. According to the HTML spec:

A button (and its value) is only included in the form submission if the button itself was used to initiate the form submission.

In other words, a button is not submitted unless it is clicked. Simply adding the ID as value to each delete button should make things quite simple. You know you'll only get one value sent with that button's name.

But first, you need to separate your logic from your presentation. Ideally the HTML should be in a separate file, but we'll stick them together here for ease, moving all the PHP code to a separate block. Use of alternative control structure syntax and short echo tags will clean things up nicely in your HTML.

You are abusing operators in your first two lines when a simple conditional statement would be more understandable.

It's not clear what your escape() function does, but use of htmlspecialchars() is all that's needed for HTML display.

The id attribute of an element must be unique in the document, and must be present for the for attribute of the <label> element to work properly. Typically a database ID is used to make the values unique.

Where do you think $db_image_filename is coming from in the delete code? You've defined it on the display page inside a loop, and then aren't submitting it which is obviously not going to be of any help. So you'll need to fetch it again here. (A better strategy would have been to name the files with the database ID so you could just delete image_XXX_.* where XXX is your id.)

Never echo database errors to your users. At best it confuses them; at worst it gives them details of your code and database schema that can be used in an attack.

<?php
// avoid gross misuse of the ternary and null coalesce operators
if (empty($_REQUEST["username"]) || empty($_SESSION["logged_in"])) {
    header("Location: login.php");
    exit;
}

// why is this not in the session?
$username = $_REQUEST['username'];
// this array key should have a better name (like "user_id" for example)
$user_id = $_SESSION['logged_in'];


if(isset($_POST['upload-details-delete'])) {
    // delete files and database entries

    $imagesLibrary = 'images-lib/';
    $imagesDownload = 'images-download/';
    // get image ID from the button value
    $image_id = $_POST['upload-details-delete'];

    try {
        // get the file info before we delete the entry
        $sql = "SELECT * FROM lj_imageposts WHERE image_id = ? AND user_id = ?";
        $stmt = $connection->prepare($sql);
        $stmt->execute([$image_id, $user_id]);
        $fileinfo = $stmt->fetch(\PDO::FETCH_ASSOC);

        // delete the entry    
        $sql = "DELETE FROM `lj_imageposts` WHERE image_id = ? AND user_id = ?";
        $stmt = $connection->prepare($sql);
        $stmt->execute([$image_id, $user_id]);

        // delete image files from 'images-lib' folder
        $pattern = $imagesLibrary . $fileinfo["image_filename"] . "-{500,750,1000,1500}" . "." . $fileinfo["image_ext"];
        foreach(glob($pattern, GLOB_BRACE) as $i) {
            unlink($i);
        }

        // delete files from 'images-download' folder
        $pattern = $imagesLibrary . $fileinfo["image_filename"] . "." . $fileinfo["image_ext"];
        foreach(glob($pattern) as $i) {
            unlink($i);
        }

    } catch (\Throwable $e) {
        error_log($e->getMessage());
        // don't echo database errors to your users
        http_response_code(500);
        echo "Database Error";
        exit;
    }
}

if (isset($_POST["upload_submit"])) {
    // do your upload stuff
}


try {
    $stmt = $connection->prepare("SELECT * FROM lj_imageposts WHERE user_id = ?");
    $stmt->execute([$user_id]);
    // fetch all the rows into a single array
    $data = $stmt->fetchAll(\PDO::FETCH_ASSOC);
    // escape the data for html display
    foreach ($data as &$row) {
        $row = array_map('htmlspecialchars', $row);
    }
} catch (\Throwable $e) {
    $data = [];
}
?>
<form method="post" enctype="multipart/form-data">

    <div class="upload-details-component">
        <div>
            <img src="project/images-lib/image.jpg">
        </div>

        <?php foreach ($data as $row): ?>
        <div class="edit-zone">
            <div class="form-row">
                <label for="upload-details-title<?=$row["image_id"]?>">Image Title</label>
                <input id="upload-details-title<?=$row["image_id"]?>" type="text" name="image-title[]" value="<?=$row["image_title"]?>"/>
            </div>
            <div class="form-row upload-details-form-row">
                <label for="upload-details-tags<?=$row["image_id"]?>">Comma Separated Image Tags</label>
                <textarea id="upload-details-tags<?=$row["image_id"]?>" name="image-tags[]"><?=$row["image_tags"]?></textarea>
            </div>
            <div class="form-row">
                <input type="hidden" name="image-id[]" value="<?=$row["image_id"]?>"/>
                <button name="upload-details-delete" value="<?=$row["image_id"]?>">DELETE</button>
            </div>
        </div>
        <?php endforeach ?>

    </div>
    <div class="form-row upload-details-submit-row">
        <input type="hidden" name="username" value="<?=$username?>"/>
        <button name="upload-submit">COMPLETE UPLOAD</button>
    </div>
</form>

Thanks @miken32 . I will look through this properly later when I'm at my desk. I've tweaked the question and changed `escape()` to `htmlspecialchars()`. That was a function I wrote to do the same thing that was easier to type that I forgot to swap back for the purposes of the question. — pjk_ok, Jan 05 '22 at 15:39

Using A Parent Associative Foreach Loop That Contains A Glob Function (to delete files) and a PDO Statement To Update A Database - PHP

1 Answers1