PHP function returns false even if executed successfully

Question

I have the following functions.

function generateOTP($theUser, $thePhone){
  global $pdo;

  $stmt = $pdo->prepare("DELETE FROM otp WHERE phone = :phone");
  $stmt-> bindValue(':phone', $thePhone);
  $stmt-> execute();

  $stmt = $pdo->prepare("INSERT INTO otp(phone, otp, type, validity)VALUES(:phone, :otp, :type, :val)");
  $stmt-> bindValue(':phone', $thePhone);
  $stmt-> bindValue(':otp', rand(1000, 9999));
  $stmt-> bindValue(':type', 'new');
  $stmt-> bindValue(':val', date('Y-m-d H:i:s', strtotime('+5 mins')));
  $stmt-> execute();
}

function sendOTP($theUser, $thePhone){
  global $pdo;
  generateOTP($theUser, $thePhone);
}

// CALLED SOMEWHERE LIKE THIS
if(sendOTP($theUser, $thePhone){
  echo "OTP SENT";
}else{
  echo "OTP SENDING FAILED";
}

The problem I have encountered is that even if the sendOTP() function is executed well and all the records have been inserted in the database, it always moves to the else block and prints OTP SENDING FAILED. In other words, it always returns false assuming that the function failed to execute successfully. But actually, function has been executed well and all the queries are executed properly. This is a strange issue I have never come across before. How can I solve this?

Your function doesn't return anything, so I'm not sure how you're expecting to be able to tell the real result. Trying to get the result of a function with no `return` statement will always result in `null`, which is false-y, which is why your `if` behaves as it does. — ADyson, Jul 19 '21 at 15:49
If you genuinely want to know whether all the PDO code succeeded then enable PDO error handling: https://www.php.net/manual/en/pdo.error-handling.php . You could then wrap it in a try/catch if you need to handle those. — ADyson, Jul 19 '21 at 15:55
Also your output messages are misleading (unless you have omitted some of the code) because all your code does is delete and insert values into a database. It doesn't actually _send_ the value to anyone's phone. Sending could still easily fail after the insert has occurred, but I assume that process is handled elsewhere in your application (or in some code ommitted from the sendOTP() snippet in your example above). — ADyson, Jul 19 '21 at 15:57
@ADyson you are right and I even tried returning true as `if(generateOTP($theUser, $thePhone){ return true; }` but still the result is same. After trying every possible methods I knew, I was forced to raise a question. — Relaxing Music, Jul 19 '21 at 15:58
@ADyson I know the PDO code executed well as the records are inserted in the database. Do I really still need to use the try catch method? — Relaxing Music, Jul 19 '21 at 15:59
@ADyson it is obvious that right now I am only dealing it with the database level. Actual OTP sending is yet to be integrated (later). I am seeking a solution at the current database level issue currently. — Relaxing Music, Jul 19 '21 at 16:00
`it is obvious`...to you, maybe. We can only work with what you've shown us. Anyway I've already explained what the issue is and suggested a better approach. — ADyson, Jul 19 '21 at 16:04
`I know the PDO code executed well as the records are inserted in the database. Do I really still need to use the try catch method`...well you said you were trying to catch failures, so if you want to catch a failure when it happens then yes. Issues can occur unexpectedly, e.g. network problems or database goes down, or bad data input values, or something like that. If you just `return true;` without testing any of that, then it would be just as un-informative and predictable as the code you've got now. — ADyson, Jul 19 '21 at 16:05

score 2 · Accepted Answer · answered Jul 19 '21 at 16:24

I can advice next solution (The otp table must have unique/primary key phone):

<?php
function generateOTP($theUser, $thePhone){
    global $pdo;
    try {
        $stmt = $pdo->prepare("
            INSERT INTO otp(phone, otp, type, validity) 
            VALUES (:phone, :otp, :type, :val)
            ON DUPLICATE KEY UPDATE
                otp = :otp,
                type = :type,
                validity = :val");
        $stmt-> bindValue(':phone', $thePhone);
        $stmt-> bindValue(':otp', rand(1000, 9999));
        $stmt-> bindValue(':type', 'new');
        $stmt-> bindValue(':val', date('Y-m-d H:i:s', strtotime('+5 mins')));
        return $stmt-> execute();
    } catch (PDOException $Exception ) {
        // print_r($Exception);
        // here you can log Exception
        return false;
    }
}

function sendOTP($theUser, $thePhone){
  global $pdo;
  return generateOTP($theUser, $thePhone);
}

Here You can test PHP code online

score 0 · Answer 2 · answered Jul 19 '21 at 15:59

0

your functions doesn't return any values you need to change your code from :

function generateOTP($theUser, $thePhone){
  global $pdo;

  $stmt = $pdo->prepare("DELETE FROM otp WHERE phone = :phone");
  $stmt-> bindValue(':phone', $thePhone);
  $stmt-> execute();

  $stmt = $pdo->prepare("INSERT INTO otp(phone, otp, type, validity)VALUES(:phone, :otp, :type, :val)");
  $stmt-> bindValue(':phone', $thePhone);
  $stmt-> bindValue(':otp', rand(1000, 9999));
  $stmt-> bindValue(':type', 'new');
  $stmt-> bindValue(':val', date('Y-m-d H:i:s', strtotime('+5 mins')));
  $stmt-> execute();
}

function sendOTP($theUser, $thePhone){
  global $pdo;
  generateOTP($theUser, $thePhone);
}

to:

function generateOTP($theUser, $thePhone){
  global $pdo;

  $stmt = $pdo->prepare("DELETE FROM otp WHERE phone = :phone");
  $stmt-> bindValue(':phone', $thePhone);
  $stmt-> execute();

  $stmt = $pdo->prepare("INSERT INTO otp(phone, otp, type, validity)VALUES(:phone, :otp, :type, :val)");
  $stmt-> bindValue(':phone', $thePhone);
  $stmt-> bindValue(':otp', rand(1000, 9999));
  $stmt-> bindValue(':type', 'new');
  $stmt-> bindValue(':val', date('Y-m-d H:i:s', strtotime('+5 mins')));
  if ($stmt-> execute()) {
   return true;
  } else {
   return false;
  }
}

function sendOTP($theUser, $thePhone){
  global $pdo;
  if (generateOTP($theUser, $thePhone)) { 
   return true;
  } else {
   return false;
  }
}

now you made your function return true/false

answered Jul 19 '21 at 15:59

Omar Khaled

66
1
9

1

There are a lot more ways for the PDO code to fail than just the final execute() statement. PDO error handling enabled, possibly with a try/catch, would be a much more robust approach – ADyson Jul 19 '21 at 16:03
Oh I got it now.. I had only tried to return true in the `sendOTP()` function. I needed to do that in both the functions. Got it. Also, `else{ return false; }` is not required. I can check with just `true` and see `if(sendOTP($theUser, $thePhone))` which means if it's true else it will be default always mean false. – Relaxing Music Jul 19 '21 at 16:05
@RelaxingMusic Be aware that if your DELETE fails to execute(), or any of the prepare() or bindValue() calls fail for any reason, this half-baked answer you've accepted would not detect it properly. – ADyson Jul 19 '21 at 16:08
@ADyson sure.. I will keep that in mind :) – Relaxing Music Jul 19 '21 at 16:09
@ADyson or, can you please give a full baked answer to make it more secure? – Relaxing Music Jul 19 '21 at 16:11
@RelaxingMusic https://stackoverflow.com/a/68443913/5947043 beat me to it – ADyson Jul 19 '21 at 16:29
@ADyson I will "try" to beat you. However, I accept it's a good way but my view relies on this answer here https://stackoverflow.com/a/273090/16383273 . Since I am using the type of query (both delete and insert) in such a way that it will either execute or fail. Delete query here is kind of independent. It will never fail and would always execute doesn't matter if it finds something to delete or not (if I do not consider failing internet or database down as you said) and the only time I would like to try and catch a query if I want to perform a particular task if my query fails. – Relaxing Music Jul 19 '21 at 17:01
@ADyson I maybe wrong but I know for sure that at least for the queries above try/catch is not important. – Relaxing Music Jul 19 '21 at 17:04
@RelaxingMusic if you're going to discount network issues fine that's up to you, as long as you enable PDO error reporting it will just crash your app which might be fine in such a scenario. Discounting failure due to bad data is unwise though I think. But it depends how robust you want this to be. You asked how to know if the operation had failed. I told you, but now you don't like the answer, although you've accepted the answer I pointed you to, which suggests you _do_ like the answer, so now I'm not sure what you're complaining to me about? – ADyson Jul 19 '21 at 17:09
@ADyson I am not complaining about anything. The answer you suggested and I accepted is great (which just uses an extra and simple try/catch blocks for logging errors differentiated from what I am doing). I am only suggesting that in my case I don't find it necessary using try/catch block. It's good if I do and log errors, I know, but that would be the case when I am executing a query at which I am expecting failures to happen in case (discounting netowrk issues, of course). However, I am also aware that I should never be writing queries or functions such that which expects failures to happen. – Relaxing Music Jul 19 '21 at 17:21
@RelaxingMusic I see, perhaps I misunderstood your point then. I agree you shouldn't just throw try/catch around everywhere unless you think a problem could occur. But equally if you want to be able to report success/failure through the regular UI then you need it. The alternative is to enable PDO error handling without try/catch, and then just have your application report success every time. If it then crashes, you'd see a crash screen and know it had failed. If it doesn't crash, you can assume success because no exception was thrown. – ADyson Jul 19 '21 at 17:25
@ADyson If it will crash screen I would not like to enable PDO error reporting too. I don't want my users to have a bad user experience. I will just try and write queries that would never fail (again, discounting network issues because users can always retry when they are having a good network connection that won't drop). I would write code logic in such a way that it satisfies the query execution only when things are actually going well. – Relaxing Music Jul 19 '21 at 17:29
@RelaxingMusic but if you don't enable PDO error reporting then you wouldn't necessarily know if the DB query had failed due to some issue. e.g. if the DELETE failed. So then it might even report success without actually being true (e.g. the DELETE could fail but the INSERT succeed due to some momentary problem. Unlikely, but possible. ) Again, depends on how robust you want/need it to be. Also you can still handle exceptions more globally further up your application stack so users don't ever have to see a "dirty" crash screen, they can just get a generic "oops something went wrong" view. – ADyson Jul 19 '21 at 17:33
@ADyson totally agreed.... Will keep the points in mind... :) – Relaxing Music Jul 19 '21 at 17:36

PHP function returns false even if executed successfully

2 Answers2