The is some subtle unexpected behaviour going on in \Drupal\Core\Database\Connection. I suspect it is just me so I went through the documentation looking for anything that did not match my expectations.

It was enlightening to me to find out just when Connection::query returns null

In here there are lots of trivial corrections, the one useful change is the annotation of $schema
This is also a trivial error in the annotation of setKey()

CommentFileSizeAuthor
#2 interdiff-0-2.txt3.47 KBmartin107
#2 connection-2345915-2.patch15.02 KBmartin107
fixup-0.patch15.07 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks! Mostly great fixes. A few things to fix:

a)

-   * @var Log
+   * @var \Drupal\Core\Database\Log logger

This is not the correct syntax. It should just be @var [type].

b)

+   * @var \Drupal\Core\Database\Schema|Null

In @var/@param/@return types, we use lower-case "null" not "Null" or "NULL". This occurs in other spots in the patch as well.

c)

+   *
+   * @param string $table
+   *   (optional) The table to be searched.
    */
   public function tablePrefix($table = 'default') {

I don't think I like the documentation of this parameter much. The table is not being searched. Maybe "The table to find the prefix for"?

d)

+   * @param string $table
+   *   The name of the table associated with the query.

I wouldn't say a table is "associated with" a query. Maybe "The name of the table to query" would be better here? This occurs several times in the patch.

e)

+   * @param string $database
+   *   The unescaped database name.
    * @return string
    *   The sanitized database name string.

We need a blank line between @param and @return. This applies to a few places in the patch.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
connection-2345915-2.patch15.02 KB
interdiff-0-2.txt3.47 KB

Thank you I do appreciate how long it takes to double check these patches.

All comments make the thing look better. I will try and carry the null thing into the next issue.

a) Fixed
b) Fixed
c) Fixed
d) Fixed
e) Fixed

PS I can stop crying wolf now ... there is no unexpected behaviour it really WAS just me. :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

(b) is not fixed in the 2nd hunk in the patch:

+   * @var \Drupal\Core\Database\Schema|Null
    */
   protected $schema = NULL;

The rest looks good -- one more pass should do it. Thanks!

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs work » Postponed

I found a duplicate

#2342521: Docblock fixes for core/lib/Drupal/Core/Database/Connection.php

I will postpone until the other is committed ... and learn to work in a team where possible :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Postponed » Closed (duplicate)

Shouldn't this just be closed as a duplicate then? You can merge the changes here into the other patch?