[RFC] [Under Discussion] PDO driver specific sub-classes

2 years ago by Dan Ackroyd — view source

unread

Hi,

Following previous discussions, here is an RFC to have DB specific
classes for PDO.

https://wiki.php.net/rfc/pdo_driver_specific_subclasses

cheers
Dan
Ack

2 years ago by Stephen Reay — view source

unread

Sent from my iPhone

Hi,

Following previous discussions, here is an RFC to have DB specific
classes for PDO.

https://wiki.php.net/rfc/pdo_driver_specific_subclasses

cheers
Dan
Ack

--

To unsubscribe, visit: https://www.php.net/unsub.php

Hi Dan,

There’s a typo in the heading about the static factory method (pdf vs pdo)

Cheers

Stephen

2 years ago by Larry Garfield — view source

unread

Hi,

Following previous discussions, here is an RFC to have DB specific
classes for PDO.

https://wiki.php.net/rfc/pdo_driver_specific_subclasses

cheers
Dan
Ack

I'm conceptually in favor of this. A few comments:

Any chance I could convince you to move away from the DSN for the factory? Every DB system I've used has had to do some kind of mangling to build the DSN, but almost never takes its own configuration in the same format, which means there's always some amazingly stupid "parse a URL string to its components, then build up a new string to pass to PDO" step that serves no purpose other than to introduce bugs. If user/pass are separate args, why can't we just make type and host separate args, too? Especially with named args now so it's easy to skip or reordr the args. Please?
"Should the sub-classes only be avaiable when support for that DB is compiled in?" - I agree, probably not. Also, "available" is misspelled.
"Create all DB sub-classes?" - I'd say they all should have subclasses, even if empty. It's more consistent that way, and you can then also rely on instanceof giving you useful information rather than "well, it's not one of the special ones, so beyond that, NFI."
Re PQescapeIdentifier - My gut thought is that we should have separate methods for escaping value and identifier in the base class, and then on most DBs they're just identical. That way it's consistent for anyone building on top of it, because they can rely on escapeValue() and escapeIdentifier() always being The Right Thing To Do without worrying about which DB they're using.
"This RFC does not propose adding subclasses for any other PDO database not listed above. " - But does it presume that future RFCs may do so? This could currently be read as "let's not do that" rather than what I think the intent is, which is more "I'm not dealing with that here, talk about that later." If so, that could be clarified.

--Larry Garfield

2 years ago by Rowan Tommins — view source

unread

"Should the sub-classes only be avaiable when support for that DB is compiled in?" - I agree, probably not. Also, "available" is misspelled.

"Create all DB sub-classes?" - I'd say they all should have subclasses, even if empty. It's more consistent that way, and you can then also rely on instanceof giving you useful information rather than "well, it's not one of the special ones, so beyond that, NFI."

It's important to remember that PDO drivers are an open set, each
provided by their own extension. Like any extension, they can be moved
between php-src, PECL, and private repositories. For example, the
pdo_sqlsrv driver for SQL Server is provided and actively maintained
directly by Microsoft.

I think that goes some way to answering these two questions:

