2

I use php GET to check for an ID in an URL, then retrieve tekst from a database that matches that ID. This is the snippet of code that does that:

function getLyric() {
$id = (int)$_GET['id'];
$query = mysql_query("SELECT * FROM lyrics WHERE id = ".$id."") or die(mysql_error());
if ($row = mysql_fetch_assoc($query)) { ?>
    <h1><?= $row['title']; ?> lyrics</h1>
    <h2><?= $row['author']; ?></h2>

    <pre><?= $row['lyrics']; ?></pre>
    <?php }
else { header("HTTP/1.1 404 Not Found"); header("Status 404 Not Found"); }
}

And this is how the page looks where I call this function:

<?php include 'includes/header.php' ?>
<?php getLyric(); ?>
<?php include 'includes/footer.php' ?>

Unfortunately the 404 part in the function does not work. I get the following warning.Warning: Cannot modify header information - headers already sent by [redacted] functions.php on line 61. Line 61 is the 404 part; "else { header}~".

I've learnt from Stackoverflow that this is because the <html><head> part is already outputed on the page, so therefore I can't change this anymore. This is correct?

Previously when I didn't have the else~ part I just got a blank page. How can I properly send a 404 in this case? If I can't what is the closest option?

user1333327
  • 795
  • 1
  • 6
  • 11
  • 1
    FYI, your code is **wide open** to SQL injection, and you **will be hacked** if you haven't been already. Learn to do prepared queries with PDO to avoid this problem entirely. – Brad Apr 23 '12 at 20:02
  • 3
    Typecasting a string as an int will just return `0`. Unless requesting an id 0 from his database magically opens it up to invasion, I don't think it's all that threatening. Now, if he's in the habit of using strings directly in the same manner, I can see where it will be a problem. – MetalFrog Apr 23 '12 at 20:05
  • Thanks, I will look into it. What does PDO stand for? @MetalFrog: I have my strings run through a `mysql_real_escape_string` first. I've read that that will do. – user1333327 Apr 23 '12 at 20:06
  • 1
    @Brad: No, the input is sanitized with a typecast. – netcoder Apr 23 '12 at 20:07
  • @user1333327: PHP Data Objects. – netcoder Apr 23 '12 at 20:07
  • 2
    It's sufficient. PDO is short for PHP Data Objects, and it's a generic interface class to interact with databases. It's not a magic pill as Brad suggests, but it certainly makes things easier if you're using multiple DBMs. http://us.php.net/PDO – MetalFrog Apr 23 '12 at 20:08
  • @user1333327, MetalFrog is correct. I missed the typecasting you do there. And no, it isn't a magic pill, but using prepared statements does avoid the issue of first-order direct SQL injection. I don't intend to suggest it will fix all of your security problems. – Brad Apr 23 '12 at 20:34

7 Answers7

6

You just need to separate the logic from the presentation:

function getLyric() {
    $id = (int)$_GET['id'];
    $query = mysql_query("SELECT * FROM lyrics WHERE id = ".$id."") 
                 or die(mysql_error());
    return mysql_fetch_assoc($query);
}

...then:

<?php
$row = getLyric();

if (!$row) {
    header("HTTP/1.1 404 Not Found");
    exit;
}

include 'includes/header.php' 
?>

<h1><?= $row['title']; ?> lyrics</h1>
<h2><?= $row['author']; ?></h2>

<pre><?= $row['lyrics']; ?></pre>

<?php 
include 'includes/footer.php'
?>
netcoder
  • 66,435
  • 19
  • 125
  • 142
  • Thank you, this is an excellent option. I don't like how I have to have a piece of code above my `` though ;). Oh well. – user1333327 Apr 23 '12 at 20:36
  • 1
    @user1333327: Separating logic from presentation is considered a best practice and is unavoidable in larger systems. This above is a short example, but you'd usually want to store the logic and the presentation in *two separate files*, and use includes as it makes them reusable, e.g.: `lyric.php` for logic, and `lyric.phtml` for presentation. – netcoder Apr 23 '12 at 20:42
2

Once you have sent out HTML code you cannot change the header. The header tells the browser what to do with the requested page. If it has already received HTML, it assumes content-type:text\html and begins rendering the page. At that point your header are defined and you can't go back.

2

No you can't, the header is the first thing that goes out, so once you send text back, the header is already out the door so to speak.

You probably will want to run your query before including your header, run the check to make sure data was returned, then print.

function getLyric() {
  $id = (int)$_GET['id'];
  $query = mysql_query("SELECT * FROM lyrics WHERE id = ".$id."") or die(mysql_error());
  if ($row = mysql_fetch_assoc($query)) {
    include 'includes/header.php'; 
  ?>
      <h1><?= $row['title']; ?> lyrics</h1>
      <h2><?= $row['author']; ?></h2>

      <pre><?= $row['lyrics']; ?></pre>
      <?php }
  else { header("HTTP/1.1 404 Not Found"); }
}

<?php getLyric(); ?>
<?php include 'includes/footer.php' ?>
Developer
  • 2,021
  • 12
  • 11
  • Thanks for your reply, this is what I already feared. So what is the best way to handle this? Perhaps a 301 redirect? Or should I completely redesign my structure (e.g. all PHP stuff above the html header) – user1333327 Apr 23 '12 at 20:08
  • I added some sample code above as to how you can do it. You'd need to include the header once you have already checked your result. Depending on how this is implemented it might be a bit different from the other pages. Another way would be to have a function which checks if the lyrics exist before the header, and only returns the 404 if it doesn't. – Developer Apr 23 '12 at 20:10
0

You can't send anymore headers once they have been sent. What you could do is enable Output Buffering which basically waits until the script has been executed before outputting stuff. This will allow you to set headers at any point in your script, but has some minor consequences depending on your script and you should read up on it before blindly enabling it.

Also, read up on SQL inejections. Your site is extremely vulnerable to being exploited.

Alex Turpin
  • 46,743
  • 23
  • 113
  • 145
0

You can enable output buffering in your scripts.

Example:

<?php
ob_start();
echo 'Hi';
header('Location: http://someurl');
exit;
?>

Please make sure what output buffering is before using it.

Manish
  • 4,903
  • 1
  • 29
  • 39
0
ob_start();
header("location:somepage.php");
ob_end_flush();

Actually,If you have output buffrening On,nothing will be sent before you flush it.Not even header.

H2O
  • 39
  • 6
  • That's not entirely true. The output buffer has a limit (it typically defaults to 4096 bytes), once you reach it, it's flushed. Also, this is just a workaround, it doesn't really resolve the issue. – netcoder Apr 23 '12 at 20:11
0

As you can see, a 404 can't be sent when contents have already been sent. You have 2 options, IMO

  • Modify your code to enable output buffering. This essentially means that all you echo out or the contents outside the PHP tags go into a buffer and are not sent to the client. Read about it here

  • modify the function to give out the Data and not the UI. This means the function getLyrics() should give out the concerned row. Before you include any header, be sure to check if the the function says the record exists. Else, you can always send out a 404. This is actually recommended and is a good design because it essentially separates the logic from the UI.

UltraInstinct
  • 43,308
  • 12
  • 81
  • 104