How clean is my close_account code?

Question

Hello i'm coding a social site and I need to know how clean this code is. I read other answers people told me from my previous questions like this and used it as guidelines. So now I wanna know if anything could be improved. Thank you.

<?php 

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
error_reporting(-1);

include 'includes/header.php';

$userLoggedIn = $_SESSION['thesocial_username'];

$post_id = isset($_POST['cancel']) ? header("Location: settings.php") : '';

if(isset($_POST['close_account'])) {
    
    $close_query = $con->prepare('UPDATE users SET user_closed = "yes" WHERE username = ?');
    $close_query->bind_param("s", $userLoggedIn);
    $close_query->execute();

    session_destroy();
    header("Location: register.php");
}

?>

<center>
<div class="close_column close_account">
    
    <h4>Close Account</h4>
    Are you sure you want to close your account ?<br><br>

    Closing your account will hide your profile and activity from users.<br><br>
    You can reopen your account at anytime by logging in.<br><br>

    <form action="close_account.php" method="POST">
        
        <input type="submit" name="close_account" id="close_account" value="Yes, Close my account.">
        &nbsp;&nbsp;
        <input type="submit" name="cancel" id="update_details" value="Nevermind.">

    </form>
</div>
</center>

Answer 1

The code is overall good basic PHP and above the average. However, I would like it more structured.

For example, why you are setting the error reporting manually in this file? Apparently you have sort of a bootstrap file as includes/header.php, why not to put these lines there?

Besides, i don't see any point in the line assigning $post_id variable. What does a post id to do with deleting an account? And yes, such a syntax structure looks embarrassing.

It is also a good rule of thumb to follow Location header with an explicit exit call. Although it is not critical in this particular case, sometimes the lack of the script termination could pose a serious threat, as the header itself doesn't stop the script execution.

Some notes on the database structure

• The computer world has its own representation for "yes" and "no", it's 1 and 0 respectively. So consider making the field of tinyint type instead of varchar.
• it is considered a good practice to address all database entities by the unique id. It's better to store the user id in the session and use it for all the database operations.

The code structure

One big issue I overlooked at first is the structure of your script. It seems that includes/header.php is not just a bootstrap file but it also starts some HTML output. that's a big NO. An HTTP header such as Location cannot be sent after HTML. Your PHP lets you to do it due to some permissive setting, but it is nevertheless wrong, from the logical and technological points of view. It just makes no sense to start any output when your script is not going to display anything but would only redirect to another page.

Split your includes/header.php file into two: bootstrap.php and header.php. The former should contain all the configuration options such as database connection, error reporting etc, but not a byte should it output. And the latter would contain all the HTML.

Than make your code like this

<?php 

include 'includes/bootstrap.php';

$id= $_SESSION['user_id'];

if(isset($_POST['close_account']))
{
    $stmt = $con->prepare('UPDATE users SET user_closed = 1 WHERE id = ?');
    $stmt->bind_param("s", $id);
    $stmt->execute();

    session_destroy();
    header("Location: register.php");
    exit;
}
include 'includes/header.php';
?>
<center>

Answer 2

To add to "Your Common Sense" answer:

1. I would suggest that the next step would be to separate your HTML and PHP code.

i.e. moving out the logic for account closure into its own class since you might want to use that logic elsewhere. (DRY Principle)

2. It's best to let JS/HTML handle redirects too since that is something related to the front end facing application.

3. In a larger application, speed is of the essence and every data byte may count, due to which I would suggest declaring variables only when and where you need them.

i.e.

• $userLoggedIn is used within an if statement, yet it was declared outside of an if statement.
• $userLoggedIn is used only once. The question to ask is - does having this variable improve the readability of my code? If not, then just use $_SESSION['thesocial_username'].