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

  118029
June 20, 2022 23:01 Danack@basereality.com (Dan Ackroyd)
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
  118030
June 21, 2022 00:53 php-lists@koalephant.com (Stephen Reay)
Sent from my iPhone
> On 21 Jun 2022, at 06:08, Dan Ackroyd <Danack@basereality.com> wrote: > > 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 > > -- > PHP Internals - PHP Runtime Development Mailing List > 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
  118031
June 21, 2022 01:25 larry@garfieldtech.com ("Larry Garfield")
On Mon, Jun 20, 2022, at 6:01 PM, Dan Ackroyd wrote:
> 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
  118033
June 21, 2022 09:41 rowan.collins@gmail.com (Rowan Tommins)
On 21/06/2022 02:25, Larry Garfield wrote:
> * "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]
  118086
June 24, 2022 15:25 Danack@basereality.com (Dan Ackroyd)
On Tue, 21 Jun 2022 at 10:41, Rowan Tommins collins@gmail.com> wrote:
> > 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
  118085
June 24, 2022 15:21 Danack@basereality.com (Dan Ackroyd)
On Tue, 21 Jun 2022 at 02:26, Larry Garfield <larry@garfieldtech.com> wrote:
> > 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
  118037
June 21, 2022 14:39 ramsey@php.net (Ben Ramsey)
On 6/20/22 18:01, Dan Ackroyd wrote:
> 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
  118099
June 27, 2022 06:50 marc@mabe.berlin (Marc Bennewitz)
On 21.06.22 01:01, Dan Ackroyd wrote:
> 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: 1. 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.
2. 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 >
  118101
June 27, 2022 07:39 php@beccati.com (Matteo Beccati)
On 27/06/2022 08:50, Marc Bennewitz wrote:
> 2. 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/