Problem/Motivation

In \Drupal\Core\Database\Connection::query the $trim_chars contains two ordinary spaces but should contain one ordinary space and one non-breaking space.

// function or stored procedure in SQL. Trim any trailing delimiter to
// minimize false positives unless delimiter is allowed.
$trim_chars = "  \t\n\r\0\x0B";
if (empty($options['allow_delimiter_in_query'])) {
  $trim_chars .= ';';
}
$query = rtrim($query, $trim_chars);

Proposed resolution

Replace space (U+0020) with No-Break Space (U+00A0) to minimize false positives.

CommentFileSizeAuthor
#10 interdiff-7-10.txt1.56 KBKrzysztof Domański
#10 3143618-10.patch1.49 KBKrzysztof Domański
#7 interdiff-2-7.txt1.24 KBKrzysztof Domański
#7 3143618-7.patch1.82 KBKrzysztof Domański
#2 3143618-2.patch1.82 KBKrzysztof Domański
#2 3143618-test-only.patch781 bytesKrzysztof Domański
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Krzysztof Domański created an issue. See original summary.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Active » Needs review
FileSize
3143618-test-only.patch781 bytes
3143618-2.patch1.82 KB
Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Issue summary: View changes

The last submitted patch, 2: 3143618-test-only.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Instead of using the Unicode character directly should we use the \x escape to embed it in the string, so it's obvious to casual readers that it's not just a normal space?

Beakerboy’s picture

Beakerboy
Primary language English
Location Kentucky
CreditAttribution: Beakerboy as a volunteer commented

Good catch! I knew there were two spaces there, but just assumed they were different. Should this string also include zero width space (U+200B)?

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
FileSize
3143618-7.patch1.82 KB
interdiff-2-7.txt1.24 KB

Addressing #5.

Status: Needs review » Needs work

The last submitted patch, 7: 3143618-7.patch, failed testing. View results

daffie’s picture

daffie CreditAttribution: daffie commented
  1. I am not sure if we are using UTF-8 or unicode in the Drupal project. The non-breaking space is in unicode "\xA0" and in UTF-8 "\xC2\xA0" . See: https://en.wikipedia.org/wiki/Non-breaking_space. 
  2. +++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -726,9 +726,10 @@ public function query($query, array $args = [], $options = []) {
-        // function or stored procedure in SQL. Trim any trailing delimiter to
-        // minimize false positives unless delimiter is allowed.
...
+        // function or stored procedure in SQL. Trim any trailing delimiter
+        // (including non-breaking space) to minimize false positives unless
+        // delimiter is allowed.

    I do not think this change is good, because we are now hard coding the non-breaking space. We are also not mentioning all the other special characters.

@Krzysztof Domański: Thank you for the bug report, the fix and the added testing.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs work » Needs review
FileSize
3143618-10.patch1.49 KB
interdiff-7-10.txt1.56 KB

This can be a simple fix. https://www.php.net/manual/en/language.types.string.php

Note: Unlike the double-quoted and heredoc syntaxes, variables and escape sequences for special characters will not be expanded when they occur in single quoted strings.

Status: Needs review » Needs work

The last submitted patch, 10: 3143618-10.patch, failed testing. View results

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs work » Needs review

Unrelated test failure. #3143812: Random test failure Table 'sequences' already exists.

daffie’s picture

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

The patch look good.
There is testing added for the bug fix.
It is for me RTBC.

I leave it for the committer to decide if there also needs to be testing for the UTF-8 version of the non-breaking space character. Which is "\xC2\xA0". See: https://en.wikipedia.org/wiki/Non-breaking_space.

alexpott’s picture

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

Committed aa97890 and pushed to 9.1.x. Thanks!

  • alexpott committed aa97890 on 9.1.x 
    Issue #3143618 by Krzysztof Domański, daffie, longwave, Beakerboy:...

Status: Fixed » Closed (fixed)

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