UPDATE is affecting all my rows?

Databases
1

Hi all

I have been trying to get my ‘hits’ column to increment by 1 each time the page is viewed. The problem is each time a page is viewed, every pages hits column is updated in the DB.

If I print $_GET['faq_id'] it shows the correct value.

Can anybody spot what I’m doing wrong below?

$update = mysql_query("UPDATE tbl_faqs SET hits = hits+1 WHERE faq_id=" . str_replace("-", " ", mysql_real_escape_string(strtolower($_GET['faq_id']))));

Cheers, Barry

2

There are a couple of obvious things:

  1. You are using $_GET['faq_id'] without validating the value you are getting.

  2. You are using out-dated mysql_* functions.

You would also make you code clearer if you didn’t nest functions but executed one at a time.

3

Thanks gandalf

  1. How do I valuate it? And will this make things work? As mentioned, if I print $_GET['faq_id'] the correct value is there.
  2. I understand I should be using mysqli… and a couple of newer functions though using an old system and lots of outdated code. Big job :neutral_face:

Barry

4

Not to overlook the obvious, but faq_id is unique, correct? (i.e. there is only one record on tbl_faqs which has faq_id of, say, 1)

If that’s not the case, I think we’ll need to see some more code. It looks like you’re not running what you think you’re running.

5

I may be wrong, but I don’t think putting SET hits = hits+1 within your UPDATE statement is going to increase its value. I would get the value of hits in a SELECT, increment it and then UPDATE it, I think

6

is faq_id meant to be a numeric? If it is then typecast it as a numeric:

$faq_id = (int) $_GET['faq_id'];

If the supplied value isn’t a numeric it’ll get replaced by 0.

7

Thanks guys!

Not to overlook the obvious, but faq_id is unique, correct? (i.e. there is only one record on tbl_faqs which has faq_id of, say, 1)

It’s unique, its the Primary Key.

is faq_id meant to be a numeric?

No, faq_id is varchar, though hits has the int value.

As mentioned, the hits updates, but instead of updating only the single $_GET['faq_id'] all the faqs get updated.

Barry

8

My full code which works, if this helps make things clearer (update at the bottom):

$sql1 = "SELECT faq_id,
  faq_slug,
  main_txt,
  quick AS quickanswer,
  faq_photo,
  DATE_FORMAT(updated, '%M %D %Y') AS subdir
  FROM tbl_faqs
  WHERE faq_id ='" . str_replace("-", " ", mysql_real_escape_string(strtolower($_GET['faq_id']))) . "'";

  $result1 = mysql_query($sql1) or die(mysql_error());
  $row1 = mysql_fetch_array($result1, MYSQL_ASSOC);
  $row1['title'] = str_replace("-", " ", ($_GET['faq_id']));
  $update = mysql_query("UPDATE tbl_faqs SET hits = hits+1 WHERE faq_id=" . str_replace("-", " ", mysql_real_escape_string(strtolower($_GET['faq_id']))));

Barry

9

Any ideas anybody?

Thanks, Barry

10

Why it isn’t autonumeric? It would be more practical for what you want to do

11

I understand what your saying molona, though not sure how I could code this. Could you show an example of the best approach with our existing code?

Thanks, Barry

12

Autonumeric is not a code, it is a property of the field itself so the change would have to be done on the database. But looking at your code, it may not be the right answer for you because in order to add up, it will have to create a new record.

Your code seems OK and since you’re using a primary key to get the right line, I fail to understand why all pages HITS column gets updated.

Maybe you should try to get more information. Change your update query for a select, and check that the number of rows selected is just one. If that’s the case, then you may want to output the values of the row in the browser to see that the right record is selected

13

Yes molona, this is what I’ve already done, spent lots of time yesterday and printed out the result to show a single correct faq_id.

Anyhow, somethings not right, I’ll keep testing see if I can fix it.

Thanks again for taking a look, I’ll post back when I fix it.

Barry

1 Like
14

Sorry that I can’t be of more help. :frowning:

15

I’m curious to know why you’re using a numeric comparison instead of a string comparison function.

It may “work” but can’t be trusted to not have unexpected effects. (eg. collation differences)

IMHO for the amount of time you’ve already spent on this, switching over to viable (as in more secure, more robust) database functions now while you’re rewriting it could have saved you and future-proofed.

16

If I understand correctly, I’m updating a numeric column, which is based on the string column fetched from faq_id. Or maybe I have this wrong? I know SQL and the basics though not a specialist in this area.

I’m working on an old site, lots of code like this with numerous select statement which are crying out for some stored procedures amongst other things. I might rebuild this in the coming weeks, as you say, future proof things.

Though saying all this, I’ve used this setup for many years and never seemed to have any problems, just this update I’m working on.

Cheers, Barry

Update: And curious myself Mittineague, what sort of approach would you recommend, when I do update this? Stored procedures? Mysqli… ?

17

Ah, that makes sense. In that case I’d lose the string functions and extract the numeric portion;

<?php
$test_str = "text-83";
$default_id = 0;
preg_match('/\d+/', $test_str, $match);
$extracted_id  = isset( $match[0] ) ? (int) $match[0] : $default_id;
var_dump($extracted_id);
?>

I like PDO better for a few reasons, being able to use named placeholders in prepared statements instead of only “?” probably being my favorite.

18

Ha, over my head right now this :slight_smile:
Fully rewriting the code… I’ll continue with what I have see if I can find the error.

Sounds good, I was working on this approach some time back, though due to my shared hosting at the time I was not able to use the lastest functions due to php versions etc. I’ll need to get back into my study and rebuild things from the ground up :grinning: Couple of weeks work.

Thanks anyhow Mittineague for the advice, as before, if I managed to solve this I’ll post back for those curious of the issue/fix needed for this.

Barry

19

If your host doesn’t have a version of PHP that has PDO you need to consider switching hosts ASAP.
PDO has been enabled by default as of PHP 5.1.0
5.1.0 was Released: 24 Nov 2005

IMHO any host that doesn’t offer any PHP versions newer than 10 years old should be avoided.

20

I know. This was a long time ago and, at the time I had lots of live websites using this version. When they finally updated, was just too much work as you can imagine, though after our chat I’m very keen to get back up to speed with this and start doing things correctly :smile:

Plus saving me a lot of coding time once I have things in place. On some pages I’m using over 6 Select Statements with numerous joins to show the data ha :tired_face:

I’ve also started a new job, been here a couple of months now and C# is in order ha .net environment windows, though some database work is involved most of it is done automatically through the CMS.

Cool, and thanks again Mittineague, no doubt I’ll be back once I start the upgrade :sunglasses: