Critique of my PHP thread deletion class

[Fishcakes]

I'm creating a forum and I created a button which deletes from the SQL database and also the file (image/pdf or mp4)

Could I get some guidance on how to turn the SQL query into a prepared statement?

As at the moment I've made it so only a User of their thread can delete their own thread (I intend to implement moderators that can delete threads also). Do I need to do additional security checks on a $_SESSION variable?

<?php
include 'Dbh.php' ;

class DeleteThread extends Dbh {
    
    
    public function DisplayDeleteButton($number) {
        if($UserOfThread == $CurrentUser)
        {
            echo "	<form action='Includes/ClassDeleteThread_ViewThreadPage.php' method='post' enctype'multipart/form-data'>
        <input type='submit' name='Delete Thread' value='Delete'/>
     		 <input type='hidden' name='name' value=$number>
     		 
    	</form>" ;
        }
    }
    
    public function DeleteThreadFromDataBase($PostID, $UserOfThread, $CurrentUser)
    {
        
        if($UserOfThread == $CurrentUser)
        {
            $sql = "DELETE FROM `Threads` WHERE id = $PostID";
            $result = $this->connect()->query($sql);
            echo "success" ;
        }
        else {
            echo "failure" ;
        }
        
        
    }
    
    public function DeleteFileFromServer($UserOfThread, $CurrentUser,$FilePath)
    { 	if($UserOfThread == $CurrentUser)
    {
        
        $UploadDirectory = "../upload/" ;
        $ThumbnailDirectory = "../upload/Thumbnails/" ;
        $FilePath = $UploadDirectory . $FilePath  ;
        $ThumbnailDirectory = $ThumbnailDirectory . $FilePath  ;
        unlink($FilePath);
        echo "<P> Upload File Deleted" ;
        
        unlink($ThumbnailDirectory);
        echo "<P> Thumbnail File Deleted" ;
    }
    else { 
        echo "<P> You are not the owner of this thread nor a mod" ; 
    } 
    
    
    }
    
}

$CurrentUser = $_POST['CurrentUser'] ;
$UserOfThread = $_POST['UserOfThread'] ;
$PostID = $_POST['id'] ;
$FilePath =  $_POST['FilePath'] ;


echo  $CurrentUser;
echo  $UserOfThread;
echo  $PostID;
echo  $FilePath;


$Gen = new DeleteThread();
$Gen->DeleteThreadFromDataBase($PostID, $UserOfThread, $CurrentUser) ;
$Gen->DeleteFileFromServer($UserOfThread, $CurrentUser, $FilePath);

 

Also a question on global variables: When the user clicks the delete button which the code is on the viewthread.php and the CurrentUser variable comes from the $User variable which is derived from $User = $_SESSION['username']  ;

	<form action='Includes/ClassDeleteThread_ViewThreadPage.php' method='post' enctype'multipart/form-data' onclick='return confirm('Are you sure?')'>
        <input type='submit' name='Delete Thread' value='Delete'/>
     		 <input type='hidden' name='id' value=$number>
     		 <input type='hidden' name='CurrentUser' value=$User>
     		 <input type='hidden' name='UserOfThread' value=$UserOfThread>
     		 <input type='hidden' name='FilePath' value=$imageURL>
          
    	</form>

Is here a way that end users can spoof the $_SESSION['username']  variable?

Thanks

Edited February 9, 2022 by Fishcakes

[ginerjm]

Are you using PDO or MySqlI?  The former is the easier one to use.   That said - read the manual under PDO to see how a query statement would look and then simply do it.  Here is a very brief and simplified spot of code:

//	(write a function that makes your connection.)
$pdo = PDOConnect($dbname);	// this function also sets the db name
$q = "select fld1, fld2, fld3 from tablename where fld1=:argument1 and fld3=:argument3";
$qst = $pdo->prepare($q);
$parms = array('argument1'=>$myvalue,
	'argument3'=>$another_arg);
if (!$qst->execute($parms))
{
//	(handle the error and exit?)
}
//*************
//(now process the query results)

As you can see this is simpler than the way MySqlI uses binds and results to manage prepared queries.  PDO simply asks you to put the needed :parms into the query and then to prepare it.  Then you simply set an array with the values for those parms and execute it whenever you are ready to run the query.

Note:  you only need to make your db connection once in a script unless you are doing something that requires a 2nd connection (??).  Call the connection function once to get a handle and then keep using that handle when you need to run any query.  Don't put the connect call inside a function that gets called repeatedly.  Simply pass the handle to the function as one of its arguments.

Edited February 9, 2022 by ginerjm

[benanamen]

For starters....

The DB connection "should" be passed to the Class using Dependency Injection, not extending Dbh.

The submit button does not need a name attribute

You should use Guard Clauses and get rid of the elses's

[gizmola]

I agree with the prior responses you got.  

We really can't give you any practical advice when your database logic is in a superclass you didn't provide.

Modern PHP apps use composer with a composer.json, an autoloader and PSR-4 compatible namespacing.

Some things you are doing that are anti-patterns:

