Hi internals,
I'm looking for feedback on a small pdo_sqlite addition.
PR: https://github.com/php/php-src/pull/21456
Implements: https://github.com/php/php-src/issues/21322
Two new read-only statement attributes on Pdo\Sqlite:
- Pdo\Sqlite::ATTR_SQL — original SQL text of a prepared statement (via
sqlite3_sql()) - Pdo\Sqlite::ATTR_EXPANDED_SQL — SQL with bound parameters inlined (via
sqlite3_expanded_sql())
$stmt = $db->prepare('SELECT :name AS greeting');
$stmt->bindValue(':name', 'hello');
$stmt->execute();
$stmt->getAttribute(Pdo\Sqlite::ATTR_SQL); // "SELECT :name AS
greeting"
$stmt->getAttribute(Pdo\Sqlite::ATTR_EXPANDED_SQL); // "SELECT 'hello' AS
greeting"
This mirrors SQLite3Stmt::getSQL() from the non-PDO API, but uses PDO's
getAttribute() mechanism rather than adding a driver-specific method. The
attribute system is how PDO drivers expose driver-specific functionality,
so it's a natural fit.
ATTR_EXPANDED_SQL is gated behind a configure check for
sqlite3_expanded_sql availability.
I don't think this needs a full RFC given the scope (two read-only
attributes, single driver, no BC impact), but I wanted input from the list.
Thoughts?
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws
Hi internals,
I'm looking for feedback on a small pdo_sqlite addition.
PR: https://github.com/php/php-src/pull/21456
Implements: https://github.com/php/php-src/issues/21322Two new read-only statement attributes on Pdo\Sqlite:
- Pdo\Sqlite::ATTR_SQL — original SQL text of a prepared statement (via sqlite3_sql())
- Pdo\Sqlite::ATTR_EXPANDED_SQL — SQL with bound parameters inlined (via sqlite3_expanded_sql())
$stmt = $db->prepare('SELECT :name AS greeting');
$stmt->bindValue(':name', 'hello');
$stmt->execute();$stmt->getAttribute(Pdo\Sqlite::ATTR_SQL); // "SELECT :name AS greeting"
$stmt->getAttribute(Pdo\Sqlite::ATTR_EXPANDED_SQL); // "SELECT 'hello' AS greeting"This mirrors SQLite3Stmt::getSQL() from the non-PDO API, but uses PDO's getAttribute() mechanism rather than adding a driver-specific method. The attribute system is how PDO drivers expose driver-specific functionality, so it's a natural fit.
ATTR_EXPANDED_SQL is gated behind a configure check for sqlite3_expanded_sql availability.
I don't think this needs a full RFC given the scope (two read-only attributes, single driver, no BC impact), but I wanted input from the list.
Thoughts?
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws
I agree with Saki that a dedicated function would be better than
treating this as an attribute. However, I fail to understand why this
is useful at all. SQL is something that is coming from the developer
using the function, so why would they need to look it up again from
the prepared statement? The emulated statement could be useful in case
an error is triggered due to a data issue, but not only does this
happen rarely, but the error already contains all the necessary
information. I think I would like to see a concrete use case of when
this is helpful.
In the context of the SQLite extension, this makes 100% sense as a separate
function. In the context of PDO which tries to be generic when fetching
essentially meta-data IMO it makes more sense to fetch via the attribute
framework which is generic as opposed to creating driver-specific
functions.
In terms of utility this primarily be helpful for debugging / analysis
purposes and completes the driver implementation, making what PDO offers
match what the native extension offers. It doesn't impact performance
etc... so no negative effect that I can see
Hi internals,
I'm looking for feedback on a small pdo_sqlite addition.
PR: https://github.com/php/php-src/pull/21456
Implements: https://github.com/php/php-src/issues/21322Two new read-only statement attributes on Pdo\Sqlite:
- Pdo\Sqlite::ATTR_SQL — original SQL text of a prepared statement (via
sqlite3_sql())- Pdo\Sqlite::ATTR_EXPANDED_SQL — SQL with bound parameters inlined (via
sqlite3_expanded_sql())$stmt = $db->prepare('SELECT :name AS greeting');
$stmt->bindValue(':name', 'hello');
$stmt->execute();$stmt->getAttribute(Pdo\Sqlite::ATTR_SQL); // "SELECT :name AS
greeting"
$stmt->getAttribute(Pdo\Sqlite::ATTR_EXPANDED_SQL); // "SELECT 'hello'
AS greeting"This mirrors SQLite3Stmt::getSQL() from the non-PDO API, but uses PDO's
getAttribute() mechanism rather than adding a driver-specific method. The
attribute system is how PDO drivers expose driver-specific functionality,
so it's a natural fit.ATTR_EXPANDED_SQL is gated behind a configure check for
sqlite3_expanded_sql availability.I don't think this needs a full RFC given the scope (two read-only
attributes, single driver, no BC impact), but I wanted input from the list.Thoughts?
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.wsI agree with Saki that a dedicated function would be better than
treating this as an attribute. However, I fail to understand why this
is useful at all. SQL is something that is coming from the developer
using the function, so why would they need to look it up again from
the prepared statement? The emulated statement could be useful in case
an error is triggered due to a data issue, but not only does this
happen rarely, but the error already contains all the necessary
information. I think I would like to see a concrete use case of when
this is helpful.
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws
In the context of the SQLite extension, this makes 100% sense as a separate function. In the context of PDO which tries to be generic when fetching essentially meta-data IMO it makes more sense to fetch via the attribute framework which is generic as opposed to creating driver-specific functions.
In terms of utility this primarily be helpful for debugging / analysis purposes and completes the driver implementation, making what PDO offers match what the native extension offers. It doesn't impact performance etc... so no negative effect that I can see
That's the thing. I still don't understand what kind of code would
necessitate the use of this function. What would the debugging code
look like? Why can it not be achieved with the existing functionality?
Not everything deserves to be brought from SQLite3 to PDO.
When a query is structured (generated), easily getting the final value has
merit. This is especially true when you need to see how the bound
parameters were substituted, which might occur in a different file or
function, therefore, having the ability to fetch that clearly adds value,
in my opinion.
One other scenario can be audit trails and logging, if you wanted to
capture changes made for update/insert operations without actually
capturing the data, using $stmt->getAttribute(Pdo\Sqlite::ATTR_SQL); to
get the query without the data could also be a valid use-case for this
functionality.
I am somewhat curios as to the reasoning for the objection, give that the
functionality does not add any meaningful complexity, performance impact or
any other conceivable downsides I can think of, perhaps you can share some
your reasoning?
In the context of the SQLite extension, this makes 100% sense as a
separate function. In the context of PDO which tries to be generic when
fetching essentially meta-data IMO it makes more sense to fetch via the
attribute framework which is generic as opposed to creating driver-specific
functions.In terms of utility this primarily be helpful for debugging / analysis
purposes and completes the driver implementation, making what PDO offers
match what the native extension offers. It doesn't impact performance
etc... so no negative effect that I can seeThat's the thing. I still don't understand what kind of code would
necessitate the use of this function. What would the debugging code
look like? Why can it not be achieved with the existing functionality?Not everything deserves to be brought from SQLite3 to PDO.
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws
When a query is structured (generated), easily getting the final value has merit. This is especially true when you need to see how the bound parameters were substituted, which might occur in a different file or function, therefore, having the ability to fetch that clearly adds value, in my opinion.
One other scenario can be audit trails and logging, if you wanted to capture changes made for update/insert operations without actually capturing the data, using $stmt->getAttribute(Pdo\Sqlite::ATTR_SQL); to get the query without the data could also be a valid use-case for this functionality.
I am somewhat curios as to the reasoning for the objection, give that the functionality does not add any meaningful complexity, performance impact or any other conceivable downsides I can think of, perhaps you can share some your reasoning?
Hi Ilia,
First of all, one quick note: there is a convention here that you should not post above the quoted text, so please keep that in mind.
As a basic premise, retrieving the SQL is already possible at present:
$sql = $stmt->queryString;
What you are trying to implement for SQLite this time makes it possible to retrieve the SQL in either its pre-binding or post-binding state, so it is slightly more capable than the property above.
That said, these are almost certainly very similar features, and from the user’s point of view, being able to retrieve SQL from a property in one case and from an attribute in another may feel somewhat inconsistent.
(I would also add that, personally, SQL does not feel like an “attribute” to me.)
If so, I think it would be better to make these new features methods instead, which would also make it clearer that they are SQLite-specific functionality.
This has another advantage as well: it would make it possible to declare the return type explicitly.
Regards,
Saki
First of all, one quick note: there is a convention here that you should
not post above the quoted text, so please keep that in mind.
Sorry my bad. PHP convention is a bit different from the mail client
default. I will improve, but habits take a while to break, so I can't
promise this is the last one :)
As a basic premise, retrieving the SQL is already possible at present:
$sql = $stmt->queryString;What you are trying to implement for SQLite this time makes it possible to
retrieve the SQL in either its pre-binding or post-binding state, so it is
slightly more capable than the property above.
This is a slightly different functionality and connotation as you've
correctly pointed out. Therefore, it definitely warrants having it due to
the different functionality/capability.
If so, I think it would be better to make these new features methods
instead, which would also make it clearer that they are SQLite-specific
functionality.This has another advantage as well: it would make it possible to declare
the return type explicitly.
I don't feel strongly about one way or the other. I'm happy to revise this
into a function if the consensus is that everyone is more comfortable with
parameters allowing either pre/post binding state to be returned, probably
defaulting to the pre-binding state.
I don't think 2 methods are needed for functionality that is functionally
very similar.
That being said, I still prefer an attribute because this isn't an
actionable function (like sqliteCreateFunction) but rather something that
retrieves information, which to me seems more appropriate for attribute
retrieval similar to Pdo\Pgsql::ATTR_RESULT_MEMORY_SIZE for example.
I don't think this warrants its own function and the value of setting a
return type seems minimal.
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws
I don't feel strongly about one way or the other. I'm happy to revise this
into a function if the consensus is that everyone is more comfortable with
parameters allowing either pre/post binding state to be returned, probably
defaulting to the pre-binding state.
I don't think 2 methods are needed for functionality that is functionally
very similar.That being said, I still prefer an attribute because this isn't an
actionable function (like sqliteCreateFunction) but rather something that
retrieves information, which to me seems more appropriate for attribute
retrieval similar to Pdo\Pgsql::ATTR_RESULT_MEMORY_SIZE for example.
I don't think this warrants its own function and the value of setting a
return type seems minimal.
One more thing :)
Looking at pdo/PgSQL and pdo/SQlite they both limit custom functions to
things that change data. However, pdo/MySQL and pdo/Firebird break the
convention, each has a single custom function for info retrieval, so there
isn't technically as convention universally followed convention here.
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws
Sorry my bad. PHP convention is a bit different from the mail client default. I will improve, but habits take a while to break, so I can't promise this is the last one :)
No worries, I make that kind of mistake all the time too :)
This is a slightly different functionality and connotation as you've correctly pointed out. Therefore, it definitely warrants having it due to the different functionality/capability.
Implementing every feature that exists would increase the maintenance burden on the codebase, so it may be worth being a little cautious in deciding whether this should be implemented.
I suspect this is probably something Kamil is also concerned about.
Of course, if a good use case becomes clear in the discussion going forward, I would not oppose implementing it.
I don't feel strongly about one way or the other. I'm happy to revise this into a function if the consensus is that everyone is more comfortable with parameters allowing either pre/post binding state to be returned, probably defaulting to the pre-binding state.
I don't think 2 methods are needed for functionality that is functionally very similar.That being said, I still prefer an attribute because this isn't an actionable function (like sqliteCreateFunction) but rather something that retrieves information, which to me seems more appropriate for attribute retrieval similar to Pdo\Pgsql::ATTR_RESULT_MEMORY_SIZE for example.
I don't think this warrants its own function and the value of setting a return type seems minimal.
Looking at pdo/PgSQL and pdo/SQlite they both limit custom functions to things that change data. However, pdo/MySQL and pdo/Firebird break the convention, each has a single custom function for info retrieval, so there isn't technically as convention universally followed convention here.
I’m sorry, I just realized something very important.
The place where this should be implemented is the statement class, not the driver, right?
Drivers have subclasses, but statements do not.
So implementing this as a specialized method would not really be very practical after all…
One possible approach would be to add a method on the driver and pass the statement as an argument, but that seems very confusing to me.
So in this case, the implementation approach used in your PR may be the only realistic option after all…
Regards,
Saki
The place where this should be implemented is the statement class, not the
driver, right?
Drivers have subclasses, but statements do not.
So implementing this as a specialized method would not really be very
practical after all…
Technically yes, but practically I think the complexity of that approach
would likely outweigh its utility, hence the current approach.
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws
Technically yes, but practically I think the complexity of that approach would likely outweigh its utility, hence the current approach.
OK I see, thank you.
I do not think there is much room left for discussion regarding the implementation approach.
As for demand, I tried the following search on GitHub code search:
/(?-i)->getSQL\(true/ language:PHP
It returns roughly 500 hits (although, of course, there is no guarantee that all of them are from the SQLite3 extension).
Looking through the code at a high level, it seems to be used mainly for SQL reuse and debugging.
It is certainly possible to log the SQL before binding and the parameters to be bound separately, but being able to retrieve the actually assembled SQL may still be convenient.
Regards,
Saki
Sorry my bad. PHP convention is a bit different from the mail client
default. I will improve, but habits take a while to break, so I can't
promise this is the last one :)No worries, I make that kind of mistake all the time too :)
Back in the early days of email, replying after the thing being replied
to was the convention: when replying, the body of the email would be
quoted and the cursor would be placed on the line following. Users would
edit and trim, and follow quoted sections with their replies to those
sections.
When Microsoft Outlook came out, the cursor was placed at the top,
before the quoted email. The thinking was that people would go through
the quoted text, editing as they went and putting their replies after
the bits they were replying to, until they got to the bottom.
No-one who used Microsoft Outlook did that. They just hit "Reply" and
started typing.
And that's how that habit started.
This has been your diversionary history lesson of the day.
Hi internals,
I'm looking for feedback on a small pdo_sqlite addition.
PR: https://github.com/php/php-src/pull/21456
Implements: https://github.com/php/php-src/issues/21322Two new read-only statement attributes on Pdo\Sqlite:
- Pdo\Sqlite::ATTR_SQL -- original SQL text of a prepared statement
(via sqlite3_sql())- Pdo\Sqlite::ATTR_EXPANDED_SQL -- SQL with bound parameters inlined
(via sqlite3_expanded_sql())$stmt = $db->prepare('SELECT :name AS greeting');
$stmt->bindValue(':name', 'hello');
$stmt->execute();$stmt->getAttribute(Pdo\Sqlite::ATTR_SQL); // "SELECT :name AS
greeting"
$stmt->getAttribute(Pdo\Sqlite::ATTR_EXPANDED_SQL); // "SELECT 'hello'
AS greeting"This mirrors SQLite3Stmt::getSQL() from the non-PDO API, but uses PDO's
getAttribute() mechanism rather than adding a driver-specific method.
The attribute system is how PDO drivers expose driver-specific
functionality, so it's a natural fit.ATTR_EXPANDED_SQL is gated behind a configure check for
sqlite3_expanded_sql availability.I don't think this needs a full RFC given the scope (two read-only
attributes, single driver, no BC impact), but I wanted input from the
list.Thoughts?
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws
Hello,
I don't doubt the need to extract statement strings since logging the
executed statements is something I do often, especially during
development (tools like laravel-debugbar and newrelic's achieve that).
It helps noticing repeated queries and other issues.
But two things here are unclear to me:
- What's insufficient with
->queryStringand other existing features?
Does one of these overlap with->queryStringor not? - What's so special about SQLite? Wouldn't the same need be just as
present for any other driver?
BR,
Juris
But two things here are unclear to me:
- What's insufficient with
->queryStringand other existing features?
Does one of these overlap with->queryStringor not?
The output of ATTR_SQL and queryString are essentially the same;
ATTR_EXPANDED_SQL is the novelty that gives you access to the resolved
prepared statement. So the ATTR_SQL is there mostly for completion, if
ever something changes in terms of how PHP does pre-prepare handling this
could have utility.
- What's so special about SQLite? Wouldn't the same need be just as present
for any other driver?
Nothing special, just that it provides library functionality to get this
data programmatically.
--
Ilia Alshanetsky
Technologist, CTO, Entrepreneur
E: ilia@ilia.ws
T: @iliaa
B: http://ilia.ws