Request for vote - ONLY support PDO database driver

[lucasjkr]

Topic to vote on: Drop support for all database access methods except PDO.

Background: opencart has abstracted its database access methods, so that it can use a variety of native PHP drivers including: mysql, mysqli, mssql, Postgres, and PDO.

A long time ago, I convinced @arnis to drop support for old style mysql_ drivers, given that they were deprecated in later versions of PHP5 and removed from php7

We've had debates about what version of MySQL to support, but we've not addressed all of the other "supported" databases. I use quotes because while there are database wrappers for a variety of databases, I doubt that most of opencart would install or even execute using those drivers due to MySQL specific code.

I am therefor suggesting dropping any pretend support for anything but MySQL, and not only that, drop support for the mysqli driver and ONLY use PDO.

Benefits are:

PDO is database agnostic. The same code should work across any database that has PDO drivers available. The SQL itself would need to be tweaked, though, so it would pose the same difficulty to support other databases using PDO as it would as it is currently.

PDO also supports positional placeholders in prepared statements. In case you're not aware, prepared statements are a means of inserting data into the directly into database, without all the arduous casting and escaping that litters the queries in Opencart.

For reference, look at the insert queries in Copona/opencart compared to what I've done in my own little project (working name: Opencommerce -also a fork, but being very focused/dropping support for lots of things in order to clean up the code base) - I’ll continue on that, but I wish I could share stuff over to you guys.. Note: I also got rid of DB_PREFIX, but even discounting that:

https://raw.githubusercontent.com/copona/copona/master/admin/model/catalog/product.php

vs

https://raw.githubusercontent.com/lucasjkr/opencommerce/master/common/model/catalog/product_admin.php

Named placeholders read easier, and are not positionally dependent - big plus once you have more than two parameters going into queries.

Importantly, @prhost is working on implementing an ORM in Copona 99% likely this will require PDO as well, and for a while there will likely be raw queries AND ORM generated queries being generated. They should all be sent through the same adapter.

Personally, I don’t see ANY downside to making Copona only support the mpdo driver and dropping support for mysqli, along with all the non-MySQL databases. 

IF you want to support other databases in the future, you can through PDO. And will require the same amount of effort as it would currently (rewriting SQL), unless and until @prhost finishes his ORM task.

So, hopefully @arnisjuraga will allow this to be voted on.

[arnisjuraga]

My short comment on this:

1. DB_PREFIX is something consider to stay. I agree, that code is easier to read without it. And again, yes, for self respecting customers and developers, you will have own Dedicated, VPS, or HA server infrastructure, with multiple databases and so on. But there are, yes, edge cases, when this can be handful. E.g. - to have multiple store on the same DB, which can access each other much easier, my sharing data between them. But, if you hardly stick to the tables without prefix, it will be much harder to add it later.
   And as @prhost mentioned before - you don't care about prefix for scripts, as it has been added as core standart, for the same e.g. phinx

2. I agree, that keeping "fake" support is no use.

I am against dropping DB prefix. But besides that - my only concern is about amount of time for rewriting the code and changing habit for SQL queries. I saw, that You have done it already. It still needs to be review for Copona, because we have been based on older Opencart, and it's hard to find the time for this right now.

I have told it already - the best framework for anyone is the one, you know. If someone does not know PDO, this will be drawback for choosing Copona.

[lucasjkr]

1 - The question was strictly about dropping the following drivers from /system/library/db/
mysqli, mssql, postgre.php

2 - There is literally NO change required to switch to the PDO driver today. You just change DB_DRIVER from mysqli to mpdo, and everything will continue to work fine. OC abstracted things away, so $this->db->query($sql) is all you need.

The only changing that happens is if you actually want to use prepared statements. But switching to the PDO driver means you CAN use prepared statements in a simple manner (rather than tediously having to bind each parameter, etc, PDO lets you put your parameters as an array within execute().

Yes, things would have to be reviewed before copying and pasting, mainly because you're based off of OC 2.3, and I forked from 3.0.

Point is, switching copona from using mysqli driver to pdo (and removing non mysql) drivers is:

1 - Delete the 3 files from Library/db/ directory

2 - Change install routine to not allow selecting database driver.

3 - In existing installs, change DB_DRIVER constant from mysqli to mpdo.

Really, that's all you need to do.

Prepare statements are the next step, but you don't have to do that to switch drivers

[lucasjkr]

Secondly as for DB_PREFIX goes, if you insist on keeping it for those cases, I would still suggest stripping it from the existing SQL (simple find and replace), and replace with cp_.

Thereafter, if you need a different table prefix for a site, you can build a function in the query() method to scan the SQL and replace cp_ with your_prefix_.

You still get to have prefixes, and your database code becomes a little readable at least.

Anything to improve readability of OC SQL is good. But wading through event the simplest query like: SELECT * FROM " . DB_PREFIX . "product p LEFT JOIN " . DB_PREFIX . "product2 p2 ON p.product_id = p2.product_id WHERE p.name = " . ($string)$this->db->escape($data['variable']) is horrendous enough, once it has a few tables and variables, might as well forget about ever understanding what the code is.

Just offering a suggestion. We have slightly different ideas/goals, but really, I'm just trying to make all of our lives at least a little easier (as are you, @prhost and everyone else :)

The best part about OC, I think, is the database that's been built. It's extensive, and would take a long time to recreate. But the decisions that have been made long before us about how to implement the SQL and PHP makes the code extremely difficult to edit/manipulate or anything else...

[ADDCreative]

Prepare statements would be an improvement. It's so easy to make a mistake with the escaping and casting in the SQL, that I see a extensions with them missing all the time. However I can't see the point of removing the other drivers until are repare statements actually used and make the incompatible or flawed.

[lucasjkr]

@ADDCreative - if you look at the rewrites I've done over on http://github.com/lucasjkr/opencommerce (look in common/model as I'm merging models together), many of the models have already been rewritten to use prepared statements. If Copona switched to PDO only, I could bring over most of them (I started from OC 3.0 vs Copona which is based on OC 2.3).

But that will only work with the mpdo driver. Again, I'm using named placeholders instead of positional placeholders, because it's easier to troubleshoot (placeholders can appear in the array in any order).

Other databases can be supported, but again, it's dependent on dialect. I know that OC's SQL installer wouldn't even create the first table in MSSQL. Unless one of us is going to run Postgres, MSSQL, etc, it seems pointless to say they're supported.