DBTNG: Make it easier to write weird database drivers

- DatabaseStatement should, as PDOStatement, extends Traversable.
- I agree that phpdoc blocks should go in the interface (this can wait for another patch)
- Yes, we want all PDOStatement methods in that interface, that's the whole point, but we don't need to reimplement those in BaseDatabaseStatement, because it will have those from its parent class anyway.

The rest looks great (but a part is from me, so I'm not the most impartial out there...

Can you provide some more insight as to why it is not desirable to extend the base DatabaseStatement. I can _guess_ and it would likely be a good guess, but it would be great to outline your motivations.

In general, +1 for having a proper interface. That's just generally good OO design. There's a couple of places where I want to do that when I've time. We should then also replace type hints elsewhere in the code with DatabaseStatementInterface.

Interfaces in DBTNG, and Drupal in general properly, should all be named FooBarInterface. So DatabaseStatementInterface is the proper name.

Yes we should replicate the PDO methods in DatabaseStatementInterface. If we're going to do an interface, then it should be hypothetically possible to fulfill the interface without extending PDOStatement. However, I don't believe calling parent::fetch() et al will be necessary. By extending PDOStatement we get all of its parent methods, which fulfill the interface, and all is right with the world. (At least I think that's the case. A quick check is recommended.)

We should migrate the docblocks from BaseDatabaseStatement to DatabaseStatementInterface here and now.

+1 on getQueryString(). +1 on fixing the bug in table prefixing. -1 for killing kittens by mixing that into this patch, as the prefixing is a legit bug and the rest is a design refactoring (albeit a good one).

Why SQLite in particular needs to not extend PDOStatement->DatabaseStatement, I don't know. chx?

The better way to handle the constructor, IMO, is to simply += a class name if one was not specified by the driver constructor. To wit:

class DatabaseConnection {
  function __construct($dsn, $username, $password, $driver_options = array()) {
    $database_options += array(
      'statement_class' => 'DatabaseStatementBase', // I'm thinking we may want to use this name instead.
    );

    // Because the other methods don't seem to work right.
    $driver_options[PDO::ATTR_ERRMODE] = PDO::ERRMODE_EXCEPTION;
    // Call PDO::__construct and PDO::setAttribute.
    parent::__construct($dsn, $username, $password, $driver_options);
    $this->setAttribute(PDO::ATTR_STATEMENT_CLASS, array($driver_options['statement_class'], array($this)));
  }
}

And then drivers can simply set the 'statement_class' key before calling parent::__construct() if they are so inclined.

Well poopy. Why is there no way to write such an interface? Did the signature of the class change that much?

The answer to this question from above will determine how we proceed:

Why SQLite in particular needs to not extend PDOStatement->DatabaseStatement, I don't know. chx?

Following a long discussion in #drupal, and the discussion chx mentioned on #php.pecl, here is a new patch.

We implement DrupalStatementInterface, listing only the methods from PDOStatement we support (several methods from PDOStatement are not implemented by all drivers anyway, so we should not support them).

As of why we need this: the short answer is that SQLite doesn't do buffering of queries and only release locks on table when the PDOStatement object has been destroyed, and we can't solve the issue in a subclass of PDOStatement.

The long answer is that we implemented a fake PDOStatement object, that fetches all data from the query. Due to a PHP pecularity, we can't override the Traversable interface of PDOStatement (the one that defines how PHP can support foreach-ing on the object), because that interface is internal to the PHP engine, and has no methods we can override.

We can also remove dump escapeTable() methods.

Added new documentation.

And fixed grammar.

if (!empty($driver_options['statement_class'])) {

That value will never be empty thanks to the += above.

For fetch(), can we include a link to the list of constants on php.net? Or maybe in the interface's docblock, since it's relevant to several methods.

In the docblock for fetchAllAssoc(), there's several extra spaces: "The fetchmode to use. If set to PDO::FETCH_ASSOC, PDO::FETCH_NUM, or" between "to" and "PDO::FETCH_ASSOC".

For fetchObject(), should we specifically say stdClass object? (I'm not sure what the php.net docs say here. Probably we can just do that.)

Should we put all of the pseudo-method definitions from PDOStatement together at the end of the interface so they're easier to see as "for reference only"? It looks like they're in a functionally logical order now. I can go either way here, really.

+class DatabaseStatementBase extends PDOStatement {

Doesn't that need to also implement DatabaseStatementInterface?

Implementation of DatabaseStatementInterface::execute()

We're not doing that on any of the other interface-implementing methods in DBTNG. Those are currently just bare. It's probably better to keep them bare so that OOP-understanding doc parsers (api.module is not one, yet) can do the right thing and default to the parent class. (At least I believe that's what PHPDoc does.)

(Some of the doc comments may be in the existing code, but I figure we may as well fix those as long as we're refactoring stuff.)

Visually this looks good otherwise. Just minor doc tweakage, really. Haven't had a chance to apply and test it yet. Will try to do so as soon as I can unless someone wants to beat me to it. The approach here looks good, though.

Adding back rowCount() that was missing. I prefer to keep the functional order in the interface. It makes much more sense. Fixed all other code comments as per Crell recommendations.

#12 applies cleanly. Looks good on visual inspection.

There was one place in log.inc where you forgot to update the type hint. I've done so in the attached patch, which just passed all database tests (so webchick, you can skip those ). Looks good to me.

It looks to me like + if (!empty($driver_options['statement_class'])) { can't be empty due to the array merge above.

@Dries: it can, if you set it explicitely like that:

    // We don't need a specific PDOStatement class here, we simulate it below.
    $connection_options['statement_class'] = FALSE;

That's what we do in the SQLite implementation.

Committed to CVS HEAD. Thanks! :)

Oups apparently PDO doesn't like to see foreign options in $driver_options. In that case, PDO refused to set the exception mode, thus breaking one test.

Committed to CVS HEAD. Thanks.