Image upload script more cleaner and secure using php 7 oop style.

Question

I want to utilize namespaces and make the code on this file uploading script more secure, and cleaner looking, here is what i have so far

it uploads an image and inserts it into a database.

I want to be able to use namespaces within view files, without it looking messy.

i know i can use laravel blade etc, but for the sake of sharpening my php skills, i want to be able to do this from scratch.

Image.php

<?php

require 'Db.php';


class Image{

    private $dbh;
    private $connect;
    public $directory;
    public $uploadfile;




    public function __construct()
    {
        $this->dbh = new Db();
        $this->connect =  $this->dbh->connect();
        $this->directory = "uploads/";
    }


    public function upload_image($filename, $image_name)
    {

        $this->uploadfile = $this->directory . basename($filename);

        try{
            $stmt = $this->connect->prepare("INSERT INTO images (img, image_name) VALUES (:img, :imagename) ");
            $stmt->bindparam(':img', $this->uploadfile);
            $stmt->bindparam(':imagename', $image_name);
            $stmt->execute();
            return $stmt; 
        }
        catch(PDOExeception $e)
        {
            echo $e->getMessage();
        }


    }

    public function get_image()
    {
        return $this->uploadfile;
    }

}

upload_image.php (view file)

    <form action="/ImgUpload.php" enctype="multipart/form-data" method="post">

          <div class="form-group">
            <label for="exampleInputFile">File input</label>
            <input type="file" name="profile_img" class="form-control-file" id="exampleInputFile" aria-describedby="fileHelp">
            <input type="text" name="image_name" placeholder="Enter Name Of File" class="mt-3">
            <small id="fileHelp" class="form-text text-muted">This is some placeholder block-level help text for the above input. It's a bit lighter and easily wraps to a new line.</small>
            <button type="submit" class="btn btn-primary">Submit</button>
          </div>

        </form>

ImgUpload.php

<?php
session_start();
require_once 'Image.php';



if(isset($_FILES['profile_img'])){

    $image = new Image();

    $filename = $_FILES["profile_img"]["name"];
    $realname = "uploads/" . basename($filename);
    $directory = "/Applications/MAMP/htdocs/eliphp1/uploads/";

    $path = $directory . basename($filename);

    $image_name = $_POST['image_name'];


    if($image->upload_image($realname, $image_name)){

        move_uploaded_file($_FILES["profile_img"]["tmp_name"], $path);

        echo '<img src="'.$image->get_image().'">';


    };


};




?>

mickmackusa · Accepted Answer · 2018-06-16 22:46:40Z

To elaborate on the answer from Stexxe...

it can have connection to database ($this->connect of your Image class) as dependency

A database class is different from all other classes. It cannot be created any time on demand. It has to be created only once, connect to a database, and hold this connection all the time. Otherwise numerous connections from the same script will crash your database server.

So it has to be:

class ImageMapper{

    private $connect;

    public function __construct($connect)
    {
        $this->connect = $connect;
    }

and called like this:

$dbh = new Db();
$connect =  $dbh->connect();

$image = new Image($connect);

manipulation images classes should not expose any information about their inner workings

Among other things, it means that error reporting should never reveal the actual error to a site user. Therefore you should never ever do anything like die($error) -- which also makes the whole try..catch business misplaced in this code. So it should be:

public function upload_image($filename, $image_name)
{
    $sql = "INSERT INTO images (img, image_name) VALUES (?, ?)";
    $this->connect->prepare($sql)->execute([$filename, $image_name]);
}

whereas a possible exception thrown by PDO must be caught elsewhere. Learn more from my article on PHP error reporting.

Some other stuff has also been fixed, mostly your extremely confusing way assigning a filename, when you are adding, removing and re-adding the directory name. Some useless variables like $directory and $filename are also removed.

Stexxe · Accepted Answer · 2018-06-15 10:01:52Z

I suggest to separate responsibility to following classes:

Image Basically value object to store image's state (temporary path to uploaded image and actual path or any other information about image)
ImageUploader The only one responsibility of this class is to move uploaded file to new place and change image's state after. I think this class could be static.
ImageStore Class for storing image in a database. it can have connection to database ($this->connect of your Image class) as dependency. It should only be responsible to store data about image in a database.

Also, manipulation images classes should not expose any information about their inner workings (how image data is actually stored or how works image uploading)

Pseudo code for ImgUpload.php

Get information about image from POST
Create image object
Create store for images
Save Image to store
Upload image

Stack Exchange Network

Image upload script more cleaner and secure using php 7 oop style.

2 Answers 2

You must log in to answer this question.

Linked

Hot Network Questions

Image upload script more cleaner and secure using php 7 oop style.

2 Answers 2

You must log in to answer this question.

Linked

Related

Hot Network Questions