2
\$\begingroup\$

I hope this is not a really bad question for a first time here beginner.

This piece of code selects from the left table and will list the content in the right hand table. This is a working code but I would like to see how a professional would protect and make it faster.

Any suggestion (with some code) would be appreciated. Thanks a lot

PS: There is also a little glitch with it: after deleting it lose the selected item on the right list.

<?php include("db_con1.php");?>

<html>
<head>
</head>
<body>
<form method="post" action="test.php">

<div id="left">
<?php
  $queryl = $pdo->prepare('SELECT id, name FROM test1 ORDER BY name ASC');
  $queryl->execute();
?>

<ul>

  <?php foreach ($queryl as $i => $rowl) { ?>

  <li>
   <?php if ($i) {?>
  <input name="checkbox1_del[]" id="test_<?php echo $i ?>" type="checkbox" value="<? echo $rowl['id']; ?>"/>
  <label for="test_<?php echo $i ?>">
   <a href="test1.php?gid=<?php echo $rowl['id']; ?>"><?php echo $rowl['name']; ?></a>
  </label>
 </li>
  <?php } ?>
 </ul>
</div>

<div id="right">

<?php
  if(isset($_GET['gid'])) {
   $gid=$_GET['gid'];    
   $queryr = $pdo->prepare('SELECT test3.name FROM test1, test2, test3 WHERE test1.id=test2.groupid AND test3.id=test2.peopleid AND test1.id='.$gid.' ORDER BY test3.name ASC');
   $queryr->execute();
  }
?>

<ul>

  <?php foreach ($queryr as $i => $rowr) { ?>

    <li>
      <?php if ($i) {?>
      <input name="checkbox2_del[]" id="test_<?php echo $i ?>" type="checkbox" value="<? echo $rowr['id']; ?>"/>
      <label for="test_<?php echo $i ?>"><?php echo $rowr['name']; ?></label>
    </li>
  <?php } ?>
</ul>
</div>

<input type="submit" name="del" value="Delete the selected items">
</form>

<?php
if (isset($_POST['del'])) {
echo "Don't delete:)";
  for ($c = 0; $c < count($_POST['checkbox1_del']); $c++){
    $checkbox1_del = $_POST['checkbox1_del'][$c];
    $sql = 'UPDATE test1 SET status=0, log="'.date("Y-m-d").'"WHERE id='.$checkbox1_del;
    echo $sql;
    $query = $pdo->prepare($sql);
    $query->execute();
  }

  for ($c = 0; $c < count($_POST['checkbox2_del']); $c++){
    $checkbox2_del = $_POST['checkbox2_del'][$c];
    $sql = 'UPDATE test2 SET status=0, log="'.date("Y-m-d").'"WHERE id='.$checkbox2_del;
    echo $sql;
    $query = $pdo->prepare($sql);
    $query->execute();
   }

    if($query){
      echo "<meta http-equiv=\"refresh\" content=\"0;URL=test1.php\">";
     }
 }
?>

</body>
</html>

Revise 1: This is a extracted part of my program, however here is the definition of this function:

Having 3-5000 contact, to send emails them easier to group them to Distribution list. So this part of the program provide 3 tables:

Distribution list | Members of selected group | People

you can make group names and can add people into it.

So basic functions that I wrote for this part:

  • new distribution list can be added
  • By clicking on any Distribution list you'll get the Members table ($_GET)
  • Also by clicking on Distribution list should be BOLD (class="bold") as highlighted
  • Selecting any members in the member list can be deleted (checkbox2_del())
  • From people list anyone can be added to the list

This is what I tried to write and this test shows that basic steps of it. I just didn't want to overload anybody to read through the whole script. If anyone would like to see I happy to replace with it. Thanks

\$\endgroup\$

1 Answer 1

Reset to default
5
\$\begingroup\$

You are using prepared SQL statements, but not using parameterised queries in them, so you're still wide open to SQL injections. This is how you should be doing it:

$gid = $_GET['gid'];    
$queryr = $pdo->prepare('SELECT test3.name FROM test1, test2, test3 WHERE test1.id = test2.groupid AND test3.id = test2.peopleid AND test1.id = :gid ORDER BY test3.name ASC');
$queryr->bindParam(':gid', $gid, PDO::PARAM_STR);
$queryr->execute();

Assuming that your $gid is supposed to be numeric, you should probably extract it this way too:

if(is_numeric($_GET['gid'])) {
  $gid = (int)$_GET['gid'];
  // ...
  $queryr->bindParam(':gid', $gid, PDO::PARAM_INT);

The same applies to your later queries that are being generated from $_POST.

This segment also seems to be missing a set of braces. If it works, then it's likely only a coincidence. if without braces is hard enough to read in just PHP, but if you drop out of PHP, I honestly have no idea how PHP will react.

  <?php foreach ($queryl as $i => $rowl) { ?>

  <li>
   <?php if ($i)?> <!-- **** BRACES MISSING HERE? **** -->
  <input name="checkbox_del[]" id="test_<?php echo $i ?>" type="checkbox" value="<? echo $rowl['id']; ?>"/>
  <label for="test_<?php echo $i ?>">
   <a href="test1.php?gid=<?php echo $rowl['id']; ?>"><?php echo $rowl['name']; ?></a>
  </label>
 </li>
  <?php } ?>

There is a larger problem with this script in general. Mixing logic with the output can cause design problems. Ideally you would load all your data at the top, then generate your page all at once. I don't know how big a system this is, but in general it's much easier to manage the system as a whole if you structure your pages this way. What happens if you want to conditionally show something at the top of the page, but the condition isn't generated till later when you are processing data?

Eg, you might end up with something like the following:

<?php 
include("db_con1.php");
$queryl = $pdo->prepare('SELECT id, name FROM test1 ORDER BY name ASC');
$queryl->execute();

$lists = array();
while (($rowl = $queryl->fetch(PDO::FETCH_ASSOC)) !== false) {
  $lists[] = $rowl;
}

// And so on for all the data you need to load
?>
<html>
<head>
<!-- And so on... -->

It seems your right bar and left bar aren't associated directly, but if they were then you could put more in that while loop to load stuff into $rowl before adding it to $lists. You can then just iterate over this array in your code. All your SQL is at the top of the file and easy to change, all your presentation is at the bottom of the file and will read a lot nicer too, with just some loops and some echo's.

\$\endgroup\$
3
  • \$\begingroup\$ +1. Thanks very much your effort and help whit this code. I am going to explain about this program a bit more in the question so people might give more ideas. \$\endgroup\$
    – TryHarder
    Commented Aug 31, 2011 at 10:00
  • \$\begingroup\$ could you show me how would you design the whole page? So I could learn from it and I would redesign my other codes. Thanks \$\endgroup\$
    – TryHarder
    Commented Aug 31, 2011 at 10:28
  • 1
    \$\begingroup\$ @Andras: I use ->fetch explicitly in my code because I'm not sure what the iterator returns by default. If it returns a standard associative array and not some class that acts like one, then feel free to use that too. I just wasn't sure, so didn't use it. \$\endgroup\$
    – Matthew Scharley
    Commented Aug 31, 2011 at 10:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.