Problem/Motivation

The PostgreSQL driver overrides the escapeField, escapeAlias and escapeTable methods to deal with case sensitive names and reserved keywords.
Leaving aside the fact that this code has a serious lack of factorization, the table escape introduces a major bug. Currently, the escapeTable method escapes the entire table name as it has been given. The thing is that this table name can include the database and the schema names too.

echo \Drupal::database()->escapeTable('myDb.public.node');

Expected: "myDb".public.node
Current: "myDb.public.node"

Proposed resolution

Factorize for sanity
Explode the table name and handle each part separately

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions Done
Add automated tests Instructions Done
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#12 drupal_pgsql_escape_issue-2726747-12.patch3.66 KBDuaelFr
#12 interdiff.2726747.7.12.txt973 bytesDuaelFr
#7 drupal_pgsql_escape_issue-2726747-7.patch3.42 KBDuaelFr
#7 drupal_pgsql_escape_issue-2726747-7-tests-only.patch718 bytesDuaelFr
#7 interdiff.2726747.3.7.txt1.63 KBDuaelFr
#2 drupal_pgsql_escape_issue-2726747-2.patch2.73 KBDuaelFr
#3 interdiff.2726747.2.3.txt718 bytesDuaelFr
#3 drupal_pgsql_escape_issue-2726747-3.patch3.43 KBDuaelFr
#3 drupal_pgsql_escape_issue-2726747-3-tests-only.patch718 bytesDuaelFr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DuaelFr created an issue. See original summary.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Issue summary: View changes
Status: Active » Needs review
FileSize
drupal_pgsql_escape_issue-2726747-2.patch2.73 KB
DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Issue summary: View changes
Issue tags: -Needs tests
FileSize
drupal_pgsql_escape_issue-2726747-3-tests-only.patch718 bytes
drupal_pgsql_escape_issue-2726747-3.patch3.43 KB
interdiff.2726747.2.3.txt718 bytes

I just added a tiny test to demonstrate the issue.

The last submitted patch, 3: drupal_pgsql_escape_issue-2726747-3-tests-only.patch, failed testing.

The last submitted patch, 3: drupal_pgsql_escape_issue-2726747-3.patch, failed testing.

Crell’s picture

Crell CreditAttribution: Crell at Platform.sh commented 
  1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -246,16 +232,33 @@ public function escapeAlias($field) {
+    // properly and independently escaped. ¶

    Extra space at the end of this line, according to Dreditor. 

  2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -246,16 +232,33 @@ public function escapeAlias($field) {
+  /**
+   * Escape a string if needed.
+   *
+   * @param $string
+   *   The string to escape.
+   * @return string
+   *   The escaped string.
+   */
+  protected function _doEscape($string) {

    There's no need to name methods with a _ prefix if you're on PHP 5.0 or later. :-)

I don't know why the MySQL testbot is affected by this patch. That seems... odd. Other than the above the code looks reasonable on visual inspection.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
FileSize
interdiff.2726747.3.7.txt1.63 KB
drupal_pgsql_escape_issue-2726747-7-tests-only.patch718 bytes
drupal_pgsql_escape_issue-2726747-7.patch3.42 KB

Thanks @Crell for your review.
As I don't really understand why tests failed while they are passing on my local environment, I just updated my patch with your suggestions.
Let's see if the testbot is happier.

The last submitted patch, 7: drupal_pgsql_escape_issue-2726747-7-tests-only.patch, failed testing.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented

The testbot seems quite unstable theses days. Re-requeueing the test.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

The remarks of @Crell in comment #6 are all fixed.

There is a test added to make sure that the problem is fixed.

It all looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php
@@ -40,6 +40,8 @@ public function providerEscapeTables() {
+      // Sometimes, table names are following the pattern database.schema.table.
+      array('"camelCase".nocase', 'camelCase.nocase'),

I think we should add test coverage of the database.schema.table pattern... since the pattern tested here only has one dot.

DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Status: Needs work » Needs review
FileSize
interdiff.2726747.7.12.txt973 bytes
drupal_pgsql_escape_issue-2726747-12.patch3.66 KB

Done :)

bzrudi71’s picture

bzrudi71 CreditAttribution: bzrudi71 commented
Status: Needs review » Reviewed & tested by the community

Looks good to me and we have PG pass too, back to RTBC. Thanks @DuaelFr

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 9233946 and pushed to 8.2.x. Thanks!

Only committing to 8.2.x because of the addition of the protected method.

  • alexpott committed 9233946 on 8.2.x 
    Issue #2726747 by DuaelFr: pgsql improperly escape table names leading...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.