Database Schema field/column default value is not properly quoted via PDO::quote()

There's a Schema-related test addition in this patch, but not a query related test?

Sorry, now I'm even more confused...

You removed all the changes to schema related classes, but you kept the schema related test addition... (?)

I think this patch should convert everything to use quote() (as in #1). Or is there a special reason for why not?

My comment in #2 only asked for non-schema, but query-related test coverage. There is already some test coverage for quoting values, but the change to quote() might introduce new cases?

In other words, #1 looked pretty good to me. #281058: Schema API: properly quote column names appears to be a stone-age issue, which I'd rather mark as duplicate... I think this issue should cover all instances that need to be fixed.

(That said, I'm not a database system maintainer, so I don't know why the current code doesn't use PDO::quote()...)

Added #1600670: Cannot query Postgres database that has column names with capital letters which seems related.

I'm having trouble differentiation what you mean by which tests are schema and which are query.

All code in a class name containing "schema" is database schema related. Everything else lives in other classes (that may have "query" in their name, but not necessarily).

But I just recognized that I wrongly interpreted the patch, and you're still changing schema code only. The only difference between #1 and #3 appears to be that you removed quoting for field identifiers, only quoting values.

That's correct, see http://php.net/manual/en/pdo.quote.php (+ user comments). PDO::quote() is only meant for values, not fields/identifiers.

That said, there's an interesting user comment that says that quote() turns NULL into '' (possibly when not specifying custom PDO::PARAM_* constants) — does that affect our usage? Let's check whether there is a test for that, and if not, we might need to add one?

Forgot that I wanted to attach interdiff between #1 and #3.

Thanks for the clarifications. I think this patch makes sense.

3: quote.patch queued for re-testing.

Testbot fluke. (Out of disk space, it seems)

Committed 0231d35 and pushed to 8.x. Thanks!

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
+  protected function _escapeDefaultValue($value) {

No need to add a "private" method underscore prefix here.

Also, MySQL's Schema class contains the same code, so why don't we move this helper method into the base Schema class?

1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -631,6 +626,7 @@
    
   +
      protected function _createIndexSql($table, $name, $fields) {
   

   Erroneous newline.

2. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -722,4 +722,20 @@
   +   * Escape the value used as a default value on a column
   

   Should start with a third-person verb and end with a period.

3. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -722,4 +722,20 @@
   +   * @param $value
   

   Either of type mixed or string|null if those are the only possible values at this point.

4. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -722,4 +722,20 @@
   +   * @return
   

   Always appears to be of type string?

23: quote_default_null.patch queued for re-testing.

(#25 is somehow still queued but as noted in the interdiff #23 is the same except for comments and whitespace.)

re-roll

Patch still applies, works and seems like a nice cleanup also.

Committed cfd30d6 and pushed to 8.0.x. Thanks!