Expose pdo_parse_params to userspace

php.internals

Karoly Negyesi

2 years ago
Hello, Drupal has been using PDO for a long time now but as it doesn't have async support, the project is adding a mysqli driver. A sticky point here is the lack of support for named placeholders which are widely used in Drupal land but mysqli doesn't support them. Currently the project is considering parsing the SQL string in userspace to convert the named placeholders to positional ones. This does seem like a waste because PDO already contains battle tested, performant code to do this: pdo_parse_params. Exposing this to userspace would solve the problem. What do you think? If the maintainers of the project agree to the idea I can take a stab at coding this. It can't be too hard (famous last words). Thanks Karoly Negyesi

Kamil Tekiela

2 years ago
Hi, I have to say I am not a fan of this proposal. While definitely a super nice feature in PDO, it's more of a hack rather than proper feature. Certain RDBMSs support named parameters in prepared statements, but MySQL doesn't. Therefore, the solution implemented in PDO is a hack. It's very flawed and the current implementation has multiple bugs and shortcomings. One could say that some bugs are security issues. For the above reason, I don't think we should support this in mysqli. We cannot reliably implement such feature. We could make a copy of pdo_parse_params and fix as many MySQL issues as possible, but we would never be able to fix it fully due to the nature of MySQL API. If someone wants to implement this in userland, then be my guest. As long as you are aware of the shortcomings and you know how to use it safely then it's fine. Unrelated, but I would not consider the async feature of mysqli useful or even a good reason to abandon PDO. IMHO the async feature is a failed experiment with limited applicability. Regards, Kamil Tekiela

Karoly Negyesi

2 years ago
That's rather concerning on both fronts. Drupal doesn't plan on abandoning PDO, I believe. At least I haven't seen that proposed. Is there anything I could help with fixing pdo_parse_params ? Also what's the problem with mysqli async? With fibers introduced in 8.1 it seems a very good fit , where can I read / help on problems with this pairing? Thanks Karoly Negyesi On Wed, Oct 11, 2023 at 5:47 PM Kamil Tekiela <tekiela246@gmail.com> wrote:

Robert Landers

2 years ago
On Wed, Oct 11, 2023 at 8:36 PM Karoly Negyesi <karoly@negyesi.net> wrote:
> > That's rather concerning on both fronts. > > Drupal doesn't plan on abandoning PDO, I believe. At least I haven't seen > that proposed. > > Is there anything I could help with fixing pdo_parse_params ? > > Also what's the problem with mysqli async? With fibers introduced in 8.1 it > seems a very good fit , where can I read / help on problems with this > pairing? > > Thanks > > Karoly Negyesi > > > On Wed, Oct 11, 2023 at 5:47 PM Kamil Tekiela <tekiela246@gmail.com> wrote: > > > Hi, > > > > I have to say I am not a fan of this proposal. While definitely a super > > nice feature in PDO, it's more of a hack rather than proper feature. > > Certain RDBMSs support named parameters in prepared statements, but MySQL > > doesn't. Therefore, the solution implemented in PDO is a hack. It's very > > flawed and the current implementation has multiple bugs and shortcomings. > > One could say that some bugs are security issues. > > > > For the above reason, I don't think we should support this in mysqli. We > > cannot reliably implement such feature. We could make a copy of > > pdo_parse_params and fix as many MySQL issues as possible, but we would > > never be able to fix it fully due to the nature of MySQL API. > > > > If someone wants to implement this in userland, then be my guest. As long > > as you are aware of the shortcomings and you know how to use it safely then > > it's fine. > > > > Unrelated, but I would not consider the async feature of mysqli useful or > > even a good reason to abandon PDO. IMHO the async feature is a failed > > experiment with limited applicability. > > > > Regards, > > Kamil Tekiela > >
There isn't a PHP event loop, so fibers are basically useless in Drupal, unless Drupal is going to provide their own or use Revolt, or something. In that case, you pretty much have to change your entire programming model to work with it and accept the interrupts. It's funny because fibers were intended to prevent the 'what color is your function' problem, but instead it became the 'I have no idea what color my function is' problem. So, just like with async/await, you have to go all-in or none-at-all, or very carefully craft your stack to handle multiple layers of fibers. Or maybe I've just been "doing it wrong" which is certainly possible.

Jeffrey Dafoe

2 years ago
> I have to say I am not a fan of this proposal. While definitely a super nice > feature in PDO, it's more of a hack rather than proper feature. > Certain RDBMSs support named parameters in prepared statements, but > MySQL doesn't. Therefore, the solution implemented in PDO is a hack. It's very > flawed and the current implementation has multiple bugs and shortcomings. > One could say that some bugs are security issues.
What is the big risk/challenge in converting named bound parameters to positional bound parameters? It seems straightforward. -Jeff

Matteo Beccati

2 years ago
Hi Karoly, Il 11/10/2023 17:03, Karoly Negyesi ha scritto:
> Drupal has been using PDO for a long time now but as it doesn't have async > support, the project is adding a mysqli driver. A sticky point here is the > lack of support for named placeholders which are widely used in Drupal land > but mysqli doesn't support them. Currently the project is considering > parsing the SQL string in userspace to convert the named placeholders to > positional ones. This does seem like a waste because PDO already contains > battle tested, performant code to do this: pdo_parse_params. Exposing this > to userspace would solve the problem. What do you think? If the maintainers > of the project agree to the idea I can take a stab at coding this. It can't > be too hard (famous last words).
TBH I'm not very excited to expose PDO (or other) internal functions to userland. The pdo_parse_params function works on a PDO statement, so I would expect you need a PDO statement to call its userland equivalent, unless some major refactoring takes place. Also, tangentailly related is the fact that the SQL parser is currently hardcoded to MySQL syntax[1] and I was planning to improve that. I had a working PR[2] to allow drivers to have their own SQL parser, but haven't yet managed to find time to go through the RFC process. References: [1] https://externals.io/message/114016 [2] https://github.com/php/php-src/pull/6852 Cheers
-- Matteo Beccati Development & Consulting - http://www.beccati.com/