yes and no. the try blocks itself are not that of a problem, but the whole class design.
never echo/print inside a method. always return the value to be printed and let the environment decide, what to do with the value.
likewise, never die() from an exception unless you are at the top-most level. this allows you to orderly end the output (HTML).
never show error messages to your users. most user don’t care or don’t understand what’s written there, except when they are out for hacking you, then it’s valuable information.
a database object/connection is a dependency of this class and is supposed to be configured elsewhere.
insert queries are supposed to be done with PDO::exec().
the current timestamp is avalable in SQL through the NOW() function and a date/time column should have the appropriate data type (datetime/timestamp). (and timestamp columns allow for automatic datetime insertion, much like an auto-increment column does)
I don’t get why you have the if{} else{} error thing inside of your try{} block.
If security is your concern, why aren’t you using preprepared statements for the insert?
I’m hoping the error echoing is there temporarily just for debugging purposes, to be removed later.
#There shouldn’t be no try…catch blocks at all.
These two blocks you have at the moment are insecure, inconvenient and simply bloating your code. Just leave PDO exceptions alone.
Besides, there are other criticall issues in your code:
there should be no private function dbConn() in the class that also has add_banned_user() method. a pdo connection should be passed as a constructor parameter instead.
you should use prepared statements for all your queries
like I said above, ther should be not a single try catch block for this code.
so it should be like
class whatever {
function __construct($pdo) {
$this->pdo = $pdo;
}
private function add_banned_user() {
$sql = "INSERT INTO banned_users (username, timestamp) VALUES (?,?)";
$this->conn->prepare($sql)->execute([$this->username,time()]);
}
}
$conn = new PDO("mysql:host=".DB_SERVER.";dbname=".DB_NAME, DB_USER, DB_PASS);
// set the PDO error mode to exception
$conn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$user = new whatever($conn);
$user->add_banned_user();
@Dormilich there is no logic in this statement.
The only cause for the blank page is an absence of the error handler.
And a zillion of try catch blocks all over your code won’t make one.
If you need a handler to handle errors, you have to write one. And catching only some errors has nothing to do with it.
@colshrapnel, your argument like this in cryptic terms most probably won’t help the OP to understand how to organize code better, some examples of how to do it would be more useful.
I agree that catching PDO exceptions in all classes is bloat but the exception should be caught somewhere - the best place would be a central index.php or other php script that wraps the whole application. Then there is only one try…catch block and individual classes don’t need to be concerned with that at all.
Do you have any sense of the context? Can you refrain for a moment from making generalized statements and descend to the particular problem discussed here? If so, will the following statement satisfy you enough?
[for the code present in this particular question] There shouldn’t be no try…catch blocks at all.
It makes no sense as error handler is much more flexible than a blatant catch right in the middle of index php. Why bloat index php with error handling code if you can put it where it belongs somewhere among the other libraries? Not to mention you need an error handler anyway, despite any catch blocks, making you essentially writing a duplicated code.
You should write your own error handling code only if you’re a pro and know what are you doing. Otherwise just leave errors alone or you will make it worse, just like the poor guy did. So I appreciate your high-brow theoretical musings, but as long as you cannot suggest a reliable exception handling code, do not suggest to catch them as well. PHP is much more intelligent than an average PHP user, so as a rule of thumb the latter should leave exceptions alone.