Does this appear as proper use of the commit and rollback functions?
I honestly haven’t used PDO functions directly – typically used a framework like Laravel or similar.
I do agree with the advice on phpdelusions.net and it has a page about PDO usage with a section about Transactions. It mentions:
Please note the following important things:
- PDO error reporting mode should be set to
- you have catch an
PDOException, as it doesn’t matter what
particular exception aborted the execution.
- you should re-throw an
exception after rollback, to be notified of the problem the usual way.
- also make sure that a table engine supports transactions (i.e. for
Mysql it should be InnoDB, not MyISAM)
- there are no Data definition
language (DDL) statements that define or modify database schema among
queries in your transaction, as such a query will cause an implicit
It appears that the error reporting mode is set to
PDO::ERRMODE_EXCEPTION✅ , though the
catch only catches
PDOException instances and the exception isn’t re-thrown.
What, if anything, can I do to improve this code?
Let’s look at these three lines near the top:
$addname = isset($value('addname ')) ? $value('addname ') : ''; $addemail = isset($value('addemail')) ? $value('addemail') : ''; $addcomment = isset($value('addcomment')) ? $value('addcomment') : '';
Should the first string index contain a space –
'addname '? Perhaps that is a typo?
The server is running PHP 7.3 or higher, right? Hopefully that is the case since at the time of writing those are the supported versions receiving Security fixes.
Presuming that is the case, then these lines could be simplified using the null-coalescing operator i.e.
$addname = $value('addname ') ?? ''; $addemail = $value('addemail') ?? ''; $addcomment = $value('addcomment') ?? '';
If the first two are actually
'' then does it even make sense to run the queries? If not, then handle that case appropriately – e.g. with an error message or other means.
Next, let’s look at that first query:
$selectCheck = $dbc->prepare("SELECT * FROM mainTable WHERE `name` = :newname AND `email` = :newemail");
Since that query is only used later to call
$selectCheck->rowCount() then there isn’t a point to select
*. It could simply select
1 or a single field. This will avoid the database query returning data that isn’t needed.