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>

• mickmackusa: I don't support that usage of a ternary variable declaration which contains a page redirection. I mean, you aren't setting a value to $post_id so I find it to be an inappropriate syntax. There's not much to review here.
• slepic: Definitely use if for that. Actually the $post_id variable should not be there at all. Further, you should exit the script after sending location header. I can close the account and get redirected to settings.php by posting cancel=1&close_account=1. The script would actually send two location headers, I assume the browser will redirect to the adress of the first of them.
• user13477176: Ohhhh lol. I was confused when everyone was talking about $post_id. I just took that code and threw in it there. I honestly don't know why but I was up for a long time trying to fix the code and I must've screwed up. Thanks a lot though.

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>

• Nigel Ren: I would just like to add about the "yes" and "no" part - it also helps reduce the instances where values become YES, Yes or even yes as well as many other combination, these can then fail to get recognised as the case does not match. 1 and 0 are more difficult to get wrong.
• user13477176: I was confused when everyone was talking about $post_id. I just took that code and threw in it there. I honestly don't know why but I was up for a long time trying to fix the code and I must've screwed up. Thanks a lot though.
• user13477176: Ok I'm going to recreate my project and do it the best way I can because I was following a tutorial and it was a 2019/2020 tutorial but it apparently it was loaded with bad habits and practices. He said he wanted to do it that way to make it simple for beginners which makes zero sense.
• user13477176: Would it be ok if I just started each page with html or php instead of having includes folders/files ?

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'].

• Your Common Sense: To be honest, I find all these suggestions but a cargo cult and scarcely relevant to the code posted. PHP and HTML already separated here. And using classes is irrelevant to such separation. (2) is unclear. And there is absolutely no difference whether you declared a variable or not.
• user3402600: 1. In the post we have PHP logic and HTML code in the same file, what I am suggesting is to extract the logic into a separate file. 2. As part of DDD, the application layer accepts user commands and presents data back to the user; UI/FrontEnd determines how data is handled [and where redirections should take place.] I know this post is not about DDD but that is what I personally follow. 3. PHP has issues with memory i.e. this is an example of it - bugs.php.net/bug.php?id=76982 hence my point (3) - declare things only when you need them.
• Your Common Sense: 1. in the same file but perfectly separated. 2. DDD is not about backend/front end at all. 3. I don't see how this link is even remotely related to a variable declaration.
• user3402600: 1. If we want to use the same logic elsewhere we should just copy and paste the contents from this file to another? What if I want to write a test for this file, with a separate class, all I need to do is test that specific class 2. Do you expect backend to tell client applications to which pages they should be redirecting users to, i.e. Stripe API? 3. That link was showing one example, there are numerous others.