What's wrong in this code?

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
MicroBoy
Forum Contributor
Posts: 112
Joined: Sat Mar 14, 2009 5:16 pm

What's wrong in this code?

Post by MicroBoy »

Can anyone tell me what's wrong in this code because when I try to delete "news" It says that the "news" deleted, but still the "news" is in the table:

Code: Select all

<html>
<head>
<link rel="stylesheet" type="text/css" href="menu/stili.css" />
</head>
<body>
<?php
include ('menu/menu.php');
$id = $_GET['id'];
if (empty($id)) 
{
?>
 
<TABLE class="col_top" width="800" align="left" cellspacing="0" cellpadding="0" border="0">
  <TR>
<TD valign="left">
 
<?php
if (!$lidhja)
  {
  die('Nuk Mund Të Konektohet: ' . mysql_error());
  }
 
mysql_select_db("$databaza", $lidhja);
 
$result = mysql_query("SELECT * FROM lajmet ORDER BY ora DESC");
 
echo "<table border='1'>
<tr>
<th>Emri</th>
<th>Lajmi</th>
<th>Data&Ora</th>
<th>Fshi</th>
</tr>";
 
$id = $_GET['id'];
 
while($row = mysql_fetch_array($result))
  {
  echo "<tr>";
  echo "<td>" . $row['emri'] . "</td>";
  echo "<td>" . $row['lajmi'] . "</td>";
  echo "<td>" . $row['ora'] . "</td>";
  echo "<td><a href=" . $url . "/lexo.php?id=" . $row['id'] . "><img src='menu/el.png' /></a></td>";
  echo "</tr>";
  }
echo "</table>";
}
 
if (isset($id)) 
{
mysql_query("DELETE FROM lajmet WHERE id='$id'");
echo "</br><center><strong>The News Deleted.<strong></br></center>";
}
 
mysql_close($lidhja);
 
include ("menu/posht.php");
?>
 </TR>
</TABLE>
</body>
</html>
User avatar
Apollo
Forum Regular
Posts: 794
Joined: Wed Apr 30, 2008 2:34 am

Re: What's wrong in this code?

Post by Apollo »

MicroBoy wrote:

Code: Select all

mysql_query("DELETE FROM lajmet WHERE id='$id'");
echo "</br><center><strong>The News Deleted.<strong></br></center>";
You're always printing "The News Deleted", regardless of whether it was actually deleted or not. You're not using the result from mysql_query at all!

Try this instead:

Code: Select all

if (mysql_query("DELETE FROM lajmet WHERE id='$id'"))
{
  echo "News deleted";
}
else
{
  echo "Epic fail :-(";
}
Oh, and by the way: deleting rows based on an ID directly specified by the user seems tricky business to me. Besides the fact that you don't escape anything, so your code is extremely vulnerable to SQL injections.
MicroBoy
Forum Contributor
Posts: 112
Joined: Sat Mar 14, 2009 5:16 pm

Re: What's wrong in this code?

Post by MicroBoy »

Now I'm using this, but it reply "Epic fail :-(" why?

Code: Select all

if (isset($id)) 
{
mysql_query("DELETE FROM lajmet WHERE id='$id'");
 
if (mysql_query("DELETE FROM lajmet WHERE id='$id'"))
{
echo "News deleted";
}
else
{
echo "Epic fail :-(";
}
}
Based on what do you suggest to delete the rows?
-------------------------------------------------------------------
By the way:
When I use this code everything it's ok, the news delete, but I don't want to use this because when I delete a news the message and the table are showed in the same time, I want that when I click Delete, to show just the message not the table:

Code: Select all

<html>
<head>
<link rel="stylesheet" type="text/css" href="menu/stili.css" />
</head>
<body>
<?php
include ('menu/menu.php');
?>
 
<TABLE class="col_top" width="800" align="left" cellspacing="0" cellpadding="0" border="0">
  <TR>
<TD valign="left">
 
<?php
if (!$lidhja)
  {
  die('Nuk Mund Të Konektohet: ' . mysql_error());
  }
 
mysql_select_db("$databaza", $lidhja);
 
$result = mysql_query("SELECT * FROM lajmet ORDER BY ora DESC");
 
echo "<table border='1'>
<tr>
<th>Emri</th>
<th>Lajmi</th>
<th>Data&Ora</th>
<th>Fshi</th>
</tr>";
 
$id = $_GET['id'];
 
while($row = mysql_fetch_array($result))
  {
  echo "<tr>";
  echo "<td>" . $row['emri'] . "</td>";
  echo "<td>" . $row['lajmi'] . "</td>";
  echo "<td>" . $row['ora'] . "</td>";
  echo "<td><a href=" . $url . "/lexo.php?id=" . $row['id'] . "><img src='menu/el.png' /></a></td>";
  echo "</tr>";
  }
echo "</table>";
 
if (isset($id)) 
{
mysql_query("DELETE FROM lajmet WHERE id='$id'");
echo "</br><center><strong>News Deleted.<strong></br></center>";
}
 
mysql_close($lidhja);
 
include ("menu/posht.php");
?>
 </TR>
</TABLE>
</body>
</html>
Last edited by MicroBoy on Mon Mar 23, 2009 12:10 pm, edited 1 time in total.
User avatar
jayshields
DevNet Resident
Posts: 1912
Joined: Mon Aug 22, 2005 12:11 pm
Location: Leeds/Manchester, England

