-3

I'm trying to build a forum with PHP and PDO at the moment and I have a link which takes you to the page for a category with the categories ID in the URL (eg. WEBSITE/category.php?id=1). When I get there, I want to display the name of the category you are looking at using this $_GET information, but it won't seem to do it for me. Here is what I've got:

<?php

include 'dbconfig.php';
include 'header.php';
$sql = "SELECT cat_id, cat_name, cat_description FROM categories WHERE cat_id = " . $_GET['id'];
$query = $DB_con->prepare($sql);

$query->execute();

$numRows = $query->fetchColumn();

if (!$query) {
    echo 'Something went wrong whilst getting the category from the database.';
} else {
    if ($numRows == 0) {
        echo 'Sorry, this category does not exist';
    } else {
        while($catRow = $query->fetch(PDO::FETCH_ASSOC)){
            echo $catRow['cat_name'];
        }
    }
}

include 'footer.php';
?>

So as you can see, I have tried to make a while loop that creates an array using PDO::FETCH_ASSOC allowing me to print category details, but when I go to the page nothing shows up except the header.php and the footer.php. There also aren't any errors that come up. Can anybody see what I'm doing wrong? Or let me know if there's information that I have left out. Thanks.

Cal Courtney
  • 1,279
  • 2
  • 10
  • 22
  • 3
    Your code is vulnerable to SQL injection, you need to fix this. – Enstage May 31 '17 at 11:29
  • @Enstage How? I though SQL Injection was impossible when using PDO? – Cal Courtney May 31 '17 at 11:30
  • 1
    @CalCourtney Not inherently. You still need to bind your parameters. – ʰᵈˑ May 31 '17 at 11:31
  • if you use it correctly, it is hard to do – Jelmergu May 31 '17 at 11:31
  • you can simply use `(int)` passed for the GET array, you won't need a prepared statement for it. – Funk Forty Niner May 31 '17 at 11:31
  • type casting might help avoid using prepared statements – Rotimi May 31 '17 at 11:32
  • 1
    Fetching data twice create problem!! You already fetch result using `fetchColumn` – Saty May 31 '17 at 11:32
  • @ʰᵈˑ With ->execute(array(':id' => $_GET['id'])) ? Because I tried that and had the same problem :/ – Cal Courtney May 31 '17 at 11:33
  • Did you use a placeholder in your query? `$sql = "SELECT cat_id, cat_name, cat_description FROM categories WHERE cat_id = :id";`, use that with `execute(array(':id' => $_GET['id']))` and I think it would work. PDO does nothing natively to prevent SQL injections you need to write your queries to be parameterized and bind in the values as needed. PDO/mysqli only support the ability to use parameterized queries, it is your job to write the queries. – chris85 May 31 '17 at 11:40
  • like I said; your logic's off. Your `if (!$query)` interprets to "if no query", then you're doing `else { if ($numRows == 0)` which interprets to "else if still nothing". This is a simple fix and all you needed to do was to fix the `if/else/while`. Anyway, see the answers, I'll just sit and watch, I wanted "you" to figure it out and think ;-) @CalCourtney – Funk Forty Niner May 31 '17 at 11:46

1 Answers1

2

The problem is with $numRows PDOStatement::fetchColumn does not count the rows that are in the result set. There is PDOStatement::rowCount for that.

As for the sql injection, It is not so much the class or functions you use that make it save, but the way you use the functions. To read more about it go here (link was in the top link in related for me)

Applying what we just learned to your code will give us something like this:

$sql = "SELECT cat_id, cat_name, cat_description FROM categories WHERE cat_id = :id"; // Parameterize query to prevent sql injection
$query = $DB_con->prepare($sql);

$query->execute([":id" => $_GET['id']]); // Binding parameter(s), could also be done using bindParam
$results = $query->fetchAll(PDO::FETCH_ASSOC); // For later use
$numRows = $query->rowCount();

if ($query->errorCode() == "00000") { // I don't think that PDOStatement ever fails to be created, so $query would never not be set 
    echo 'Something went wrong whilst getting the category from the database.';
} else {
    if ($numRows == 0) {
        echo 'Sorry, this category does not exist';
    } else {
        foreach ($results as $category){
            echo $category['cat_name'];
        }
    }
}

Note that the way I bind my parameters (in the execute) is my preferred way, not nessesarily the best way

Jelmergu
  • 973
  • 1
  • 7
  • 19