What is wrong with my SQL Insert code?

Question

I'm struggling with trying to find out why this code isn't working for me. I have tables: albums (albumid, albumname), composers (composerid, composername) and tracks (trackid, tracktitle, albumid, composerid).

When I use my form to add a track and link it to a composer and an album from this:

<form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
<p>Enter the new track:<br />
<textarea name="tracktitle" rows="1" cols="20"></textarea></p>
<p>Composer:    <select name="cid" size="1">
<option selected value="">Select One</option>
<option value="">---------</option>
<?php     while ($composer= mysql_fetch_array($composers)) {
 $cid = $composer['composerid'];
 $cname = htmlspecialchars($composer['composername']);
 echo "<option value='$cid'>$cname</option>\n";} ?>
 </select></p>
 <p>Place in albums:<br />
 <?php      while ($alb = mysql_fetch_array($albs)) {
  $aid = $alb['albumid'];
  $aname = htmlspecialchars($alb['albumname']);
  echo "<label><input type='checkbox' name='albs[]' 
  value='$aid' />$aname</label><br />\n";
  } ?>
  </p>
  <input type="submit" value="SUBMIT" />
  </form>
  <?php endif; ?>

I get this message:

New track added
Error inserting track into album 2:
Track was added to 0 albums.

The php code that precedes the form is:

if (isset($_POST['tracktitle'])): 
 // A new track has been entered
 // using the form.
$tracktitle = mysql_real_escape_string($tracktitle);
$cid= $_POST['cid'];
$tracktitle = $_POST['tracktitle'];
$albs = $_POST['albs'];
if ($cid == '') {
exit('<p>You must choose an composer for this track. Click

"Back" and try again.

');}

$sql = "INSERT INTO tracks (tracktitle)
 VALUES ('$tracktitle')" ;
if (@mysql_query($sql)) {
echo '<p>New track added</p>';
 } else {
exit('<p>Error adding new track' . mysql_error() . '</p> 
echo mysql_error() ');}
$trackid = mysql_insert_id();
if (isset($_POST['albs'])) {
$albs = $_POST['albs'];
} else {
$albs = array();
}
$numAlbs = 0;
foreach ($albs as $albID) {
$sql = "INSERT IGNORE INTO tracks (trackid, albumid, 
composerid) VALUES " .
"($trackid, $albs, $cid)";
if ($ok) {
  $numAlbs = $numAlbs + 1;
} else {
  echo "<p>Error inserting track into album $albID: " .
      mysql_error() . '</p>';    }}?>
<p>Track was added to <?php echo $numAlbs; ?> albums.</p>

<?php
else: // Allow the user to enter a new track
$composers = @mysql_query('SELECT composerid, composername 
FROM composers');
if (!$composers) {
 exit('<p>Unable to obtain composer list from the database.</p>');
}
$albs = @mysql_query('SELECT albumid, albumname FROM albums');
if (!$albs) {
  exit('<p>Unable to obtain album list from the database.</p>');}?>

I keep searching for why this is failing and I keep hitting brick walls. I also know that at present it's not very secure which will be the next thing I sort out. I just want to get the actual function working first.

You are still using brain-dead string manipulation for SQL. There is no point addressing any other issues until this is fixed IMOHO, as in fixing this fundamental flaw -- and understanding why it is a flaw -- is a crucial step to a better development process. And yes, your code is vulnerable to attack. — user166390
– user166390, Commented Nov 30, 2010 at 20:50
Give your code some love. My head explodes when I try to read that mess. — Mark Baijens
– Mark Baijens, Commented Nov 30, 2010 at 20:51
Some pretty harsh comments here... He did acknowledge the code is insecure and say "I just want to get the actual function working first", guys. :-/ — stealthyninja
– stealthyninja, Commented Nov 30, 2010 at 21:13

stealthyninja · Accepted Answer · 2010-11-30 21:04:58Z

2

@paj: Change

if ($ok) {

to

if (mysql_query($sql)) {

-

I also suggest you update your SQL statements to

$sql = "INSERT INTO tracks (tracktitle) VALUES ('" . $tracktitle . "')";

$sql = "INSERT IGNORE INTO tracks (trackid, albumid, composerid) VALUES (" . $trackid . ", " . $albID . ", " . $cid . ")";

answered Nov 30, 2010 at 21:04

stealthyninja

10.4k11 gold badges56 silver badges61 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

paj Over a year ago

I don't mind harsh comments so much. I do find some comments more helpful than others. Thanks for your help. I see the Values in my Insert code were just the names of the table fields. I've changed it to what you suggested. Though I don't get the error message after adding a new track, looking at the tracks table I see the new tracktitle listed but the albumid and composerid are at 0. Will investigate further. Thanks

spanky · Accepted Answer · 2010-11-30 20:54:19Z

1

Looks to me like $ok doesn't exist except in the if ($ok) { line. It needs to be defined somewhere prior, otherwise it will always read false because it doesn't exist.

Actually you can skip the $ok which doesn't exist and put in if (@mysql_query($sql)) { for that line like you have above. I do have to agree with the comments that the code needs some love, but if you want to know why it's breaking down, it appears this is why.

edited Nov 30, 2010 at 20:54

answered Nov 30, 2010 at 20:48

spanky

1,4992 gold badges12 silver badges22 bronze badges

2 Comments

paj Over a year ago

Thanks, very helpful. I started with tutorials/books and then adapted one for my own project to try and understand how it works. I guess that's why I overlook bits like this. Thank you.

spanky Over a year ago

No problem. Good luck learning. Since you are a new stackoverflow user, you should also start off on the right foot here by "accepting" whichever answer is most helpful. Glad we could help.

Collectives™ on Stack Overflow

What is wrong with my SQL Insert code?

2 Answers 2

1 Comment

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related