• Make sure all your methods actually return something. 
• Don't echo out of a method, return the data that you will output.  Use templating (whether it's included template scripts you separate, or a full blown template component class system).

There is so much modern PHP you can do with namespacing, in terms of class design and interfaces, that also encourage you to use typehinting of all your parameters and return values.  Use those features!  I didn't include them in this example, but it's not to late to start using them, even if you are only using these techniques and features in code you are adding or revising.

An example of your code rewritten to implement a "guard clause" pattern mentioned by Benanamen.  Basically, you can just think of these as separating out individual conditions that will cause your function to exit immediately.  You move these to the top of your function/method, rather than having them tacked onto the else.  This is especially helpful when you have larger nested if-then-else conditions, as it becomes increasingly difficult to see what happens.  Typically the condition will use "not" in it, because you will return from the function (or possibly throw an exception) if that condition is true.  Your "happy path" code does not need the condition, as it will be reached only if the conditions are met that allow it to be run.

 

    public function DeleteThreadFromDataBase($PostID, $UserOfThread, $CurrentUser)
    {     
        
        if($UserOfThread !== $CurrentUser) {
            return "failure";
        }

        $sql = "DELETE FROM `Threads` WHERE id = $PostID";
        $result = $this->connect()->query($sql);
        return "success";  
    }

 

Which brings us to Dependency injection.  Here is an article series on Dependency Injection to help you understand what it is and how to implement it.

Here's one example in your code:

public function DeleteFileFromServer($UserOfThread, $CurrentUser,$FilePath)
    { 	if($UserOfThread == $CurrentUser)
    {
        
        $UploadDirectory = "../upload/" ;
        $ThumbnailDirectory = "../upload/Thumbnails/" ;
        $FilePath = $UploadDirectory . $FilePath  ;
        $ThumbnailDirectory = $ThumbnailDirectory . $FilePath  ;
        unlink($FilePath);
        echo "<P> Upload File Deleted" ;
        
        unlink($ThumbnailDirectory);
        echo "<P> Thumbnail File Deleted" ;
    }
    else { 
        echo "<P> You are not the owner of this thread nor a mod" ; 
    } 

 

Let's look at all the problems and lack of DRY-ness:

• You have low level code that handles physical file access
• Although you call this method "DeleteFileFromServer" it can only delete a file if it is in the hardwired path, and you hardwired in that the file must be a thumbnail type.
• Your hardwired relative paths could be replaced with constants or configuration variables that are built upon an application home/base constant you make available throughout your app via a bootstrap, or ideally a front controller.

In the future you have some other file that needs to be deleted, but in this case you will be rewriting all this code again.  Maybe you decide it would be cool if you were to cache images in redis, but now you have all this hardwired logic, so you have to rewrite this routine rather than a generic file deletion routine in a generic file handling class.

What should you have?

A file handling component (although there are already many that you could just be using via composer)  that handles generically storage/saving/removal of files.  With DI you would be "injecting" an object of your file handling class.

Security

At this point, hopefully it follows that security needs to be separated out into its own class.  There are many schemes people use, which often have associated database structure.  A typical one would be the user-role-permission.  Often a  usergroup is added so you can have a pre-set grouping of roles (and associated permissions) rather than having to build that up for each user.  

So you might have these roles in your system:

1. admin
2. moderator
3. user
4. unconfirmedUser
5. guest

You can then have an admin group which includes all 4 roles (and thus all associated permissions).  This also makes it possible to change your scheme around.

For your question, the important thing is that you have a granular permission type.  Not all permissions can be resolved via the relationships as in your example where the rule is that "can-delete-own-thread".  In the future you plan to implement a "can-delete-thread". 

Again, keeping in mind the ideas of Dependency Injection, you want to have a security class you inject, that can:

1. Determine what permissions a user has.
2. Determine if the required criteria is met.

 

Let's reimagine your deletion function with DI and some separation of concerns:

 

public function DeleteThreadFiles($threadId, $currentUser, $securityService, $fileService) {

    $thread = new ThreadModel($threadId);
    if (
        !$securityService->userHasPermission($currentUser, 'can-delete-thread') ||
        !$securityService->userHasPermission($currentUser, 'can-delete-own-thread' ||
        $currentUser !== $thread->getOwner()
    ){
      return false;
    }

    // Happy Path to delete
    foreach ($thread->getThreadFiles as $file) {
        $fileService->deleteFile($file);
    }
}

This shows how some code might be written that handles your envisioned functionality, where the owner AND a moderator or admin could delete a thread, and subsequently, have files deleted as well.

 

General comment and suggestion

Personally, I don't think you should be building in rendering into what is otherwise a persistence model class.  These things have nothing to do with each other.  Look at any sophisticated PHP app, and you will find a separation of concerns, where your "views" aka markup and look and feel is in templates. 

In your case, anytime you want to mess around with the markup, you will be changing a class that is primarily the "model".  You could integrate a template component like twig, and immediately begin to take advantage of the many things twig does for you, not the least of which is structure and reuse of markup.  If you really want hardwired button-type-x code, you can accomplish that in a variety of ways in twig without hardwiring it into a class where it doesn't belong.