PDO Subdrivers - Continued

[timw4mail]

Clean up the PDO driver, and make it more flexible

[timw4mail]

All tests pass now

[philsturgeon]

@narfbg as somebody who was very involved with the PDO driver changes, would you give feedback on this? I am not too sure how I feel about this.

[narfbg]

@philsturgeon I'm not really sure either. The basic idea behind it is not bad, but if I have to comment on it, I'll just repeat my earlier thoughts: #1237 (comment)

Even if we agree that PDO is a special case, at the very least - altering system/database/DB.php shouldn't be necessary. Also, why is that empty not_exists.sqlite file added?
I'll probably have some comments on individual code chunks, but we should focus on the high level design first.

[timw4mail]

That sqlite file is a slip on my part.

Without altering DB.php, you have to instantiate both the main driver, and the subdrivers, if you want inheritence. The original way I did this was to pass the main driver to the subdriver, but that doesn't allow an automatic inheritence chain.

Basically, the idea is to have the pdo driver to be an abstract super-class for common functionality, and the sub-drivers to function as the other database drivers do.

[narfbg]

I think I might have an idea about a more abstract way of doing this (the DB.php issue), but I don't want another few hours wasted (by you, me, or anybody else who wants to get involved) only to get again to this point where we're not sure if this is what we really want.

Creating the logic isn't that hard and it can be improved and/or fixed later, if needed. But this is more of an architectural thing than anything else ... so, lets first decide on wether we want to separate the logic into subdrivers or not.

The main problem is that PDO is an abstraction layer in its own right, which doesn't really fit in our two-level inheritance tree, where each driver is supposed to represent a single database platform. ODBC is another such "driver" that we have, but it's unlikely that we'd add support for more (simply because there aren't many similar extensions). Let's be honest - there's rarely a good reason to use either of them:

1. You need to access a more "exotic" platform or one that's not supported on your OS.
2. Your system administrator is being a jerk and has limited your options.

For the first one, ODBC has been a good friend of mine, but I don't expect CI to have out-of-the-box support for e.g. Teradata even though it's more logical to want that instead of 3 MySQL drivers. And yet - we have two MySQL drivers, two MSSQL drivers and one for each other popular DB platform and we're trying to get all of them implemented all over again.
Don't get me wrong - PDO is an extremely popular tool and it's only logical to have it working flawlessly and I've put a decent effort in that as well. I'm just not sure wether adding another drivers' layer just for PDO isn't a bit too far.

So, that were the cons, now the pros:

We're trying to make everything as abstract as possible and so does PDO. I'm not the one behind the current DB driver architecture, but I'd do it in a very similar manner even if I haven't seen it. Had it been put all in one (or 4 actually) class, you can all imagine how hard it would be to read through it. It's just logical to have it that way and it's logical that PDO requires a similar architecture in order to be more easily maintanable.

I've already said that this might be going a bit too far, but I also feel that it might be needed.

Even I have a problem reading through the connection string building method that I made in #1032 (and I've tried to make it as readable as possible). Sure, it fits the current architecture, it's faster, superior even, and if you ask me - performance always outweighs readability. 10 out of 10, I'll always choose the ugliest regular expression compared to anything else, just as long as it gives me a 0.00001ms speed improvement. But this is not a two-line regular expression, I'm not the sole developer working on it and CodeIgniter has always honored code readability (it has even been ridiculous at times).
It's a 400+ line method and in order to get the PDO driver working flawlessly and keep the same architecture - we'll have to have multiple of those. If I can't easily walk around my own code, who else would be able to do it?

---

And that pretty much sums it up. I've tried to outline it fairly and as you can see, I can't really say "yes" or "no". If somebody wants to vote though - go for it. Just have it mind that I'm talking about splitting it into subdrivers or not. I'm not talking about this particular pull request/patch.

[timw4mail]

I think PDO has a more limited scope than ODBC...and can therefore be more tuned to each database. The whole point of having several database drivers is to fit the need of the user of the framework. It's a lot easier to fix an issue with a sub-driver, than go through a superdriver to fix a database-specific bug.

PDO is just an abstraction of API, not SQL, so any one-for-all solution isn't going to be great. I think that sub-drivers are probably the easiest way to make the PDO driver more maintable, and ultimately more useful. I think a lot of newer frameworks use PDO drivers exclusively, so this is still pretty lightweight in comparision.

Honestly, I think it's time to think about deprecating the mysql driver, but that's another matter.

[toopay]

