Spin off from #3399150: [PP-1] Enable dynamic queries to produce SQL with positional placeholders.
While working on #3399150: [PP-1] Enable dynamic queries to produce SQL with positional placeholders, I got fed up by the low readability of the SQL strings dumped when exceptions occur, both in PHPUnit (or other CLI) output and in site pages.
So I spent some time trying to improve them.
Before CLI
After CLI
Before HTML
After HTML
doctrine/sql-formatter for dumping SQL strings and symfony/var-dumper for dumping the placeholders values.ExceptionHandler implementations.| Comment | File | Size | Author |
|---|---|---|---|
| #2 | html before.png | 308.1 KB | mondrake |
| #2 | html after.png | 159.44 KB | mondrake |
| #2 | cli after.png | 722.37 KB | mondrake |
| #2 | cli before.png | 586.31 KB | mondrake |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
mondrakeComment #3
mondrakeComment #4
mondrakeComment #6
mondrakeComment #7
longwaveCan the SQL formatter actually do the placeholder replacement as well? Sometimes I want to copy and paste the statement and run it manually - and that is always painful (the quoted field names don't help here either, but that's a separate problem)
Having said that it might be better to show the placeholders separately for some cases, so not sure we always want this.
Comment #8
longwaveThis will need to be a runtime dependency if we are formatting runtime exception messages with it.
Comment #9
mondrakeThere’s this https://github.com/doctrine/sql-formatter#compress-query but we’d have to do the placeholder replacements ourselves, which is tricky as it requires escaping that is Connection dependent. The library itself is db agnostic. Maybe we could do that for tests putting up a static flag to say whether a full replaced SQL string should be produced on top. My use case was different from yours, I am mainly checking for named placeholders in strings that should only have positional ones (see parent).
The MR has protection to do this only in dev (symfony/var-dumper is a dev dep, so by extension I thought to limit to dev also the sql formatting).
Comment #10
mondrakeWhy so? Theoretically quoting column names and other db objects should be the most accurate, no?
Comment #11
longwaveWhen I copy and paste the quoted names into an SQL client then the statement won't execute until I manually remove the quotes.
Comment #12
mondrake#11: weird.
Anyway if we get in the business of replacing the placeholders with their concrete escaped values, I think we can also get in the one of removing the quotes from the identifiers.
TBH I think #7 to #11 would be better dealt with in a follow-up: it’s additional req vs the need of showing the statement exactly as is executed by Drupal.
Comment #13
mondrakebtw #3571186: mysqli - skip prepared statement to allow async query execution is exactly doing the replacement of placeholders with their values - but that’s done at execution time and is mysqli only
Comment #14
mondrakeI was really puzzled by #11 so I investigated a bit.
Used a MariaDb client.
The problem is that Drupal produces ANSI standard SQL that uses double quotes to escape identifiers; at least MariaDb SQL client uses backticks by default instead. If you change " with the backtick character, the syntax passes. Alternatively, you can execute
SET sql_mode = 'ANSI,TRADITIONAL';in the CLI before executing a Drupal statement string (which is what Drupal does in the Connection constructor, and that explains why Drupal is not failing) and double quotes will then be recognized as valid quotes for database identifiers.
Comment #15
mondrakeMmm this now will call expensive dumping for every database exception, but many are caught before producing any output. Let’s try to restrict dumping to only when it’s needed.
Comment #16
mondrakeDone #15
Comment #17
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #18
mondrakeComment #19
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #20
mondrakeComment #21
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #22
mondrakeComment #23
mondrakeWe may just pass the value object in SqlDumper::export() instead of the 3 args.