Student status form

Question

I'm programming a system and I want to know if this is the correct way or if there is a better way to do it.

Container:

I have more fields but I only added one to show you the container.

<form method="post" class="form-horizontal" onsubmit="return submitForm();" role="form" id="div-studentStatus"> 
 <div class="form-group">
 <label class="col-sm-3 control-label">Status:</label>
 <div class="col-sm-5 text-primary" id="div-studentStatus">
 <input type="hidden" value="<?php echo $_POST['id']; ?>" id="studentId">
 <select class='form-control input-sm' id="studentStatus">
 <option value="1">Activo</option>
 <option value="0">Inactivo</option>
 </select>
 </div>
 <div class="col-sm-3" align="center"><button type="button" class="btn btn-success btn-block btn-sm" id="statusSaveButton">Guardar</button></div> 
 <div class="col-sm-1">&nbsp;</div>
 </div> 
</form> 

When the user clicks on statusSaveButton, I append a confirmation button (statusSaveButtonYes and statusSaveButtonNo). I only add the action if the user clicks on YES.

$(document).on('click','#statusSaveButton',function() {
 $('#studentStatus').attr('disabled', 'disabled');
 $('#statusSaveButton').prop('disabled', true);
 var studentStatus = $("#studentStatus").val();
 if ($("#studentStatusConfirm").length) {
 $("#studentStatusConfirm").html('<div class="col-sm-12">Are you sure you want to modify the record?&nbsp;&nbsp;<button type="button" class="btn btn-success btn-xs" id="statusSaveButtonYes" style="width:30px">Yes</button>&nbsp;&nbsp;<button type="button" class="btn btn-danger btn-xs" id="statusSaveButtonNo" style="width:30px">No</button></div>');
 $("#studentStatusConfirm").show();
 } else {
 $("#div-studentStatus").append('<div class="form-group" id="studentStatusConfirm"><div class="col-sm-12">¿Estas seguro que deseas modificar el status del alumno?&nbsp;&nbsp;<button type="button" class="btn btn-success btn-xs" id="statusSaveButtonYes" style="width:30px">Si</button>&nbsp;&nbsp;<button type="button" class="btn btn-danger btn-xs" id="statusSaveButtonNo" style="width:30px">No</button></div></div>'); 
 }
}); 
$(document).on('click','#statusSaveButtonYes',function() {
 var studentId = $("#studentId").val();
 var studentStatus = $("#studentStatus").val(); 
 $.post("content/studentsAction.php", {action:"editStudentStatus", studentId:studentId, studentStatus:studentStatus}, function(data){
 if (data != "Successful") {
 $("#studentStatusConfirm").html('<div class="col-sm-12 text-danger">'+data+'</div>');
 $("#statusSaveButtons").show();
 $('#studentStatus').removeAttr('disabled');
 $('#statusSaveButton').prop('disabled', false); 
 } else {
 $("#studentStatusConfirm").html('<div class="col-sm-12 text-success">Success.</div>');
 $("#statusSaveButtons").show();
 $('#studentStatus').removeAttr('disabled');
 $('#statusSaveButton').prop('disabled', false);
 }
 });
}); 

Action (content/studentsAction.php):

I have 8 different actions in this page (I only add one): change status, change level, change name, etc. I control the action with the $_POST["action"] variable.

