Code Critique: Using $_GET. Critique bashing welcome!

[OldWest]

As some of you know, I am still learning the fundamentals of good php code practice and have been working on a custom application for my own practice and personal schooling. My below code IS working as expected, but I wanted any ideas or critique on better, more secure, faster, etc methods of it.. Thanks for any input:

 

<?php
  
  $id = mysqli_real_escape_string($cxn, $_GET['id']);
  
  $city_name = mysqli_real_escape_string($cxn, $_GET['city_name']);
  
  $posts_by_city_sql = "SELECT id, city_id, title FROM postings WHERE city_id='$id'";
  
  $posts_by_city_results = (mysqli_query($cxn, $posts_by_city_sql)) or die("Was not able to grab the Postings!");
  
  $row_cnt = mysqli_num_rows($posts_by_city_results);
  if ($row_cnt == 0) {
      printf("We're sorry. There are %d postings in: <strong>$city_name</strong>", $row_cnt);
  } else {
      printf("Congratulations! There are %d postings in: <strong>$city_name</strong>", $row_cnt);
      echo "<ul>";
      while ($posts_by_city_row = mysqli_fetch_array($posts_by_city_results)) {
          echo "<li><a href='posting_details.php?id=$posts_by_city_row[id]'>$posts_by_city_row[title]</a></li>";
      }
      // end while loop
      echo "</ul>";
  }
  // end row_cnt if
  mysqli_free_result($posts_by_city_results);
  mysqli_close($cxn);
?>

 

[ram4nd]

remove double quotes if you aim fast

[trq]

mysqli_num_rows will throw an error if $posts_by_city_results is not a resource. You need to check it is 'true' before using it.

 

Also, don't you get sick of typing? Your variables are extremely explicit. Its probably good while your learning, but IMO it makes code even harder to read.

[rwwd]

remove double quotes if you aim fast

 

Is that really something that's true? I always use double quotes and concatenate vars into the string rather than {} them, I know that's slightly faster, but quoting style shouldn't compromise speed unless your talking about uS of difference.

 

Rw

[OldWest]

thorpe, no doubt my naming conventions are probably very annoying to an advanced php programmer like you  , and yeah, it is a bit cumbersome and annoying to type those out! im only doing that so i can follow my code a bit easier while im grasping what is going on with it all.. thanks for your critique.. Here's what I've got now working as expected checking the TRUE status of ..num_rows..

 