The sub-classes need to be provided by the individual driver
extensions, because ext/pdo itself can't know which drivers might exist,
and what methods they might add. So if you don't have a particular
driver installed, the class will be undefined, just like anything else
provided by an extension.
The RFC cannot itself promise to create all possible sub-classes. What
it can and perhaps should do is mandate that every driver must define
such a sub-class as part of whatever registration mechanism is used.
(Logically, that implies that all bundled extensions would have the
sub-class added as part of the RFC's implementation.)

Re PQescapeIdentifier - My gut thought is that we should have separate methods for escaping value and identifier in the base class, and then on most DBs they're just identical. That way it's consistent for anyone building on top of it, because they can rely on escapeValue() and escapeIdentifier() always being The Right Thing To Do without worrying about which DB they're using.

I agree that it should be a generic feature, but it MUST NOT have any
kind of default implementation. The implementation MUST be written by
someone with intimate knowledge of the DBMS in question, or it cannot be
trusted.

The syntax for identifiers varies a lot between and even within DBMSes.
To use SQL Server as an example, because I have it to hand, "a'b",
"a''b", and "a'b" are all different identifiers; but "a""b" and [a"b]
are different ways of writing the same thing. Select 1 as 'a' is
accepted as valid syntax; but Create Table Foo ( 'a' int ) is not. There
are also environment settings that can change some of this behaviour.

So if we only have a Postgres implementation right now, we should either
implement quoteIdentifier only on PDOPostgres; or implement it on PDO as
throwing a "not implemented" exception, with PDOPostgres currently the
only sub-class that over-rides it with a useful implementation.

Regards,

--
Rowan Tommins
[IMSoP]

2 years ago by Dan Ackroyd — view source

unread

The implementation MUST be written by
someone with intimate knowledge of the DBMS in question, or it cannot be
trusted.

Well, that rules me out of doing it.

So if we only have a Postgres implementation right now, we should either
implement quoteIdentifier only on PDOPostgres

I'll expose it as a method on the PDOPostgres class. Having a quote
identifier function on the PDO class itself that does the appropriate
thing for all of the databases will be for another RFC, so that is
also added to future scope.

cheers
Dan
Ack

2 years ago by Dan Ackroyd — view source

unread

Any chance I could convince you to move away from the DSN for the factory?

No. That sounds like a challenge for someone who knows about (and
cares about) that problem. There's also not enough time to have that
discussion before feature freeze.

"This RFC does not propose adding subclasses for any other PDO database not listed above. " - But does it presume that future RFCs may do so? This could currently be read as "let's not do that" rather than what I think the intent is, which is more "I'm not dealing with that here, talk about that later." If so, that could be clarified.

Yes, I've hopefully made the words clearer.

Other points answered in subsequent emails.

cheers
Dan
Ack

2 years ago by Ben Ramsey — view source

unread

Hi,

Following previous discussions, here is an RFC to have DB specific
classes for PDO.

https://wiki.php.net/rfc/pdo_driver_specific_subclasses

cheers
Dan
Ack

Is there a reason we shouldn't go ahead and add subclasses for all
database connection types, even if they won't have new methods added to
them?

For example, if I create a connection to MySQL with PDO::connect, I
would get back a PDOMysql instance, and so forth.

--
Cheers,
Ben

2 years ago by Marc Bennewitz — view source

unread

Hi,

Following previous discussions, here is an RFC to have DB specific
classes for PDO.

https://wiki.php.net/rfc/pdo_driver_specific_subclasses

Hi Dan,

Thanks for your RFC!

I do have some recommendations for it:

It would be great if driver specific constants would be added to the
driver specific sub-classes without the driver name repeated in the
const name. The counterparts living directly on PDO could be deprecated
(later).

PDO::SQLITE_DETERMINISTIC (int)
|> ||PDO::MYSQL_ATTR_USE_BUFFERED_QUERY |(int|)|
etc.

Another annoying PDO feature is the configurable behavior of exceptions.
As now exceptions are the default way it would be nice to not all disabling exceptions if instantiated the new way.

Create all DB sub-classes?

yes please

PQescapeIdentifier

I think these should be a general PDO method but as we already have
quote it would be preferable to name it quoteIdentifier or how would
you explain the difference between quote and escapeIdentifier?

Thanks

Marc

cheers
Dan
Ack

2 years ago by Matteo Beccati — view source

unread

Another annoying PDO feature is the configurable behavior of exceptions.
As now exceptions are the default way it would be nice to not all
disabling exceptions if instantiated the new way.

Create all DB sub-classes?

yes please

I too think this would be best for consistency.

PQescapeIdentifier

I think these should be a general PDO method but as we already have
quote it would be preferable to name it quoteIdentifier or how would
you explain the difference between quote and escapeIdentifier?

I haven't checked the feasibility, but how about a PDO::QUOTE_IDENTIFIER
constant to be used only with the existing quote method?

e.g.
PDO::quote('A column name', PDO::QUOTE_IDENTIFIER)

That said, I have the feeling this RFC suffers from feature creep and is
subject to a fair amount of bike shedding. To me that's a bit too late
for 8.2.

Cheers

Matteo Beccati

Development & Consulting - http://www.beccati.com/

2 years ago by Dan Ackroyd — view source

unread

Larry Garfield wrote:

"Create all DB sub-classes?" - I'd say they all should have subclasses, even
if empty. It's more consistent* that way, and you can then also rely on
instanceof giving you useful information rather than "well, it's not one of
the special ones, so beyond that, NFI."

You can get the database type through $db->getAttribute(PDO::ATTR_DRIVER_NAME);

Is there a reason we shouldn't go ahead and add subclasses for all
database connection types,

YAGNI, and I'm lazy. But mostly, I don't think adding them now
actually helps achieve anything.

Being able to check if the class has specialised methods is useful:

if ($pdo instanceof PDOSqlite) {
$pdo->loadExtension(...);
// do fun stuff with extension here.
}

But for the classes that have no specialised method:

if ($pdo instanceof PDOMysql) {
// What would be of utility here?
}

cheers
Dan
Ack

How to value being consistent is an aesthetic choice that many other
programmers disagree with me on; "The fundamental guiding force to
bear in mind is whether something is useful or not. Consistency might
satisfy a personal desire for order and simplicity, but most of the
time just having things be consistent is a lower priority than making
choices that are useful." or in this case, less code to write and
maintain.

2 years ago by Rowan Tommins — view source

unread

YAGNI, and I'm lazy. But mostly, I don't think adding them now
actually helps achieve anything.

Fair enough, but as a minimum there should be an internal API for
registering a sub-class, without any modification to ext/pdo.

For instance, it should be possible to modify
https://github.com/microsoft/msphpsql to define a PDOSqlServer class and
have it returned by PDO::connect when a relevant DSN is provided.

It looks like the current PR implements everything directly in ext/pdo,
which feels like a broken abstraction; the SQLite-specific functionality
should all live in ext/pdo_sqlite.

Regards,

Rowan Tommins
[IMSoP]