0

I am attempting to implement a click count system. I am using the following code in this link Click here to see code, but changing it to modern standards. Initially I received errors for the msqli_real_escape_ string, but I believed I resolved it(no errors). Now, I am not receiving any errors at all, but the query is not sending into my database. I am using ini_set('display_errors', 1); error_reporting(E_ALL); for error checking. Also I have my $con and session in and ini file that I call, so the session and connection are not issues.

Does anyone see what I am doing wrong or is there a good way I can check to see what isn't working?

//create current page constant
$curPage = mysqli_real_escape_string($con,htmlspecialchars($_SERVER['PHP_SELF']));

//set number of clicks variable to 0
$clicks = 0;

//do not recount if page currently loaded
if($_SESSION['page'] != $curPage) {
   //set current page as session variable
   $_SESSION['page'] = $curPage;

    $click_sql = "
    SELECT *
    FROM click_count
    WHERE page_url = ?
    ";
    if (!$click_stmt = $con->prepare($click_sql)) {
        $click_stmt->bind_param("s", $curPage);
        $click_stmt->execute();
        $num_rows = $click_stmt->fetchColumn();
        if (!$click_stmt->errno) {
            // Handle error here
        }
        $stmt->bind_result($click_id, $page_url, $page_count);
    } elseif ($num_rows == 0) {
        //try to create new record and set count for new page to 1
         //output error message if problem encountered
            $click_insert_stmt = "
            INSERT INTO click_count 
            (page_url, page_count)
            VALUES(?, ?)";

         if(!$click_stmt = $con->prepare($click_insert_stmt)) {
            $click_insert_stmt->execute(array('$curPage',1));
            echo "Could not create new click counter.";
         }
         else {
            $clicks= 1;
         }
    } else {
    //get number of clicks for page and add 1     fetch(PDO::FETCH_BOTH)
        while($click_row = $click_insert_stmt->fetch(PDO::FETCH_BOTH)) {
            $clicks = $row['page_count'] + 1;
            //update click count in database;
            //report error if not updated

            $click_update_stmt = "
            UPDATE click_count
            SET page_count = ?
            WHERE page_url = ?
            ";
            if(!$click_stmt = $con->prepare("$click_update_stmt")) {
                $click_update_stmt->execute(array('$clicks', '$curPage')); 
                echo "Could not save new click count for this page.";
         }
        }
    }
}

Edit: New Updated Code

// ********Page count************

//create current page constant
$curPage = mysqli_real_escape_string($con,($_SERVER['PHP_SELF']));

//set number of clicks variable to 0
$clicks = 0;

//do not recount if page currently loaded
if($_SESSION['page'] != $curPage) {
   //set current page as session variable
   $_SESSION['page'] = $curPage;

    $click_sql = "
    SELECT *
    FROM click_count
    WHERE page_url = ?
    ";
    if (!$click_stmt = $con->prepare($click_sql)) {
        $click_stmt->bind_param("s", $_SERVER['PHP_SELF']);
        $click_stmt->execute();
        $num_rows = $click_stmt->fetchColumn();
        if (!$click_stmt->errno) {
            // Handle error here
        }
        $stmt->bind_result($click_id, $page_url, $page_count);
    } elseif ($num_rows == 0) {
        //try to create new record and set count for new page to 1
         //output error message if problem encountered
            $click_insert_stmt = "
            INSERT INTO click_count 
            (page_url, page_count)
            VALUES(?, ?)";

         if(!$click_stmt = $con->prepare($click_insert_stmt)) {
            $click_insert_stmt->execute(array($curPage,1));
            echo "Could not create new click counter.";
         }
         else {
            $clicks= 1;
         }
    } else {
    //get number of clicks for page and add 1     fetch(PDO::FETCH_BOTH)
        while($click_row = $click_insert_stmt->fetch(PDO::FETCH_BOTH)) {
            $clicks = $row['page_count'] + 1;
            //update click count in database;
            //report error if not updated

            $click_update_stmt = "
            UPDATE click_count
            SET page_count=page_count+1
            WHERE page_url = ?
            ";
            if(!$click_stmt = $con->prepare("$click_update_stmt")) {
                $click_update_stmt->execute(array($curPage)); 
                echo "Could not save new click count for this page.";
         }
        }
    }
}
Paul
  • 3,348
  • 5
  • 32
  • 76
  • Also, this, `->fetch(PDO::FETCH_BOTH)) {`. Why are you mixing `MySQLi` and `PDO`? – Rajdeep Paul Oct 19 '16 at 18:18
  • I just started learning PDO, so I am attempting to write it in this code. Converting from old mysql always confuses me. – Paul Oct 19 '16 at 18:20
  • PDO's not that much different conceptually, there's just slight differences in method names. At a glance it's hard to tell PDO code from `mysqli`. – tadman Oct 19 '16 at 18:23
  • the most important underlying question here is; what's the API used to connect with? you tagged as mysqli and PDO; two different animals here. – Funk Forty Niner Oct 19 '16 at 18:30
  • I am attempting to do PDO. It seems all of the code I see on the internet is either mysql or mysqli. Since I am trying to learn PDO and do this the right way, I am getting mixed up some, since it all pretty much looks the same. – Paul Oct 19 '16 at 18:35
  • they're not the same. `mysql_`, `mysqli_`, and PDO are all different, just like you can't have cats breeding with dogs and sheep ;-) – Funk Forty Niner Oct 19 '16 at 18:38
  • I know they are different. I just don't know PDO well enough to know which elements are PDO. Trust me, I do not want my dogs breeding with cats. – Paul Oct 19 '16 at 18:39
  • ok well you never answered my question earlier. Which one are you using to connect with? `mysql_`? `mysqli_`? PDO. I shouldn't be the one doing all the leg work here ;-) ask Tadman now; I can't keep commenting back and forth like this. – Funk Forty Niner Oct 19 '16 at 18:40
  • I am connecting with PDO. – Paul Oct 19 '16 at 18:43
  • Wait...even with my new code I added to my question I am mixing PDO and mysqli? – Paul Oct 19 '16 at 18:44
  • You still have `mysqli_real_escape_string()` – Jay Blanchard Oct 19 '16 at 18:46