Any other suggestions are of course welcome (don't need to be easy on me!  )

 

<?php
  $id = mysqli_real_escape_string($cxn, $_GET['id']);
  $city_name = mysqli_real_escape_string($cxn, $_GET['city_name']);
  $posts_by_city_sql = "SELECT id, city_id, title FROM postings WHERE city_id='$id'";
  $posts_by_city_results = (mysqli_query($cxn, $posts_by_city_sql)) or die("Was not able to grab the Postings!");
  $row_cnt = mysqli_num_rows($posts_by_city_results);
  if (mysqli_num_rows($posts_by_city_results) != true) {
      printf("<br />We're sorry. There are %d postings in: <strong>$city_name</strong>", $row_cnt);
  } else {
      printf("<br />Congratulations! There are %d postings in: <strong>$city_name</strong>", $row_cnt);
      echo "<ul>";
      while ($posts_by_city_row = mysqli_fetch_array($posts_by_city_results)) {
          echo "<li><a href='posting_details.php?id=$posts_by_city_row[id]'>$posts_by_city_row[title]</a></li>";
      }
      echo "</ul>";
  }
  mysqli_free_result($posts_by_city_results);
  mysqli_close($cxn);
?>

 

 

 

[Pikachu2000]

What type of data is $_GET['id'] expected to be? A string, an integer, or . . . ?

[OldWest]

What type of data is $_GET['id'] expected to be? A string, an integer, or . . . ?

 

Hmmm interestingly good point. I assume I would want to check this to ensure the data passed is as expected - more of a security check?

 

I think is_int() will work for the id and is_string() would work for city_name.. hope I am getting your point correctly!

 

Back at it...

[Pikachu2000]

If its expected to be an integer, don't bother with mysqli_real_escape_string(). Validate it, cast it as an integer, and leave the value unquoted in the query string.

 

Also, since $_GET['city_name'] isn't used in a query, there's no need to send it through mysqli_real_escape_string(), in fact since all you do is echo it, it can be detrimental to do so.

if( isset($_GET['id']) && ctype_digit($_GET['id']) ) {
     $id = (int) $_GET['id'];
} else {
    // error, or set default value, etc.
}

[OldWest]

had to use is_numeric() ... was this the correct point?

 

<?php
  $id = mysqli_real_escape_string($cxn, $_GET['id']);
  if (!is_numeric($id)) {
      echo "Nice try. $id is not an integer.";
      exit();
  }
  $city_name = mysqli_real_escape_string($cxn, $_GET['city_name']);
  if (!is_string($city_name)) {
      echo "Nice try. $city_name is not a string!";
      exit();
  }
  $posts_by_city_sql = "SELECT id, city_id, title FROM postings WHERE city_id='$id'";
  $posts_by_city_results = (mysqli_query($cxn, $posts_by_city_sql)) or die("Was not able to grab the Postings!");
  $row_cnt = mysqli_num_rows($posts_by_city_results);
  if (mysqli_num_rows($posts_by_city_results) != true) {
      printf("<br />We're sorry. There are %d postings in: <strong>$city_name</strong>", $row_cnt);
  } else {
      printf("<br />Congratulations! There are %d postings in: <strong>$city_name</strong>", $row_cnt);
      echo "<ul>";
      while ($posts_by_city_row = mysqli_fetch_array($posts_by_city_results)) {
          echo "<li><a href='posting_details.php?id=$posts_by_city_row[id]'>$posts_by_city_row[title]</a></li>";
      }
      echo "</ul>";
  }
  mysqli_free_result($posts_by_city_results);
  mysqli_close($cxn);
?>

 

[OldWest]

If its expected to be an integer, don't bother with mysqli_real_escape_string(). Validate it, cast it as an integer, and leave the value unquoted in the query string.

 

Also, since $_GET['city_name'] isn't used in a query, there's no need to send it through mysqli_real_escape_string(), in fact since all you do is echo it, it can be detrimental to do so.

if( isset($_GET['id']) && ctype_digit($_GET['id']) ) {
     $id = (int) $_GET['id'];
} else {
    // error, or set default value, etc.
}

 

Interesting point and method. I like the casting thing. checking up on what ctype does.. Will mess around with this idea. Thanks o bunch

[Pikachu2000]

is_int() won't work, since all form data is sent as a string value, and is_numeric() will consider 4.5E17 (an exponential number) a valid value, so ctype_digit() is probably the best way to go.

[trq]

checking the TRUE status of ..num_rows..

 

You might want to re-read my original reply. Its the argument passed to mysqli_num_rows that needs checking. (Well, both actually)

[OldWest]

checking the TRUE status of ..num_rows..

 

You might want to re-read my original reply. Its the argument passed to mysqli_num_rows that needs checking. (Well, both actually)

 

Am I getting you?

 

if (mysqli_num_rows($posts_by_city_results) !=true && $row_cnt != true)

 

[trq]

No. You need to check that $posts_by_city_results is a resource (true) before passing it to mysqli_num_rows(). Otherwise, if your query fails, you will get an error.

 

You are however using 'or die()' after your call to mysqli_query() which will catch this error, its not at all a nice way of doing it though.

 

All queries should be something like.....

 

$sql = "your query";
if ($result = mysql_query($sql)) {
  if (mysql_num_rows($result)) {
    // $rows contains records and is good to use
  } else {
    echo "No Results found";
  }
} else {
  trigger_error('Query failed ' . $sql);
}

 

The reason you use trigger_error() instead of a simple die is that error reporting can be adjusted via configuration. You don't want php error messages (especially ones that might hint about your database setup) to be displayed to a user in production.

[OldWest]

Completed the implement of thorpes recommendations. Now getting on data decisions and filtering.

 

<?php
  $id = mysqli_real_escape_string($cxn, $_GET['id']);
  if (!ctype_digit($id)) {
      echo "Nice try. $id is not an integer.";
      exit();
  }
  $city_name = mysqli_real_escape_string($cxn, $_GET['city_name']);
  if (!is_string($city_name)) {
      echo "Nice try. $city_name is not a string!";
      exit();
  }
  $posts_by_city_sql = "SELECT id, city_id, title FROM postings WHERE city_id='$id'";
  
  if ($posts_by_city_results = mysqli_query($cxn, $posts_by_city_sql)) {
      $row_cnt = mysqli_num_rows($posts_by_city_results);
      if (mysqli_num_rows($posts_by_city_results)) {
          printf("<br />Congratulations! There are %d postings in: <strong>$city_name</strong>", $row_cnt);
          echo "<ul>";
          while ($posts_by_city_row = mysqli_fetch_array($posts_by_city_results)) {
              echo "<li><a href='posting_details.php?id=$posts_by_city_row[id]'>$posts_by_city_row[title]</a></li>";
          }
          echo "</ul>";
      } else {
          printf("<br />We're sorry. There are %d postings in: <strong>$city_name</strong>", $row_cnt);
      }
  } else {
      trigger_error('Query failed ' . $posts_by_city_sql);
  exit();
  }
  mysqli_free_result($posts_by_city_results);
  mysqli_close($cxn);
?>

[OldWest]

Completed with all recommendations.. For note, I want to pull the city_name to echo it on the page so the output is user friendly indicating which city is related to the results:

 

<?php
  if (isset($_GET['id']) && ctype_digit($_GET['id'])) {
      $id = (int)$_GET['id'];
  } else {
      echo "Nice try! The id passed is not an integer.";
  }
  if (isset($_GET['city_name']) && is_string($_GET['city_name'])) {
      $city_name = (string)$_GET['city_name'];
  } else {
      echo "Nice try! The id passed is not a string.";
  }
  $posts_by_city_sql = "SELECT id, city_id, title FROM postings WHERE city_id='$id'";
  if ($posts_by_city_results = mysqli_query($cxn, $posts_by_city_sql)) {
      $row_cnt = mysqli_num_rows($posts_by_city_results);
      if (mysqli_num_rows($posts_by_city_results)) {
          printf("<br />Congratulations! There are %d postings in: <strong>$city_name</strong>", $row_cnt);
          echo "<ul>";
          while ($posts_by_city_row = mysqli_fetch_array($posts_by_city_results)) {
              echo "<li><a href='posting_details.php?id=$posts_by_city_row[id]'>$posts_by_city_row[title]</a></li>";
          }
          echo "</ul>";
      } else {
          printf("<br />We're sorry. There are %d postings in: <strong>$city_name</strong>", $row_cnt);
      }
  } else {
      trigger_error('Query failed ' . $posts_by_city_sql);
      exit();
  }
  mysqli_free_result($posts_by_city_results);
  mysqli_close($cxn);
?>

 

[Pikachu2000]

Using is_string() is sort of pointless when validating form data, since all form data (even an empty text field) is, by default, string type. You'd be better off to make sure the string had a minimum length using strlen() on the trim()med variable.