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

Everything here looks quite uncontroversial. I hope. :-)

A few issues:

a) List formatting:
https://www.drupal.org/node/1354#lists
For parts of the patch that change/add lists, check on:
- indentation
- There should be a : before the list are problems in this patch
- capitalization of items (unless they are particular strings)

b)

-   * @param $target
+   * @param string $target
    *   The target this connection is for. Set to NULL (default) to disable
    *   logging entirely.

If a value can be NULL, it should be "string|null" in the param type. This one is obviously string|null; seeing this, I wonder about some of the other values that have been marked "string" as well?

c)

+   * @param string $field
+   *
+   * @return string
+   *   The sanitized field name.
    */
   public function escapeField($field) {

No description for $field?

am working on this issue to fix documentation based on comment #3

the attached patch fixes the documentation issues listed in comment 3a

working on items b and c from comment 3

Fix documentation issues mention from comment 3
NOTE: about 3b
there was a question as to weather the rest of the string parameters should have null i did a code scan and the rest seem to be fine

We should do the following tasks:

1. Make an interdiff between patch in #1 and #5 to show that you didn't actually make the list fix.
2. Make a patch to fix the list fixes that @jhodgdon pointed out in her review.
3. Look for better type hinting of the array types. To do this, look at the parameters and return types to see if they return objects and what kind of objects.

4. Figure out why this isn't being sent to test bot.

changes have been made based off comments in #8
add the interdiff file and also the patch file showing all changes

#2345915: \Drupal\Core\Database\Connection documentatiion fixup

I am shutting down a duplicate issue and merging in changes here.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -517,9 +517,9 @@ protected function filterComment($comment = '') {
-   *   - the executed statement,
-   *   - the number of rows affected by the query (not the number matched), or
-   *   - the generated insert ID of the last query.
+   *  	- The executed statement,
+   *  	- The number of rows affected by the query (not the number matched), or
+   *  	- The generated insert ID of the last query.

This doesn't seem to have moved, or else there's an odd character or bug with dreditor? (I see a rectangle in place of one space, but no change in indentation.)

martin: Ugh. We've failed like 3 times to get large doc patches in. We were trying to keep this one *small*.

Sorry for the noise if you choose to continue from #10 then I understand,
and my comment is that the only notable changes are the the annotations to $schema and $log.

The new patch removed the tabs characters which were causing concern.

I would love to accidentally get a ugh from Crell.... but I feel I have, as yet, achieved only a half Ugh... martin-=1/2;

The odd characters are misattributed to the merging ... they exists in #10!

testbot was down earlier - I am just retesting.

Thanks @martin107 for your work so far!
I made some changes.
(I think this is more productive than posting the same points in a review)
https://github.com/donquixote/drupal/compare/8.0.x-2342521-21-DatabaseCo...

Changes:

• "Null" should be "null"
• $logger and getLogger() may be NULL.
• $target and getTarget() may be NULL.
• $key and getKey() may be NULL.
• More type docs added.
• More detailed docblock description on Connection::query()
  This one is taken from another issue where I already made this change.
  I can take it out again if it is out of scope.

I'm cleaning up some Novice tag duplicates. removing the combined "novice documentation" tag so I can delete it, since we should be able to make a search for the novice tag combined with the documentation tag https://www.drupal.org/project/issues/search?projects=&project_issue_fol... or maybe even better: the documentation component combined with the novice tag https://www.drupal.org/project/issues/search/drupal?project_issue_follow...

Reroll of #21. There were some changes in @throws interfering with a clean apply.

Thanks for the reroll! I went through the patch carefully, and there are a few things to fix, although it mostly looks good:

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -138,6 +140,14 @@
   +   *   - Other, driver specific options.
   

   nitpick: should be "driver-specific". Line might be clearer without the comma as well?

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -252,7 +262,7 @@ public function getConnectionOptions() {
   -   * @param $prefix
   +   * @param mixed $prefix
       *   The prefixes, in any of the multiple forms documented in
       *   default.settings.php.
       */
   

   It looks like this is really string|array ?

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -355,8 +368,8 @@ public function prepareQuery($query) {
   +   * @param string|null $target
   +   *   (optional) The target this connection is for. Set to NULL (default) to disable
       *   logging entirely.
       */
      public function setTarget($target = NULL) {
   @@ -368,8 +381,8 @@ public function setTarget($target = NULL) {
   

   This is going over 80 characters.

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -509,26 +522,39 @@ protected function filterComment($comment = '') {
   +   *   one of:
   +   *   - the executed statement, if
   +   *     $options['return'] === self::RETURN_STATEMENT, or if
   +   *     $options['return'] is not set (due to self::defaultOptions()).
   +   *   - the number of rows affected by the query (not the number matched), if
   +   *     $options['return'] === self::RETURN_AFFECTED.
   +   *   - the generated insert ID of the last query, if
   +   *     $options['return'] === self::RETURN_INSERT_ID.
   +   *   - NULL, if
   +   *     $options['return'] === self::RETURN_NULL.
   +   *   If an exception occurs during query execution, this method will
   

   List formatting: we normally want to start text-based list items with capital letters.

   Also this last list item I think would fit all on one line?

5. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -509,26 +522,39 @@ protected function filterComment($comment = '') {
   +   *   If an exception occurs during query execution, this method will
   +   *   - throw an exception, if
   +   *     $options['throw_exception'] evaluates to TRUE, or
   +   *     $options['throw_exception'] is not set (due to self::defaultOptions()).
   +   *   - return NULL, if
   +   *     $options['throw_exception'] evaluates to FALSE.
       *
   

   Line before the list should end in : and list items should start with capital letters. Last one should be wrapped better also.

6. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -509,26 +522,39 @@ protected function filterComment($comment = '') {
   -   * @throws \Drupal\Core\Database\DatabaseExceptionWrapper
   -   * @throws \Drupal\Core\Database\IntegrityConstraintViolationException
       * @throws \InvalidArgumentException
   

   These exceptions cannot be thrown any more???

7. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -942,10 +993,11 @@ public function startTransaction($name = '') {
   +   * @param string $savepoint_name
   +   *   (optional) The name of the savepoint. The default, 'drupal_transaction', will roll
       *   the entire transaction back.
       *
   

   This went over 80 characters.

8. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1083,16 +1138,16 @@ protected function popCommittableTransactions() {
   +   *   (optional) An array of values to substitute into the query at placeholder markers.
   

   Over 80 characters

9. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1122,15 +1177,15 @@ protected function generateTemporaryTableName() {
   +   *   (optional) An associative array of options to control how the query is run. See
   

   Over 80 chars

10. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1142,6 +1197,8 @@ protected function generateTemporaryTableName() {
    +   *
    +   * @return string
        */
       abstract public function driver();
     
    

    Needs docs on the @return, and really is it a string?? Seems unlikely?

11. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1234,7 +1291,7 @@ public function commit() {
    +   *   (optional) After a database import, it might be that the sequences table is behind,
    

    Goes over 80 chars

working on this now.

1) Fixed.
2) Fixed, yes is is array|string
3) Fixed.
4) Fixed.
5) Fixed.
I have fixed another problem here - The docs talk about exceptions throw in methods that this function calls ( see Connection::expandArguments() )
This is not standard, but welcome given the complex nature of the return type.
But what this mean is Connection::expandArguments() should should also document what exceptions it throws.

6) Could not find the reference referred to, but checked all relevant throws and they look fine.
7) Fixed.
8) Fixed.
9) Fixed.
10) It looks like string is ok from looking at the methods that implement it in normal operation, but FakeConnection::driver() does not return a type, instead it throws an exception - which seems a reasonably sane design choice - was that the concern?

Looking at the method below Connection::version() the output of getAttribute() can be unsuccessful. so I have fixed that in the documentation.

11) Fixed.

What I was referring to in item 6 of my last review, is that in Connection.php the patch is removing some lines:

-   * @throws \Drupal\Core\Database\DatabaseExceptionWrapper
-   * @throws \Drupal\Core\Database\IntegrityConstraintViolationException
    * @throws \InvalidArgumentException
+   *
+   * @see \Drupal\Core\Database\Connection::defaultOptions()
    */
   public function query($query, array $args = array(), $options = array()) {

The patch is removing two @throws lines. I do not think it should be removing @throws, at least not in a "docblock fixes" issue. I think those two removals are a mistake? Not sure.

Another nitpick:

+   * @throws \InvalidArgumentException
+   *   - Thrown when placeholders with a trailing [] are not expanded with an
+   *     array of values.
+   *   - Thrown when the placeholder is to be expanded with an array of values
+   *     and it is missing a trailing [].
+   *

Any list in documentation needs to be prefixed with a line ending in : -- so probably this should be:

This exception is thrown when:
- Placeholders ...

Also @throws sections should go at the end of the doc block, after @return, see
https://www.drupal.org/node/1354#order

One more:

+   *
+   * @return string
    */
   abstract public function driver();

@return needs docs here.

I don't see any other problems in the fixes here (I can see some other problems in other doc blocks but oh well).. looking good!

jhogdon : Regarding issue 6 - when rerolling in #25, that is absolutely MY mistake when merging head with the old patch. All 3 are definitely there in

https://github.com/drupal/drupal/blob/8.0.x/core/lib/Drupal/Core/Database/Connection.php#L529-L531,

.. so I was in error to cut them out. I've corrected this and updated with your other requests.

Regards.

Great, almost there! Just the list formatting in:

+   * @throws \InvalidArgumentException
+   *   This exception is thrown when:
+   *     - Thrown when placeholders with a trailing [] are not expanded with an
+   *       array of values.
+   *     - Thrown when the placeholder is to be expanded with an array of values
+   *       and it is missing a trailing [].
    */
   protected function expandArguments(&$query, &$args) {
 

Please see https://www.drupal.org/node/1354#lists -- indentation is not quite right on the list.

Also I do not think we need "thrown when" at the beginning of each item, since it is now at the end of the list prefix.

Hi, thanks for review and reference to List formatting.

Enclosed are requested changes.

Regards.

I still don't think the two items in that list should start with "Thrown when...":

+   *
+   * @throws \InvalidArgumentException
+   *   This exception is thrown when:
+   *   - Thrown when placeholders with a trailing [] are not expanded with an
+   *     array of values.
+   *   - Thrown when the placeholder is to be expanded with an array of values
+   *     and it is missing a trailing [].
    */
   protected function expandArguments(&$query, &$args) {

For instance, I think the first one should be "Placeholders with a trailing...", because the previous line says "This exception is thrown when:" so starting again with "Thrown when" doesn't seem quite right?

Formatting is good though, thanks!

Thanks and apologies +jhodgdon, It seems I completely ignored that portion of your review in #33. Fixed it up.

Great. Everything in this patch now improves the documentation. Not saying that the docs are perfect, but they're certainly better. Thanks everyone!

Note for committers: Please also credit pcorbett, who worked on the original issue that was split up into this patch and others. Some of that work got into this patch.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -509,26 +522,40 @@ protected function filterComment($comment = '') {
   +   *   If no exception occurs during query execution, this method will return
   +   *   one of:
   +   *   - The executed statement, if
   +   *     $options['return'] === self::RETURN_STATEMENT, or if
   +   *     $options['return'] is not set (due to self::defaultOptions()).
   +   *   - The number of rows affected by the query (not the number matched), if
   +   *     $options['return'] === self::RETURN_AFFECTED.
   +   *   - The generated insert ID of the last query, if
   +   *     $options['return'] === self::RETURN_INSERT_ID.
   +   *   - NULL, if
   +   *     $options['return'] === self::RETURN_NULL.
   +   *   If an exception occurs during query execution, this method will:
   +   *   - Throw an exception, if
   +   *     $options['throw_exception'] evaluates to TRUE, or
   +   *     $options['throw_exception'] is not set (due to self::defaultOptions()).
   +   *   - Return NULL, if $options['throw_exception'] evaluates to FALSE.
   

   The grammar of this block is extremely confusing. The information is complete but the structure of the lists here has grammar that doesn't parse at all. It should be reformatted, as right now it's somewhere between a bulleted list and a broken-out sentence. It took me 3 read-throughs to understand what it was saying, which is a bad sign. :-)

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -700,11 +734,11 @@ public function getDriverClass($class) {
   -   * @param $alias
   +   * @param string|null $alias
       *   The alias of the base table of this query.
       * @param $options
       *   An array of options on the query.
   

   Here, we're documenting $alias as an optional parameter with |null. We're documenting $options as optional with "(optional)" in the description instead. We should be consistent and pick one.

   I would prefer (optional), as parameters should only ever be of one type (to the extent possible).

   (I didn't check if that inconsistency appears elsewhere.)

Agreed on item 2 in comment #38 -- you're right, |null is not so good, should just say (optional).

And agreed on item 1. My suggestion would be to reformat it as:

The function returns one of the following:

And then for each one, start with the "if".

I don't think we need to say anything about exceptions. If an exception is thrown, the function won't return, that's just PHP. Presumably we have a @throws for that?

Thanks for the input @Crell, @jhodgdon, I'll take a crack at this.

Made the changes mentioned in #39.

Oops! changed permissions in last upload.

Looking pretty good!

Still a few things to fix:

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -252,7 +262,7 @@ public function getConnectionOptions() {
   -   * @param $prefix
   +   * @param array|string $prefix
       *   The prefixes, in any of the multiple forms documented in
       *   default.settings.php.
       */
   

   This is kind of weird documentation. The parameter is called $prefix, and it can either be a single prefix, or an array of prefixes. But the docs don't really reflect that.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -355,9 +368,9 @@ public function prepareQuery($query) {
   +   *   (optional) The target this connection is for. Set to NULL (default) to
   +   *   disable logging entirely.
       */
   

   Maybe this would be better as:

   Omit to disable logging entirely.

   And weren't we not going to have string|null in @param lines, as per previous comments?

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -509,26 +522,36 @@ protected function filterComment($comment = '') {
   +   *   - If either $options['return'] === self::RETURN_STATEMENT, or
   +   *     $options['return'] is not set (due to self::defaultOptions())
   +   *     returns the executed statement.
   +   *   - If $options['return'] === self::RETURN_AFFECTED
   +   *     returns the number of rows affected by the query
   +   *     (not the number matched).
   +   *   - If $options['return'] === self::RETURN_INSERT_ID
   +   *     returns the generated insert ID of the last query.
   +   *   - If either $options['return'] === self::RETURN_NULL, or
   +   *     an exception occurs and $options['throw_exception'] evaluates to FALSE
   +   *     returns NULL
       *
   

   These need some commas to separate the If from the "returns", I think.

   Last line needs to end in . also.

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -628,6 +651,13 @@ protected function handleQueryException(\PDOException $e, $query, array $args =
   +   *   This exception is thrown when:
   +   *   - Placeholders with a trailing [] are not expanded with an array of
   +   *     values.
   +   *   - The placeholder is to be expanded with an array of values and it is
   +   *     missing a trailing [].
       */
   

   I don't understand the first one... oh I think I know what it means.

   How about changing this to:

   - A placeholder that ends in [] is supplied, and the supplied value is not an array.
   - A placeholder that does not end in [] is supplied, and the supplied value is an array.

5. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1026,7 +1085,7 @@ public function pushTransaction($name) {
       *   The name of the savepoint
   

   Not patched: this should end in .

6. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1234,7 +1299,7 @@ public function commit() {
   +   *   (optional) After a database import, it might be that the sequences table is behind,
   

   This line got too long, so it needs a rewrap.

Made the changes from #44.

This looks good!

One very minor thing to fix, and one question remaining:

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -355,9 +368,9 @@ public function prepareQuery($query) {
   +   * @param string $target
   +   *   (optional) The target this connection is for. Omit to disable logging
   +   *   entirely.
       */
      public function setTarget($target = NULL) {
        if (!isset($this->target)) {
   

   I'm really wondering about this setTarget()... if you omit $target, how does this disable logging entirely? Does this documentation belon gperhaps in setLogger() instead of setTarget()?

   I do not know the answer to this question but it seems suspicious and I don't see anything in the class that ever logs anything so I have no idea about it.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1234,9 +1299,9 @@ public function commit() {
   +   *   is behind, so by passing in the maximum existing id, it can be assured
   +   *   that we never issue the same id.
       *
   

   id -> ID
   (look up id and ID in a dictionary -- "id" is a psychological term, while ID is short for identifier)

I'm really wondering about this setTarget()... if you omit $target, how does this disable logging entirely? Does this documentation belon gperhaps in setLogger() instead of setTarget()?

I find this confusing myself, and don't understand it completely.
(EDIT: At the end of writing this, I think I do :) )
But I found that the $target is used to identify a database connection in \Drupal\Core\Database\Database::$connections, like this:

// See Database::openConnection().
Database::$connections[$key][$target] = $driver_class::open(Database::$databaseInfo[$key][$target]);

The Database::$databaseInfo is the array of database connection info from settings.php.

The call to Connection::setTarget() in Database::openConnection() is not recognized by the IDE, because of the lack of type docs.
A string search suggests that this is really the only call to this method.
So we can safely assume that it is only meant for initialization.
It is not part of the constructor, because:
- This allows to create connection objects which don't have a target, and are not registered in Database::$connections.
- It reduces the constructor signature, making it easier for different implementations to have their own constructor.

I hope this helps :)

Ummmm... so how is the target related to logging? And is the target required?

In Core\Database\Log::log():

  public function log(StatementInterface $statement, $args, $time) {
    foreach (array_keys($this->queryLog) as $key) {
      $this->queryLog[$key][] = array(
        'query' => $statement->getQueryString(),
        'args' => $args,
        'target' => $statement->dbh->getTarget(),
        'caller' => $this->findCaller(),
        'time' => $time,
      );
    }
  }

So apparently it does write the target to the log entry.
However, I don't see how not specifying a target would disable logging. Database::openConnection() does not suggest that it would have any effect.

Btw, this would be much easier to figure out if we had all the type docblocks in place. Then the IDE could tell us where something is called.

Imo the thing to do here is to write a comment with what we know, and add a @todo that this needs to be verified.

I think we should just take out the "Omit to disable logging" piece in setTarget(). I have a feeling it was left over from copying a doc block from setLogger() or something like that.

See also #46. I apparently forgot to set this to Needs Work.

I concur. The logging just records what target the query was run against. Disabling logging is something else entirely; setTarget() has nothing to do with it.

Addressing #46. Thanks for the reviews.

Interdiff looks good, and addresses the two problems identified in #46... If we're fixing this line in addition though:

-   * Retrieves an unique id from a given sequence.
+   * Retrieves an unique ID from a given sequence.

probably we should also fix an -> a? However this is a preexisting problem and this has gone on quite a while so I'd be inclined to just let it go at this point and mark RTBC. The patch is good.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -24,7 +24,7 @@
-   * @var string
+   * @var string|null
    */
   protected $target = NULL;

@@ -35,14 +35,14 @@
    * connection can be a single server or a cluster of primary and replicas
...
-   * @var string
+   * @var string|null
    */
   protected $key = NULL;
...
-   * @var Log
+   * @var \Drupal\Core\Database\Log|null
    */
   protected $logger = NULL;

@@ -111,7 +111,9 @@
-   * @var object
+   * Set to NULL when the schema is destroyed.
+   *
+   * @var \Drupal\Core\Database\Schema|null
    */
   protected $schema = NULL;

The setting to NULL and documenting NULL seems completely unnecessary to me. We'd have this everywhere since all class properties start off as null (or not set). Setting to needs review to get thoughts from @jhodgdon since this was probably done as a result of her feedback.

Those null defaults are already in HEAD now. I suspect that goes back to me writing code in 2008. :-) Many of those are lazily filled in, such as $schema, when first requested or only at certain times, like $logger. note that, eg, $connection (the underlying PDO object) is not set to null by default and is not documented as being null. The ones that are so documented really could be NULL post-constructor, for better or worse.

Hm. In #3(b), I suggested string|null for a particular @param where NULL had a particular meaning, according to the docs. I do not think I ever advocated for putting string|null in @var. But I also marked the patch RTBC with string|null in there. I don't feel strongly either way. Removing the setting to NULL is out of scope for "docblock fixes" however.

The setting to NULL and documenting NULL seems completely unnecessary to me. We'd have this everywhere since all class properties start off as null (or not set). Setting to needs review to get thoughts from @jhodgdon since this was probably done as a result of her feedback.

I don't know how this is usually done in core, but I personally always do @var (type)|null if the property is not initialized in the constructor, but only by a setter method. We can rely on the constructor (if inheritance is done well), but we cannot rely on setters being called.

Docs are not frozen in beta. Committed 554f9a4 and pushed to 8.0.x. Thanks!