Skip to main content
added 867 characters in body
Source Link

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.

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.

added 674 characters in body
Source Link

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?

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 } ?>

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?

added 674 characters in body
Source Link

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 } ?>

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.

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 } ?>
Source Link
Loading