Re: What's wrong in this code?

Post by jayshields »

Remove the first myql_query() call. You'd be better off refactoring your code to include some debugging info, such as using mysql_query($query) or die(mysql_error()). Also, you should really sort out your security vulnerabilities as Apollo said.
MicroBoy
Forum Contributor
Posts: 112
Joined: Sat Mar 14, 2009 5:16 pm

Re: What's wrong in this code?

Post by MicroBoy »

When I used "die(mysql_error())" it said "No database selected" then I use:

Code: Select all

mysql_select_db("$databaza", $lidhja);
 
if (isset($id)) 
{
if (mysql_query("DELETE FROM lajmet WHERE id='$id'"))
{
echo "News deleted";
}
else
{
die(mysql_error());
}
}
But I'm still not understanding why it didn't select the database from the code upper, because I used "mysql_select_db("$databaza", $lidhja);"?

jayshields wrote:Also, you should really sort out your security vulnerabilities as Apollo said.
How to do that? Any suggest?
User avatar
jayshields
DevNet Resident
Posts: 1912
Joined: Mon Aug 22, 2005 12:11 pm
Location: Leeds/Manchester, England

Re: What's wrong in this code?

Post by jayshields »

Again, use "or die(mysql_error())" after the mysql_select_db() call. Don't use quotes around your database name variable. Have you actually established a database connection and saved the handle to $lidhja?

Use mysql_real_escape_string() to safe-guard against SQL injection.

These points I am raising are extremely common. I have brought them up about 3 times in the last couple of days. Simply searching Google or reading the PHP manual would have told you all this and more.
MicroBoy
Forum Contributor
Posts: 112
Joined: Sat Mar 14, 2009 5:16 pm

Re: What's wrong in this code?

Post by MicroBoy »

Thnx a lot for help. I'm still curious to know why I have to use "mysql_select_db("$databaza", $lidhja);" twice?
jayshields wrote:Have you actually established a database connection and saved the handle to $lidhja?
Yes I did.
User avatar
jayshields
DevNet Resident
Posts: 1912
Joined: Mon Aug 22, 2005 12:11 pm
Location: Leeds/Manchester, England

Re: What's wrong in this code?

Post by jayshields »

MicroBoy wrote:I'm still curious to know why I have to use "mysql_select_db("$databaza", $lidhja);" twice?
You don't. Your script doesn't show you doing that either. Have you posted your full script?
MicroBoy
Forum Contributor
Posts: 112
Joined: Sat Mar 14, 2009 5:16 pm

Re: What's wrong in this code?

Post by MicroBoy »

Look, when I delete the code in the 49 line, the news doesn't delete. Why the 23 line doesn't work for all the code but I have to use the same code twice?

Code: Select all

<html>
<head>
<link rel="stylesheet" type="text/css" href="menu/stili.css" />
</head>
<body>
<?php
include ('menu/menu.php');
$id = $_GET['id'];
if (empty($id))
{
?>
 
<TABLE class="col_top" width="800" align="left" cellspacing="0" cellpadding="0" border="0">
  <TR>
<TD valign="left">
 
<?php
if (!$lidhja)
  {
  die('Nuk Mund Të Konektohet: ' . mysql_error());
  }
 
mysql_select_db("$databaza", $lidhja) OR die(mysql_error());
 
$result = mysql_query("SELECT * FROM lajmet ORDER BY ora DESC");
 
echo "<table border='1'>
<tr>
<th>Emri</th>
<th>Lajmi</th>
<th>Data&Ora</th>
<th>Fshi</th>
</tr>";
 
$id = $_GET['id'];
 
while($row = mysql_fetch_array($result))
  {
  echo "<tr>";
  echo "<td>" . $row['emri'] . "</td>";
  echo "<td>" . $row['lajmi'] . "</td>";
  echo "<td>" . $row['ora'] . "</td>";
  echo "<td><a href=" . $url . "/lexo1.php?id=" . $row['id'] . "><img src='menu/el.png' /></a></td>";
  echo "</tr>";
  }
echo "</table>";
}
 
mysql_select_db("$databaza", $lidhja) OR die(mysql_error());
 
if (isset($id))
{
if (mysql_query("DELETE FROM lajmet WHERE id='$id'"))
{
echo "News deleted";
}
else
{
die(mysql_error());
}
}
 
mysql_close($lidhja);
 
include ("menu/posht.php");
?>
 </TR>
</TABLE>
</body>
</html>
p.s. I'm just curious
User avatar
Apollo
Forum Regular
Posts: 794
Joined: Wed Apr 30, 2008 2:34 am

Re: What's wrong in this code?

Post by Apollo »

MicroBoy wrote:Based on what do you suggest to delete the rows?
1. An escaped id, not just a plain id from the URL. What do you think will happen if some smartass fills in id=1'; DELETE * FROM lajmet; ?
Use mysql_real_escape_string. Or in the case of a numeric ID, use intval.

2. Not an id that can be freely chosen by the user. Someone can call the delete script with id=1, id=2, etc. Why would you allow that? So verify / validate the id.
sujithtomy
Forum Commoner
Posts: 46
Joined: Tue Mar 24, 2009 4:43 am

Re: What's wrong in this code?

Post by sujithtomy »

Hello,

why don't you use mysql_affected_rows() to know whether there is any rows get affected due to query :)

------------
Sujith
Post Reply