Docblock fixes in database system, part I

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -133,6 +137,9 @@ protected function createTableSql($name, $table) {
   +   * @return string
   +   *   Generated SQL string.
   

   "The generated SQL string" or "A generated SQL string". An article is needed at the start of the sentence.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -32,6 +32,9 @@ class Connection extends DatabaseConnection {
   +   * @param \PDO $connection
   +   * @param array $connection_options
   

   Shouldn't these have descriptions like everything else?

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -183,6 +186,9 @@ public function queryTemporary($query, array $args = array(), array $options = a
   +  /**
   +   * @return string
   +   */
      public function driver() {
   

   Shouldn't this be {@inheritdoc}?

4. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -237,6 +243,10 @@ public function mapConditionOperator($operator) {
   +   * @param int $existing
   +   *
   +   * @return int
   

   Needs short descriptions.

5. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -305,6 +311,15 @@ function getFieldTypeMap() {
   +   * @return string
   +   *   Generated SQL snippet.
   

   Article needed, as above.

6. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -631,6 +649,17 @@ public function changeField($table, $field, $field_new, $spec, $new_keys = array
   +   * @param string $table
   +   * @param string $name
   +   * @param array $fields
   

   Descriptions needed.

   A number of others are still needed, but I'm going to stop mentioning it to save myself time. :-)

7. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -175,6 +181,12 @@ public static function sqlFunctionIf($condition, $expr1, $expr2 = NULL) {
   +   * @param ...
   +   *   Variadic parameters.
   

   Oh for PHP 5.6... :-)

8. +++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
   @@ -126,11 +126,11 @@ public function &getUnion();
   +   * @param PlaceholderInterface $queryPlaceholder
   

   Should be namespaced.

   (Although IDEs are increasingly recognizing classes in docblocks relative to the namespace of the file, which is great. We may want to revisit that standard at some point...)

9. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -689,10 +701,12 @@ public function createTable($name, $table) {
   +   * @param array $fields
   +   *   An array of key/index column specifiers. Each array value can be either
   +   *   - a string field name.
   +   *   - an of two strings: array($field_name, $field_alias).
   

   "an of two strings". A what? An array of two strings?

10. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
    @@ -278,8 +292,8 @@ public function setFetchMode($fetchStyle, $a2 = NULL, $a3 = NULL) {
    +   * @return array|mixed|object
    +   *   The current row formatted as requested.
        */
       public function current() {
    

    "mixed" already implies array and object. This method is silly. :-)

Looking at this issue during the Forum One code sprint.

On 2, Would it be more appropriate to provide detailed descriptions in Drupal\Core\Database Connection rather than providing the same descriptions in all the driver specific classes?

The parent class should have the descriptions. (Not long ones, just the few words needed.) The child classes can probably then use {@inheritdoc} and be done with it.

Attached are is the updated patch with Crell's suggestions, plus a few other spelling/grammar corrections I came across. Donquixote is right, there are a large number of methods in the driver specific classes that require {@inheritdoc} annotations.

See #2192185: Improve DB API code documentation as well.

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -239,14 +236,19 @@ public function mapConditionOperator($operator) {
-   * @param int $existing
+   * @param $existing
+   *   After a database import, it might be that the sequences table is behind,
+   *   so by passing in the maximum existing id, it can be assured that we
+   *   never issue the same id.
    *
-   * @return int
+   * @return
+   *   An integer number larger than any number returned by earlier calls and
+   *   also larger than the $existing if one was passed in.

Why remove the type specification here?

Otherwise I think this is "close enough" to commit. I just had to postpone #2192185: Improve DB API code documentation again on this issue; there's too many DB-doc-cleanup issues floating around. Let's get at least some of them committed.

Adding @param tags that lack descriptions is not much of an improvement over not having them at all. Anyone can look at the function signature and see what the parameters are, anyway, so all you're getting is the (possibly accurate) data type of the parameter being added. Many function signatures have that in type hinting already.

jhodgdon: If we addressed #7 would you be OK committing this? This is like the 4th "futz with database docblocks" issue and I want to get at least one of them committed, just to make forward progress at all. :-(

I haven't looked at the patch. What exactly are you asking me to overlook?

I'd be happy to commit a patch that fixes some files, and conforms to our standards (like having descriptions for @return/@param, except in the case of @return $this, which by our standards doesn't require a description). I'm less happy to commit patches that don't bring the docs into closer confirmation to our standards.

The other problem with these huge patches is that as a committer, I am obligated to give them a final review before I commit them, or at least I feel that obligation. If the patch is 1000 lines, I need to at least glance through all 1000 lines, and there is a near 100% certainty that I'll find something that isn't right. I really don't want to commit patches that introduce errors, like incorrect types or misleading documentation (I'd rather have no documentation than wrong documentation -- at least if there is no docs it says "sorry, no docs, guess you'll have to read the code!)

Given that, I'd much rather have 10 smaller patches of 100 lines than one of 1000 lines -- at least if 9 of the 10 were perfect I could go ahead and commit them, rather than stalling the entire 1000 lines of patching. These huge patches are just really really hard to get right and really difficult to deal with.

Re-rolled with as many type specifications as I could find missing with @Crell 's help. First big re-roll for me! (fingers crossed)

N00b alert :) Attaching actual patch as well.

This patch could do more; there's definitely some docblocks in it that could use work.

However, it's nearly 100KB and from my read through just now I don't see any chunk that makes anything worse; they all make the situation better, just maybe not as better as it could get. That's for follow-up patches if we ever hope of getting anything in. :-)

Thus, RTBC. jhodgdon, over to you.

To keep this in scope I've only reviewed the proposed changes.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -506,14 +514,16 @@ protected function filterComment($comment = '') {
   +   * @return \Drupal\Core\Database\StatementInterface|int
   +   *   Depending on the value of $options['return'], this method will return one
   +   *   of:
   +   *   - the executed statement,
   +   *   - the number of rows affected by the query (not the number matched), or
   +   *   - the generated insert ID of the last query.
   +   *   Typically, this option will be set by default or by a query builder, and
   +   *   should not be set by a user. If there is an error, this method will
   +   *   return NULL and may throw an exception if $options['throw_exception'] is
   +   *   TRUE.
   

   Should be @return \Drupal\Core\Database\StatementInterface|int|null

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -34,7 +34,10 @@ class Schema extends DatabaseSchema {
   +   * @param string $table
   +   * @param bool $add_prefix
   

   Lets add docs here

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -631,6 +646,19 @@ public function changeField($table, $field, $field_new, $spec, $new_keys = array
   +  /**
   +   * @param string $table
   

   No line saying what the method does

4. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -655,6 +683,10 @@ protected function _createKeys($table, $new_keys) {
      /**
       * Retrieve a table or column comment.
   +   * @param string $table
   +   *   The database table whose comments to return.
   +   * @param string $column
   +   *   (optional) A column in $table whose comments to return.
       */
      public function getComment($table, $column = NULL) {
   

   Needs a new line and the method name seems to suggest a missing @return

5. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -201,6 +222,15 @@ public static function sqlFunctionConcat() {
   +   * @return string
       */
   
   @@ -208,6 +238,15 @@ public static function sqlFunctionSubstring($string, $from, $length) {
   +   * @return string
       */
   

   Should have a line saying what is returned.

6. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -244,11 +283,23 @@ public static function sqlFunctionRegexp($string, $pattern) {
   +   * @throws \PDOException
   

   @throws should have a line saying why this occurs.

7. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
   @@ -54,6 +55,12 @@ public function createTableSql($name, $table) {
   +   * @param string $tablename
   +   * @param array $schema
   
   @@ -73,6 +80,12 @@ protected function createIndexSql($tablename, $schema) {
   +   * @param string $tablename
   +   * @param array $schema
   

   param doc missing

8. +++ b/core/lib/Drupal/Core/Database/Query/Condition.php
   @@ -60,6 +63,8 @@ public function __construct($conjunction) {
   +   * @return int
       */
   

   Missing description of return value

9. +++ b/core/lib/Drupal/Core/Database/Query/Insert.php
   @@ -186,7 +186,8 @@ public function from(SelectInterface $query) {
   +   * @throws \Exception
   +   * @return \Drupal\Core\Database\StatementInterface|int|NULL
   

   Missing new line between @throws and @return and also need a line describing why exception is thrown

10. +++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php
    @@ -47,6 +47,10 @@ class PagerSelectExtender extends SelectExtender {
    +  /**
    +   * @param \Drupal\Core\Database\Query\SelectInterface $query
    +   * @param \Drupal\Core\Database\Connection $connection
    +   */
    

    Missing method description one liner and param descriptions.

11. +++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php
    @@ -60,6 +64,8 @@ public function __construct(SelectInterface $query, Connection $connection) {
    +   * @return \Drupal\Core\Database\StatementInterface|null
    

    Missing @return description

12. +++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php
    @@ -160,7 +168,9 @@ public function limit($limit = 10) {
    +   * @param int $element
    

    Missing param description

13. +++ b/core/lib/Drupal/Core/Database/Query/PlaceholderInterface.php
    @@ -14,13 +14,15 @@
    +   * @return string
        */
    

    Missing @return description

14. +++ b/core/lib/Drupal/Core/Database/Query/SelectExtender.php
    @@ -38,6 +38,10 @@ class SelectExtender implements SelectInterface {
    +  /**
    +   * @param \Drupal\Core\Database\Query\SelectInterface $query
    +   * @param \Drupal\Core\Database\Connection $connection
    +   */
    

    Missing method description and parameter descriptions.

15. +++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
    @@ -128,11 +128,11 @@ public function &getUnion();
    +   * @param PlaceholderInterface $queryPlaceholder
    

    Should be a fully namespaced reference to PlaceHolderInterface

16. +++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
    @@ -325,18 +333,18 @@ public function rightJoin($table, $alias = NULL, $condition = NULL, $arguments =
    +   * @param string|SelectInterface $table
    

    Should be a fully namespace reference to SelectInterface

17. +++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
    @@ -475,7 +484,10 @@ public function isPrepared();
    +   * @param SelectInterface $query
    

    Should be a fully namespaced reference to SelectInterface

18. +++ b/core/lib/Drupal/Core/Database/Query/TableSortExtender.php
    @@ -19,6 +19,10 @@ class TableSortExtender extends SelectExtender {
    +  /**
    +   * @param \Drupal\Core\Database\Query\SelectInterface $query
    +   * @param \Drupal\Core\Database\Connection $connection
    +   */
    

    Needs method one liner and param description

19. +++ b/core/lib/Drupal/Core/Database/Schema.php
    @@ -185,9 +190,14 @@
    +  /**
    +   * @param \Drupal\Core\Database\Driver\Sqlite\Connection $connection
    +   */
    

    Needs method one liner and param description

20. +++ b/core/lib/Drupal/Core/Database/Schema.php
    @@ -202,6 +212,8 @@ public function __clone() {
    +   * @return string
        */
    

    Missing @return description

21. +++ b/core/lib/Drupal/Core/Database/Statement.php
    @@ -36,6 +36,9 @@ class Statement extends \PDOStatement implements StatementInterface {
    +  /**
    +   * @param \Drupal\Core\Database\Connection $dbh
    +   */
    

    Needs method one liner and param description

22. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
    @@ -135,6 +135,12 @@ class StatementPrefetch implements \Iterator, StatementInterface {
    +  /**
    +   * @param \PDO $dbh
    +   * @param \Drupal\Core\Database\Connection $connection
    +   * @param string $query
    +   * @param array $driver_options
    +   */
    

    Needs method one liner and param description

Alex: As I said, this patch could be better but doesn't make anything *worse*. It's also 100 KB, and is at least the third attempt to update documentation in the DB layer that we've had; they keep stalling on "who wants to deal with big patches that somehow seem to break often". As DB maintainer I don't care if the patch is perfect. I care that it's forward progress. Caring about perfect is why none of these have been committed in the last year.

Unless anything in this patch is actually *wrong*, please let it through. There will be follow-ups, many of them, but "the best is the enemy of the good" at this point.

Crell: I agree with Alex. I don't think that changes like his example #21 and #22 are improvements. I don't agree with the philosophy of adding a bunch of doc blocks to the code base that are way out of compliance with our documentation standards. Having no doc block, in my opinion, is preferable to one that just gives the data type of parameters. Doc blocks get copied around... these ones are not good.

Please just try for a smaller patch of actually good documentation, rather than a huge patch that doesn't improve much.

Yeah, I definitely do not want to commit docs that give incorrect information -- would rather have no docs than incorrect docs -- at least if no docs, you know you have to read the code.

I kept thinking of this issue while painting my deck today... It seems that a few items are what's holding up this patch, but also no one seems particularly thrilled with the current state of it either. I'd like to second jhodgdon's suggestion in #13 of breaking this up into several, smaller issues. Some further rational:

• No one has mental stamina to give 2700+ line diff full consideration. So smaller bite-sized issues should help generate better quality descriptions in each item.
• Smaller issues could be tagged as novice, and be more approachable for sprinters at the upcoming DrupalCon Amsterdam.

By my count, this patch currently touches 28 different files (listed below), so why not a new issue for each? I volunteer to create these issues, post starter patches from the work above in each one and update this issue to track those.

1. core/lib/Drupal/Core/Database/Connection.php
2. core/lib/Drupal/Core/Database/Database.php
3. core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
4. core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
5. core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
6. core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
7. core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
8. core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
9. core/lib/Drupal/Core/Database/Install/Tasks.php
10. core/lib/Drupal/Core/Database/Log.php
11. core/lib/Drupal/Core/Database/Query/AlterableInterface.php
12. core/lib/Drupal/Core/Database/Query/Condition.php
13. core/lib/Drupal/Core/Database/Query/ConditionInterface.php
14. core/lib/Drupal/Core/Database/Query/Delete.php
15. core/lib/Drupal/Core/Database/Query/Insert.php
16. core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php
17. core/lib/Drupal/Core/Database/Query/PlaceholderInterface.php
18. core/lib/Drupal/Core/Database/Query/Query.php
19. core/lib/Drupal/Core/Database/Query/Select.php
20. core/lib/Drupal/Core/Database/Query/SelectExtender.php
21. core/lib/Drupal/Core/Database/Query/SelectInterface.php
22. core/lib/Drupal/Core/Database/Query/TableSortExtender.php
23. core/lib/Drupal/Core/Database/Query/Truncate.php
24. core/lib/Drupal/Core/Database/Query/Update.php
25. core/lib/Drupal/Core/Database/Schema.php
26. core/lib/Drupal/Core/Database/Statement.php
27. core/lib/Drupal/Core/Database/StatementInterface.php
28. core/lib/Drupal/Core/Database/StatementPrefetch.php

I am at the point of giving up on any chunk larger than an individual method as far as cleaning up the DB documentation goes. We've been trying for a long time now and it never takes and always runs into roadblocks. If you want to try splitting it up that fine-grained to see if we can novice-ify them you have my blessing but this is not a priority for me at this point.

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

Thanks!

28 child issues is also a lot... Maybe there is a happy medium between "one issue per file" and "one big issue" that would make the patches more manageable but not create tons of issues either? The one you did is fine, but maybe patches of about that size or 2-3 times as big would still be OK. So if you find files that don't have tons of changes, you could do a few together in one issue?

That would be a good solution, yes (#32)... some subfolders could also be broken up like maybe "Query subfolder, files A-M" if the patches get too large (I just made that example up).

Other files directly in the core/lib/Drupal/Core/Database/ subdir covered in #2343099: Add @param and @return or fix types in @param and @return in core/lib/Drupal/Core/Database/ (Connection.php on its own was 100 lines, so let's just keep that in its own for now).

#2343121: Docblock fixes for core/lib/Drupal/Core/Database/Driver/

#2343127: Docblock fixes for core/lib/Drupal/Core/Database/Query. That's the bulk of them. Need to find a place to put core/lib/Drupal/Core/Database/Install/Tasks.php.

I'm closing this issue as Won't Fix. The problem is that the issue is too vast to fix in this type of patch. It is much better to have a targeted issue that just goes about fixing one type of problem at a time, throughout core, than an issue that ends up trying to fix everything. See https://www.drupal.org/core/scope for more information. Closing child issues too.