Problem/Motivation

Static queries should have the field names bracket-encapsulated. In that way static queries cannot fail on a database, because a field name is a reserved keyword for the given database. For more info: https://www.drupal.org/node/2986894.

Proposed resolution

Bracket-encapsulated field names for all static queries located in the directory "core/lib/Drupal/Core" and its sub-directories, but not in the directory "core/lib/Drupal/Core/Database". Static queries can be found by doing a search for "->query(" and "->queryRange(" in the directory "core/lib/Drupal/Core" and its sub-directories, but not in the directory "core/lib/Drupal/Core/Database".

What needs to be changed:

From:
$connection->query('SELECT nid FROM {node} WHERE nid = :id', [':id' => 1']);

To:
$connection->query('SELECT [nid] FROM {node} WHERE [nid] = :id', [':id' => 1']);

Remaining tasks

TBD

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

TBD

CommentFileSizeAuthor
#14 interdiff.txt602 bytesLal_
#14 3152415-14.patch18.26 KBLal_
#11 interdiff.txt6.22 KBLal_
#11 3152415-11.patch17.55 KBLal_
#8 interdiff.txt20.97 KBLal_
#8 3152415-9.patch11.62 KBLal_
#5 Screen Shot on 2020-06-18 at 15:23:07.png344.29 KBmatroskeen
#4 3152415-3.patch35.64 KBrik-dev
#3 3152415-3.patch60.32 KBrik-dev

Comments

daffie created an issue. See original summary.

rik-dev’s picture

rik-dev commented
Assigned: Unassigned » rik-dev
rik-dev’s picture

rik-dev commented
Status: Active » Needs review
StatusFileSize
new 3152415-3.patch60.32 KB
rik-dev’s picture

rik-dev commented
StatusFileSize
new 3152415-3.patch35.64 KB
matroskeen’s picture

matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
commented
StatusFileSize
new Screen Shot on 2020-06-18 at 15:23:07.png344.29 KB

@rik-dev, please take a look into task description:

located in the directory "core/lib/Drupal/Core" and its sub-directories, but not in the directory "core/lib/Drupal/Core/Database"

It seems that only a small part of your patch covers the actual scope of this task (see the screenshot).

daffie’s picture

daffie commented
Issue summary: View changes
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
@@ -100,7 +100,7 @@ protected function doCreateItem($data) {
+      return (int) $this->connection->query('SELECT COUNT(item_id) FROM {' . static::TABLE_NAME . '} WHERE [name] = :name', [':name' => $this->name])

    The field "item_id" also needs square brackets.

  2. There are queries in "core/lib/Drupal/Core/Config/DatabaseStorage.php" that need to be changed.
  3. There are queries in "core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php" that need to be changed.
  4. There are queries in "core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php" that need to be changed.
  5. There are queries in "core/lib/Drupal/Core/Lock/DatabaseLockBackend.php" that need to be changed.
  6. There are queries in "core/lib/Drupal/Core/Queue/DatabaseQueue.php" that need to be changed.
  7. There are queries in "core/lib/Drupal/Core/Routing/RouteProvider.php" that need to be changed.
  8. There are queries in "core/lib/Drupal/Core/Test/EnvironmentCleaner.php" that need to be changed.
  9. There are queries in "core/lib/Drupal/Core/Extension/module.api.php" that need to be changed.
  10. There are queries in "core/lib/Drupal/Core/Form/form.api.php" that need to be changed.

Also is @Matroskeen right about his comment #5.

daffie’s picture

daffie commented
Category: Task » Bug report
Issue tags: +Bug Smash Initiative

The parent issue is a "bug report", therefore this issue is one too.

Lal_’s picture

Lal_
He\Him
commented
StatusFileSize
new 3152415-9.patch11.62 KB
new interdiff.txt20.97 KB
Lal_’s picture

Lal_
He\Him
commented
Status: Needs work » Needs review
daffie’s picture

daffie commented
Status: Needs review » Needs work

The patch is now a lot better.

+++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
@@ -100,7 +100,7 @@ protected function doCreateItem($data) {
+      return (int) $this->connection->query('SELECT COUNT[item_id] FROM {' . static::TABLE_NAME . '} WHERE [name] = :name', [':name' => $this->name])

This change is not correct. It should be COUNT([item_id]).

Still open are: #6.2, #6.5, #6.8, #6.9, #6.10.

Lal_’s picture

Lal_
He\Him
commented
Status: Needs work » Needs review
StatusFileSize
new 3152415-11.patch17.55 KB
new interdiff.txt6.22 KB

@daffie made the changes

The last submitted patch, 8: 3152415-9.patch, failed testing. View results

daffie’s picture

daffie commented
Status: Needs review » Needs work

@Lal_: Thanks for working on the patch. It look good, only we have missed a query in "core/lib/Drupal/Core/Session/SessionHandler.php".

Lal_’s picture

Lal_
He\Him
commented
Status: Needs work » Needs review
StatusFileSize
new 3152415-14.patch18.26 KB
new interdiff.txt602 bytes

I hope this is it @daffie

Status: Needs review » Needs work

The last submitted patch, 14: 3152415-14.patch, failed testing. View results

daffie’s picture

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

All fields are now encapsulated by square brackets.
All code changes look good.
The testbot is happy for all 3 by core supported databases.
For me it is RTBC.

Thank you for working on this issue: @Lal_, @rik-dev and @Matroskeen.

rik-dev’s picture

rik-dev commented
Assigned: rik-dev » Unassigned

  • catch committed 638f25e on 9.1.x 
    Issue #3152415 by Lal_, rik-dev, Matroskeen, daffie: Bracket-...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 638f25e and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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