1 Answers1

3

It looks like you're doing a lot of stuff like this:

$click_update_stmt->execute(array('$clicks', '$curPage'));

I'm not sure where you picked up this habit of quoting variables as strings, but you need to drop it. '$x' and $x are two hugely different things. In the first case it's literally '$x' and in the second case it's whatever the $x variable happens to represent.

Fix it like this:

$click_update_stmt->execute(array($clicks, $curPage));

Also since you're using prepared statements, which by the way is great, you do not need to and should not manually escape your values. Applying them to placeholders with bind_param is the safe way of doing it. Doing any other escaping mangles the data.

Just bind directly to the source:

$click_stmt->bind_param("s", $_SERVER['PHP_SELF']);

Don't arbitrarily run things like htmlspecialchars on input out of paranoia or because you're doing cargo-cult programming and you saw it done in a YouTube tutorial somewhere. That function is intended to be used to display values only, not store them. Data in your database should be as raw as possible.

There's a lot of problems with this code, and one of them that has me confused is why there's so much code. Remember SELECT * and then binding results to arbitrary variables is trouble, your schema might change and then your code is out of sync. Whenever possible fetch rows as an associative array if doing this, then all you have to worry about is renamed ore removed columns.

The biggest problem is this is subject to race conditions because it doesn't use an atomic increment. When writing counters, always do your updates as operations that are a single statement:

UPDATE click_count SET page_count=page_count+1 WHERE page_url=?

Your approach of reading the count, incrementing it, and then writing it back into the database means that you're inviting problems if another operation runs concurrently, something very likely on click-counter code.

tadman
  • 208,517
  • 23
  • 234
  • 262
  • Thank you, this helped me learn a lot. Though I am confused when you say I do not need to escape my values. Are you referring to when I do the execute? I added my new code based on your suggestions. It still isn't inserting into my db. – Paul Oct 19 '16 at 18:33
  • I mean "pre-escape" them or manually escape them. Remember, `bind_param` does it for you. If you do it on top of that you end up with things like `It\\\'s me!` in your database. The general rule of thumb is **escape or bind**, but never both. – tadman Oct 19 '16 at 18:40
  • Can you give me an example of where I was doing, so I can better understand? Do you see anything else within my new code that I am doing wrong? – Paul Oct 19 '16 at 18:45
  • `$curPage` looks left-over from before. Also if you're using MySQL you can collapse all this down to: `INSERT INTO click_count (page_url, page_count) VALUES (?, 1) ON DUPLICATE KEY UPDATE page_count=page_count+1` using the [`ON DUPLICATE KEY`](http://dev.mysql.com/doc/refman/5.7/en/insert-on-duplicate.html) feature. This presumes your `page_url` column has a `UNIQUE` index, which it should. – tadman Oct 19 '16 at 18:47
  • Well, what could I change `$curPage` to? Everything I see in searching just says to use PDO or prepared statements. Not sure how to modify it. – Paul Oct 19 '16 at 18:51
  • You don't need `$curPage`, so get rid of it. Use the `$_SERVER` variable directly. That has the additional benefit of making it clear what you're doing. – tadman Oct 19 '16 at 18:53
  • So change it to this? `$curPage = $_SERVER['PHP_SELF'];` Sorry if I am not fully understanding, I am trying. – Paul Oct 19 '16 at 18:55
  • I tried the method I posted last and it didn't help. – Paul Oct 19 '16 at 19:27