Geeklog-Core / geeklog

Geeklog - The Secure CMS.
https://www.geeklog.net
24 stars 19 forks source link

Migrate to PDO for databse access #1092

Open mystralkk opened 3 years ago

mystralkk commented 3 years ago

Currently, Geeklog uses mysqli and pgsql drivers through a wrapper layer. However, you cannot use prepare statements or bind values to them. You have to build an SQL statement manually like:

$rows = [];
$sql = "SELECT * FROM some_table WHERE id ='" . DB_escapeString($id) . "'";
$result = DB_query($sql);

if (!DB_error() {
    while (($row = DB_fetchArray($result, false) !== false) {
        $rows[] = $row;
    }
}

This is tiresome and easily leads to errors. With PDO as the backend, you could do like this:

$rows = [];
$sql = "SELECT * FROM some_table WHERE id = :id";
$statement = DB_prepare($sql);
$statement->bindValue(':id', $id);

if ($statement->execute()) {
    foreach ($statement->fetchAll() as $row) {
        $rows[] = $row;
    }
}

How does this sound?

eSilverStrike commented 3 years ago

I am not that familiar with PDO. PDO is also faster is it not?

I assume both ways would be accessible to support plugins that have not been migrated yet?

mystralkk commented 3 years ago

PDO might be a bit slower than native extensions like mysqli or pgsql, but it is fast enough, I believe. I am planning to keep conventional DB_* functions as they are.

eSilverStrike commented 3 years ago

Sounds good.

As I said I am not to familiar with PDO.

Is PDO automatically included in PHP installs or will we have to add some checks for it?

I guess the Geeklog install and Wiki Docs, etc. will also have to be updated

I really should find an article and read up on it.

If you think it makes sense to migrate Geeklog to it then I don't see me having a problem with it.

mystralkk commented 3 years ago

PDO is available by default( https://www.php.net/manual/en/pdo.installation.php ), but you have to enable a database-specific driver like pdo_mysql or pdo_pgsql. This is easy to check in the installer.

mystralkk commented 2 years ago

After some research, I have found it is difficult to implement DB_numRows() with PDO, since there is no equivalent method. I believe some of DB_numRows calls can be removed in some way, but there are more than 300 places where DB_numRows is called in the core and plugins shipped with the core ...

eSilverStrike commented 2 years ago

I guess rowcount for PDO is not an option since it doe not work with SELECT. I wonder why they do not have a solution out of the box for this. It seems like something most people would want when coding...

// A few possible solutions but most seem to deal with adding a COUNT to the select statement which doesn't really work in our situation... https://stackoverflow.com/questions/54283485/pdo-row-count-num-rows-equivalent https://stackoverflow.com/questions/11305230/alternative-for-mysql-num-rows-using-pdo

A lot of DB_numRows used in Geeklog are just checking to see if any records are returned before it does a fetch so I guess that could be a new function called something like DB_rowsReturned for this use type.

Where Geeklog and plugins rely on numrows to actually cycle through the records I guess we would have to change it to a loop like:

while ($row = DB_fetchArray($result)) {

Of course all those changes are a lot of work and testing.

Do you thing all that work is worth it if an actual DB_numRows solution is not found???

How would plugins be dealt with that do not support PDO (yet)?

mystralkk commented 2 years ago

Do you thing all that work is worth it if an actual DB_numRows solution is not found???

I don't think so. The main reason I suggested introducing PDO is I would like to use prepared query and placeholders in SQL statements. After I looked into mysqli* and pg* functions, I found these features are possible to implement with these functions. In that case, I would create DB_prepare(string $sql), DB_bindValue(string $name, mixed $value), DB_execute($statement) functions without introducing the PDO extension.

How would plugins be dealt with that do not support PDO (yet)?

If we decide to introduce PDO, I will make 2 APIs; one is the traditional DB_* functions (implemented with PDO) and a thin wrapper of PDO.