Agree with @narfbg point on PDO shouldnt put a specific driver logic into DB.php. And since multiple inheritance is "technically" possible using PHP magic functions(__get, __set, __call) on the pdo_driver.php to get a specific driver(s) method, i dont know why @timw4mail did not consider these option to make the PDO driver following our two-level inheritance tree architecture (and therefore, avoid to messing around with DB.php or other higher abstraction layer). No-one will reject any PDO-specific goodness, and for that purpose (to fully utilize PDO) making another abstraction layer on it(like this one), is inevitable.

[timw4mail]

@toopay If I did it that way, I'd have to hack in inheritence. As far as I know, __get, __set, and __call work only on properties that are not defined - the advantage to the current method is true inheritence.

[toopay]

@timw4mail My points are,

• To make PDO driver follow the nature/idiomatic of regular CI DB driver behaviour. It simply mean any adhoc QB APIs (get, where, etc) should be valid. This already implemented on current PDO driver. If there was a PDO-specific-driver needs, then in pdo_driver.php (with above magic functions set up) you can add something like :

// eg, in some API
function foo()
{
   // check whether we need to run some specific PDO driver method
   if (is_callable(array($this->subdriver, '_foo')) $this->subdriver->_foo();
}

This mean, you can avoid to write an exact same code-block on multiple PDO sub-drivers. This also mean, you dont have to messing around with DB.php. In pdo_driver.php constructor, you could just instantiate any PDO subdriver as subdriver property within pdo_driver.php class.

• By utilize PDO driver this way, it would give anyone possiblitly to run PDO specific syntax. Current PDO driver already can do that too, but with ugly way (look at this sample case)

[narfbg]

So I assume you guys want it separated (as expected).

@philsturgeon Agreed? Or do we need some more opinions?

[philsturgeon]

I'm abstaining from this decision, sorry!

@derekjones @pkriete?

Phil Sturgeon

On Tuesday, 5 June 2012 at 14:37, Andrey Andreev wrote:

So I assume you guys want it separated (as expected).

@philsturgeon Agreed? Or do we need some more opinions?

---

Reply to this email directly or view it on GitHub:
#1237 (comment)

[derekjones]

My position has not changed; it's pushed out of my field of view until after 3.0.

[narfbg]

Okay, so we're kind of split on this. Here's my idea about it:

1. Rename the pdodriver property to subdriver and move it to CI_DB_driver, so it can be used by ODBC as well.

2. Make every (CI database) driver that has subdrivers to determine the current subdriver in it's constructor.

3. Modify the DB() function:

   // Instantiate the DB adapter
   $driver = 'CI_DB_'.$params['dbdriver'].'_driver';
   $DB = new $driver($params);
   
   // --- New stuff below ---
   
   if ( ! empty($DB->subdriver))
   {
       $driver_file = BASEPATH.'database/drivers/'.$DB->dbdriver.'/subdrivers/'.$DB->dbdriver.'_'.$DB->subdriver.'_driver.php';
   
       if (file_exists($driver_file))
       {
           require_once($driver_file);
           $driver = 'CI_DB_'.$DB->dbdriver.'_'.$DB->subdriver.'_driver';
           $DB = new $driver($params);
       }
   }
   

It keeps it optional, which means that we can still have defaults, it takes actual advantage of the class inheritance and it doesn't need platform-specific code to be injected into the base classes. The only disadvantage is that the constructor would be run twice in those cases.

How does that sound? :)

Edit: The double constructor run can be avoided if $params['subdriver'] is set (via config probably).

[timw4mail]

@narfbg I like the idea. Do you want to fork this and start on that conversion? I'm a little tired of this :)

Just...add an additional check for the subdriver property so the constructor isn't called twice.

[narfbg]

@timw4mail No worries, I'll implement it myself. I think I've warned you on some other pull request that such large additions might come to this point where you've just wasted a lot of time. :)

Just making sure we're all for it as if done, as I said - this should be the way it works in all similar cases, so it's kind of a critical decision to make.

[narfbg]

https://github.com/EllisLab/CodeIgniter/compare/feature/db_subdrivers

Did a quick test with MySQL and PostgreSQL. Anybody else want to try it?

Edit: Someone might notice that sqlite2 is not supported. Here's a quote from php.net:

The sqlite2 driver is only available in PHP 5.1.x if you have enabled both PDO and ext/sqlite. It is not currently available via PECL.

[narfbg]

Just merged feature/db_subdrivers into develop. Closing this one.