Problem/Motivation

The docblocks for Connection::select and Select::__construct only describe $table as a string, but in reality it can also be a subquery in form of a SelectInterface object (for example, for rowcount queries).

Proposed resolution

Fix

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3216556-14.patch1.65 KBguilhermevp
#10 3216556-10.patch1.76 KBsudiptadas19
#6 3216556-6.patch1.78 KBsudiptadas19
#2 3216556-2.patch1.75 KBmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Active » Needs review
FileSize
3216556-2.patch1.75 KB
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Documentation

Looks much better

daffie’s picture

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

Two nitpicks:

  1. +++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1129,10 +1129,11 @@ public function exceptionHandler() {
+   *   query_alter hook implementations.
+   *   If a SelectInterface object, a subquery.

    Not outlined correctly. 

  2. +++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -123,8 +123,9 @@ class Select extends Query implements SelectInterface {
+   *   If a string, the name of the table that is being queried.  If a

    Double space.

The documentation text is +1 for me.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Issue tags: +Novice
sudiptadas19’s picture

sudiptadas19 CreditAttribution: sudiptadas19 at QED42 for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3216556-6.patch1.78 KB

@daffie
Added the changes as suggested. Please review it.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Needs work

@sudipta: You have fixed #4.2 correctly, only #4.1 still needs to be done.

sudiptadas19’s picture

sudiptadas19 CreditAttribution: sudiptadas19 at QED42 for Drupal India Association commented

@daffie:

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1129,10 +1129,11 @@ public function exceptionHandler() {
+ * query_alter hook implementations.
+ * If a SelectInterface object, a subquery.

Not outlined correctly.

You want some text change or how it is, not getting it clearly.

daffie’s picture

daffie CreditAttribution: daffie commented 
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1129,10 +1129,11 @@ public function exceptionHandler() {
+   *   If a SelectInterface object is passed, it will be use as a subquery.

To be more clear. This text does not need to start on a new line. Look at the other change. "If a SelectInterface object ..." also does not start on a new line.

sudiptadas19’s picture

sudiptadas19 CreditAttribution: sudiptadas19 at QED42 for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3216556-10.patch1.76 KB

Given feedback addresses. Please review it.
As it is a small change, not added an interdiff.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1129,10 +1129,11 @@ public function exceptionHandler() {
+   *   query_alter hook implementations. If a
+   *   SelectInterface, it will be use as a subquery.

still not aligned properly

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented 
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1129,10 +1129,11 @@ public function exceptionHandler() {
+   *   query_alter hook implementations. If a
+   *   SelectInterface, it will be use as a subquery.

not a native speaker, but this is incorrect grammar. How about "If a SelectInterface object, it will be treated as a subquery."

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
  1. +++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1129,10 +1129,11 @@ public function exceptionHandler() {
+   *   If a string, the base table for this query, that is, the first table in

    How about: The base table name or subquery for this query, used in the FROM clause. If a string, the table specified will also be used as the "base" table for query_alter hook implementations. 

  2. +++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -123,8 +123,9 @@ class Select extends Query implements SelectInterface {
+   *   If a string, the name of the table that is being queried. If a

    How about just: The table name or subquery that is being queried.

guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented
Status: Needs work » Needs review
FileSize
3216556-14.patch1.65 KB

Made changes suggested in #13.

Please review.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Brilliant!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: Document that the $table argument of Connection::select can be a subquery » Document that the $table argument of Connection::select() can be a subquery

  • xjm committed a314cb1 on 9.3.x 
    Issue #3216556 by sudiptadas19, mondrake, guilhermevp, daffie, andypost...

  • xjm committed 0da8f09 on 9.2.x 
    Issue #3216556 by sudiptadas19, mondrake, guilhermevp, daffie, andypost...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Good find. Committed to9.3.x and cherry-picked to 9.2.x. Thanks everyone!

Status: Fixed » Closed (fixed)

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