<?php
$regex_studentId = '/^[0-9]{1,12}$/';
$regex_studentStatus = '/^[01]{1}$/';
if (isset($_POST["action"])) {
 if ($_POST["action"] == "editStudentStatus") {
 if ((isset($_POST["studentId"])) and (isset($_POST["studentStatus"]))) {
 if ((preg_match($regex_studentId, $_POST["studentId"])) and (preg_match($regex_studentStatus, $_POST["studentStatus"]))) {
 $updateStudentStatus = updateStudentStatus($_POST["studentId"],$_POST["studentStatus"]);
 if ($updateStudentStatus) {
 echo "Successful";
 } else {
 echo "Error.";
 }
 } else {
 echo "Error.";
 }
 } else {
 echo "Error.";
 }
} else {
 echo "Error.";
}
?>

Database:

function updateStudentStatus($studentId,$studentStatus) {
 include ("./businesslogic/dbconnection/cfg.php");
 $db = new PDO('mysql:host='.$server.';dbname='.$db,$db_user,$db_password);
 $string = "update students set student_status = :studentStatus where student_id = :studentId";
 $sql = $db->prepare($string);
 $sql->bindParam(':studentId',$studentId);
 $sql->bindParam(':studentStatus',$studentStatus); 
 if ($sql->execute()) {
 $db = null;
 return TRUE;
 } else {
 $db = null;
 return FALSE;
 }
}

• tim: This: echo $_POST['id']; is vulnerable to XSS. Use htmlspecialchars to prevent XSS.

Answer 1

Variable definition

You sometimes define variables in camelCase and sometimes with underscores. I recommend to use one spelling type only.

PHP

If function updateStudentStatus is not part of a class consider developing a database class that handles it. Another option is to add a save function to each of your Model classes. This makes your code easier to read. For more details check mvc pattern.

In the updateStudentStatus function you establish a new database connection with each call. You can avoid it by promoting the function to a proper class and define a global attribute holding your database connection.

JS

In your javascript you have html code. I recommend to either load it with rest of the dom and hide it by default or load it using ajax. I personally compare it with inline css.

HTML

In html form you have an attribute onsubmit. It should be in a javascript file. I compare it with inline css as well.

"QUOTES"

Also see CodeX answer.

As tim mentioned $_POST and other variables not provided by yourself have to be validated and parsed by htmlspecialchars e.g. to avoid XSS.

A bit about security

I've added this section due to mentioned XSS.

Security is a very complex topic and must not be underestimated. There are a lot of options to implement and grant security. What kind of options you choose depends on your time/budget but also kind of project and data that are going to be stored in your database. I kindly ask you to think wise about the data that will be saved in the database and make researches about proper security measures.

There are a lot of PHP Frameworks. I recommend to take a look into some of them as those provide security standards.

Answer 2

This portion of code is very messy and has a lot of unnecessary else { echo error }

<?php
$regex_studentId = '/^[0-9]{1,12}$/';
$regex_studentStatus = '/^[01]{1}$/';
if (isset($_POST["action"])) {
 if ($_POST["action"] == "editStudentStatus") {
 if ((isset($_POST["studentId"])) and (isset($_POST["studentStatus"]))) {
 if ((preg_match($regex_studentId, $_POST["studentId"])) and (preg_match($regex_studentStatus, $_POST["studentStatus"]))) {
 $updateStudentStatus = updateStudentStatus($_POST["studentId"],$_POST["studentStatus"]);
 if ($updateStudentStatus) {
 echo "Successful";
 } else {
 echo "Error.";
 }
 } else {
 echo "Error.";
 }
 } else {
 echo "Error.";
 }
} else {
 echo "Error.";
}
?>

This is much cleaner and should work just the same:

<?php
$regex_studentId = '/^[0-9]{1,12}$/';
$regex_studentStatus = '/^[01]{1}$/';
if (isset($_POST["action"], $_POST["studentId"], $_POST["studentStatus"]) && $_POST["action"] == "editStudentStatus") {
 if ((preg_match($regex_studentId, $_POST["studentId"])) and (preg_match($regex_studentStatus, $_POST["studentStatus"]))) {
 $updateStudentStatus = updateStudentStatus($_POST["studentId"],$_POST["studentStatus"]);
 if ($updateStudentStatus) {
 echo "Successful";
 } 
 else {
 echo "Error"; 
 }
 } 
}

I think this will help your work flow and make the code easier to read, not just for you but for anyone else that might need to work with it.