Docblock fixes for core/lib/Drupal/Core/Database/Driver/

@param still needs a description for each parameter.

I am going to try solve this issue. Currently working at drupalcon amsterdam

Ben25 had some issues with his dev environment so he is no longer working on the issue.

Working on updating patch to include more data on @params.

Fixed a long docblock comment.

This is getting better! There are still quite a few things to fix though:

a) MySQL Connection object

+   *
+   * @throws \Drupal\Core\Database\DatabaseExceptionWrapper
+   * @throws \Exception
+   * @throws \Drupal\Core\Database\TransactionCommitFailedException

The third exception here is probably OK to put in with just @throws without an explanation, because TransactionCommitFailedException is pretty clear. But for the first two, I have no clear idea from this documentation why/when a DatabaseExceptionWrapper would be thrown, or a plain Exception. So these need to be documented better. See https://www.drupal.org/coding-standards/docs#throws

b)

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -33,9 +33,18 @@ class Schema extends DatabaseSchema {
   const COMMENT_MAX_COLUMN = 255;
 
   /**
-   * Get information about the table and database name from the prefix.
+   * Get information about the table name and database from the prefix.

First lines of function docs should start with verbs like "Gets" not "Get". See https://www.drupal.org/coding-standards/docs#functions - this applies to several other lines in this patch, like in the PGSQL connection object:

   /**
-   * Retrieve a the next id in a sequence.
+   * Retrieve the next id in a sequence.

c) Same class

+   * @param
+   *   Name of table to look prefix up for. Defaults to 'default' because that's
+   *   default key for prefix.

Missing the $variable name on the @param line, and optionally the type of variable (should be string here, right?). See https://www.drupal.org/coding-standards/docs#param

Oh I see, it's below:

+   * @param
+   *   Name of table to look prefix up for. Defaults to 'default' because that's
+   *   default key for prefix.
+   * @param $add_prefix
+   *   Boolean that indicates whether the given table name should be prefixed.
+   *
+   * @param string $table
+   * @param bool $add_prefix

So move those two last @param lines up to replace the upper two @param lines.

d) same class

+   *
+   * @return string
+   *   The Generated SQL string.
    */
   protected function createFieldSql($name, $spec) {

Generated should not be capitalized.

e) PGSQL connection class:

   /**
-   * Retrieve a the next id in a sequence.
+   * Retrieve the next id in a sequence.

"id" should be "ID". "id" is a psychological term; "ID" means identifier. Please do that throughout the patch.

f) PGSQL connection class:

+   * @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.

This is a nice explanation... but for a @param we want to start with a description of what the parameter value actually is. So it kind of needs rewording, like starting with "The maximum existing ID" or something like that.

g) same function

+   * @return
+   *   An integer number larger than any number returned by earlier calls and
+   *   also larger than the $existing if one was passed in.

I would drop "the" before "$existing".

h) pgsql Schema class:

+   *
+   * @return \stdClass

Should be @return object -- see https://www.drupal.org/coding-standards/docs#types

i) same class

+  /**
+   * @param array $fields
+   *   Array of field definitions. Each array value is either
+   *   - a field name (string), or
+   *   - an array of two strings, array($field_name, $alias).
+   *
+   * @return string
+   *   The Generated SQL snippet.
+   */
   protected function _createKeySql($fields) {

Function is missing one-line description. There are a couple of other places like this in the patch.

Also the list in the @param needs some formatting attention: https://www.drupal.org/coding-standards/docs#lists (capitalization, no "or" at end of line, lines end with ., and preceding line ends in :). This also applies elsewhere in the patch.

j)

-  /**
-   * {@inheritdoc}
-   */
   public function copyTable($source, $destination) {
 

Why did you take out the inheritdoc?

k)

   /**
    * Retrieve a table or column comment.
+   * @param $table
+   *   The database table whose comments to return.
+   * @param $column
+   *   (optional) A column in $table whose comments to return.
    */
   public function getComment($table, $column = NULL) {

Needs blank line between one-line description and @param start.

Also "whose comments to return" isn't very good grammar here. How about "The database table to return comments for." instead? (and similar for the $column)?

l) SQLite Connection object:

+   * @param mixed $expr1
+   *   Returned if $condition is results to TRUE.
+   * @param mixed $expr2
+   *   (optional) Returned if $condition results to FALSE.
+   *
+   * @return null
    */
   public static function sqlFunctionIf($condition, $expr1, $expr2 = NULL) {

"results to true" should be just "is TRUE" or "evaluates to TRUE". Also nothing is "returned" here, so ... maybe "Value if $condition is TRUE"?

Also do not do @return null. Just omit @return if there is no return value.

j) Same class

+   *
+   * @param ...
+   *   Variadic parameters.
+   *
+   * @return mixed|NULL
+   *   The maximum of the given arguments.
    */
   public static function sqlFunctionGreatest() {

Variadic is not a word. Maybe "Variable" is what you meant? This occurs elsewhere in the patch too.

k) same code - in @param/@return types use "null" not "NULL".

l) Same class

+   * @return string
    */
   public static function sqlFunctionSubstring($string, $from, $length) {

@return needs a description. Same for sqlFunctionSubstringIndex just after this.

m)

   /**
-   * SQLite-specific implementation of DatabaseConnection::prepare().
+   * SQLite-specific implementation of
+   * \Drupal\Core\Database\Connection::prepare().

First line should not have a break in it. Better to be one line slightly too long than two lines. Or maybe just say "SQLite implementation" and take out "specific" to make the line shorter?

please review the patch attached

Thank you for the patch, @gaurav101!

One thing that is helpful for reviewers is to create an interdiff from the previous patch. I have created and attached one with the following command:

interdiff 2343121-7.patch 2343121-10-docblock-fixes.patch > interdiff.txt

Good work so far. The patch still needs to incorporate changes from comment #19.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1184,13 +1184,14 @@ public function commit() {
+       Maximum existing id

A couple of issues here:

1. I think that section e) in comment #19 of @jhodgdon's review applies here as well. id should be capitalized.
2. Also, I think that this should be a part of the complete sentence.

Thanks for the review Jennifer. I have gone through all the changes you suggested and here is what I did.
a) I have provided explanations for the first two lines
b) I changed the first line of function docs to start with verbs
c) I moved the @param lines to replace the other two lines
d) I have uncapitalised the word 'generated'
e) I have changed id to ID
f) I reworded the explanation of the parameter value
g) I dropped the word 'the'
h) I changed the stdClass to a return object
i) I gave function a one line definition and formatted the list
j) I put back the inheritdoc
k) I edited the sentence to be gramatically correct and added a blank line
L) I changed the NULL to null and omitted @return
j) I changed variadic to variable
k) I changed NULL to null
l) I gave a description for return and sqlFunctionSubstringIndex
j) I made the line shorter by removing 'specific'

Patch and interdiff attached. This is my second patch contribution to drupal :)

Thanks!

There are still some problems in this patch -- some may not have been seen in previous reviews, sorry!

a) "an unique" should be "a unique".

b) /core/lib/Drupal/Core/Database/Connection.php

    * @param $existing_id
+       Maximum existing id
    *   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.

There is no * at the beginning of this line. Also "id" should be "ID" in text ("id" is a psychological term, while "ID" means identifier).. Also "Maximum existing ID" is not a sentence and it needs to be wrapped with the following lines to make a paragraph.

c)

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -33,9 +33,18 @@ class Schema extends DatabaseSchema {
   const COMMENT_MAX_COLUMN = 255;
 
   /**
-   * Get information about the table and database name from the prefix.
+   * Get information about the table name and database from the prefix.

All function descriptions should start with 3rd-person verbs like "Gets" , not "Get". This is all over the patch.

d) A few lines down

+   * @param
+   *   Name of table to look prefix up for. Defaults to 'default' because that's
+   *   default key for prefix.
+   * @param $add_prefix
+   *   Boolean that indicates whether the given table name should be prefixed.
+   *
+   * @param string $table
+   * @param bool $add_prefix
+   *
+   * @return array
    *   A keyed array with information about the database, table name and prefix.
    */
   protected function getPrefixInfo($table = 'default', $add_prefix = TRUE) {

Needs the $variable on the first @param line, and a data type. The other params should be ... um... this is just wrong, please fix it. Should just be two @param lines, one for each actual function parameter.

e) Same class

+   *
+   * @return string
+   *   The Generated SQL string.
    */
   protected function createFieldSql($name, $spec) {

"Generated" should be lower-case.

f)

+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
@@ -22,7 +22,7 @@
 class Connection extends DatabaseConnection {
 
   /**
-   * The name by which to obtain a lock for retrieve the next insert id.
+   * The name by which to obtain a lock for retrieve the next insert ID.
    */
   const POSTGRESQL_NEXTID_LOCK = 1000;

"for retrieve" ?!? not good English.

g) in Schema.php

+   *   Array of field definitions. Each array value is either:
+   *   - a field name (string),
+   *   - an array of two strings, array($field_name, $alias).
+   *
+   * @return string
+   *   The generated SQL snippet.
+   */
   protected function _createKeySql($fields) {

Please look at https://www.drupal.org/node/1354#lists for list syntax. Capitalization. Ending with . not ,

Also in the 2nd list item, I don't like using $field_name and $alias -- these are not real variables. Use words. Like "An array containing the field name and field alias."

h) same class

-  /**
-   * {@inheritdoc}
-   */
   public function copyTable($source, $destination) {

Why take out the inheritdoc block? The function needs *something* for docs. If it cannot inherit it needs added docs.

i) same class

+  /**
+   * @param string $table
+   *   The table on which the index will be created.
+   * @param string $name
+   *   An identifying name for this index.
+   * @param array $fields
+   *   Array of field definitions. Each array value is either
+   *   - a field name (string), or
+   *   - an array of two strings, array($field_name, $alias).
+   *
+   * @return string
+   *   The generated SQL snippet.
+   */
   protected function _createIndexSql($table, $name, $fields) {

Missing one-line description. Also see previous notes on list syntax.

j) same class

  /**
    * Retrieve a table or column comment.
+   *
+   * @param $table
+   *   The database table to return comments for.
+   * @param $column
+   *   (optional) A column in $table to return comments for.
    */
   public function getComment($table, $column = NULL) {

No @return.

k)

 /**
- * Specific SQLite implementation of DatabaseConnection.
- */
+  * SQLite implementation of DatabaseConnection::prepare(). SQLite implementation of
+  * \Drupal\Core\Database\Connection::prepare().
+  */
 class Connection extends DatabaseConnection {

Indentation is wrong and class docs need to start with a one-line summary.

l) same class

   * @return
    */
   public static function sqlFunctionIf($condition, $expr1, $expr2 = NULL) {

This @return is incomplete.

At this point I decided the patch wasn't finished and stopped reviewing. There are similar errors in the rest of the patch.

Hi Jennifer, thank you for the review. What are your thoughts on dividing the patch into three, pgsql, mysql and sqlite. I think this is important because together they amass a lot of code and it could possibly be easier to handle.

I don't have an opinion on that. Up to you, but if you do that please file separate issues.

changes made for #14

Hello!

Thank you for working on this issue!

We should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015 with no #.

When you upload an update to a big patch, PLEASE make an interdiff file. That way we can review the changes you made.
https://www.drupal.org/documentation/git/interdiff

As a start to reviewing, here's an interdiff for patches 13 - 17.

Thanks for the patch in #17 and the interdiff file!

Regarding this patch in general... I'm not sure if the SQLite compatibility function return value docs are correct -- I really doubt that they're returning SQL results. They're doing something to queries.

And ... for the other functions being documented, they also need careful reviews to make sure the content is correct.

So I've mostly reviewed writing style and formatting. There are still some things to fix:

a) Connection.php

    * @param $existing_id
+   *   This must be a Maximum existing ID.
    *   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.

This is not great documentation. Parameter documentation should be something like "The maximum existing ID", not "This must be a maximum existing ID", and "Maximum" should not be capitalized in any case.

b) Also all of the documentation in an @param should be wrapped together into one paragraph with lines as long as possible (but staying under 80 character lines).

c) #14 (c) was not addressed.

d) The @param changes in Schema.php do not have the correct indentation. See https://www.drupal.org/node/1354#param

e) Schema.php

+   * Name of table to look prefix up for. Defaults to 'default' because that's
+   * default key for prefix.

We don't need that sentence about the default. It doesn't give any information beyond the function signature.

f) Connection.php

  }
-
+  /**
+   * {@inheritdoc}
+   */
   public function driver() {
     return 'pgsql';
   }

There should always be a blank line between the end of one function and the start of the next function doc block.

g) Connection.php

+   *   also larger than $existing if one was passed in.
+    */
+
   public function nextId($existing = 0) {

The indentation of */ is wrong, and there should not be a blank line between documentation and function.

h) Schema.php

+  /**
+   *  Generates Index SQL from field definitions.

Too many spaces before Generates. Should be just one.

i) Schema.php

   /**
    * Retrieve a table or column comment.
+   *
+   * @param $table
+   *   The database table to return comments for.
+   * @param $column
+   *   (optional) A column in $table to return comments for.
+   *
+   * @return string
+   *   The generated SQL string.
    */
   public function getComment($table, $column = NULL) {
 

I do not think this @return is correct?

k) Connection.php

/**
- * Specific SQLite implementation of DatabaseConnection.
+ * @addtogroup database
+ * @{
+ * SQLite implementation of DatabaseConnection::prepare(). SQLite implementation of
+ * \Drupal\Core\Database\Connection::prepare().
  */
 class Connection extends DatabaseConnection {

Um. What? This needs to be a class documentation block. If you want to add it to the database group, use @ingroup not @addtogroup, and that should go at the end of the class documentation block.

Also classes need to start with a one-line documentation summary.

l) Same class

     else {
-      return NULL;
+      return null;
     }

This is wrong and also this is supposed to be a documentation patch.

j) same class

+   * @param mixed $expr2
+   *   (optional) Returned if $condition results to FALSE.

"results to" should just be "is", and "Returned" should be "Value", to be consistent with $expr1.

l) Same class

+   * @return string
    */
   public static function sqlFunctionSubstring($string, $from, $length) {

Needs docs on the @return.

m)

   *
    */
   public static function sqlFunctionSubstringIndex($string, $delimiter, $count) {

Extra blank line needs to be removed.

n)

  /**
-   * SQLite-specific implementation of DatabaseConnection::prepare().
+   * SQLite-specific implementation of
+   * \Drupal\Core\Database\Connection::prepare().

This should be a one-line summary.

o) SQLLite schema.php:

+   * @return array
    *   An array representing the schema, from drupal_get_schema().
    * @see drupal_get_schema()
    */

There should be a blank line before the @see.

Hi jhodgdon,

I have covered all point which are mentioned in #21.
--#21 point K) is not clear to me so removed those comments and just kept

@addtogroup database
+ * @{

there. I will need bit of help here.

--#21 point n) one-line summery goes beyond 80 columns.

Attaching patch & interdiff files. I would like to work further on this issue. So assigning this to myself.

The point in #21-K is that the doc block immediately before a class declaration needs to document the class.

A doc block with @addtogroup must be separate from that. Use @ingroup if you just want to add the class to a topic.

See https://www.drupal.org/node/1354#defgroup

Anyway the latest patch doesn't apply, and someone still needs to review the accuracy of the SQLite stuff in it.

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...

Adding the beta evaluation.

Removing sprint weekend tag!!
As suggested by @YesCT

@deepakaryan1988 The tag was added January 17, 2015 during the actual global sprint weekend. See https://groups.drupal.org/node/447258
Please leave the tag on issues that were actually worked on during that event.

I think you might have confused a message from May about the sprint weekend tag, that it should not be added to events from May. The tag is for issues worked on during https://groups.drupal.org/node/447258 by people attending sprint weekend sprints.

Last patch does not apply.

Rerolled patch

I think #31 was meant to be for review?

Thanks for the reroll!

I took a look through the latest patch. It looks pretty good, but not everything in it is an improvement. Some things to fix:

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1234,13 +1234,14 @@ public function commit() {
   +   *   The maximum existing ID.
       *   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
   

   This new line of documentation needs to be wrapped in with the rest of this paragraph. Also:
   id => ID
   (two places)

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -283,11 +286,21 @@ public function mapConditionOperator($operator) {
   -   * Retrieve a the next id in a sequence.
   +   * Retrieve the next ID in a sequence.
       *
   

   If we're going to fix this line, why not fix it completely? The first word should be "Retrieves".

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -283,11 +286,21 @@ public function mapConditionOperator($operator) {
   +   * @return
   +   *   An integer number larger than any number returned by earlier calls and
   +   *   also larger than $existing if one was passed in.
   +    */
   

   Could say
   @return int
   here it looks like? Our docs standards say that all docs should have @param/@return types, and this is new documentation, so it should have a type.

4. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -762,6 +784,21 @@ public function changeField($table, $field, $field_new, $spec, $new_keys = array
   +   * Generates Index SQL from field definitions.
   

   I think I would either say "index" or "INDEX" but not "Index" here?

5. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -762,6 +784,21 @@ public function changeField($table, $field, $field_new, $spec, $new_keys = array
   +   *   Array of field definitions. Each array value is either
   +   *   - A field name (string).
   +   *   - An array containing the field name and field alias.
   

   End the line before a list in :

6. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -12,7 +12,8 @@
   - * Specific SQLite implementation of DatabaseConnection.
   + * @addtogroup database
   + * @{
     */
    class Connection extends DatabaseConnection {
    
   

   This needs to be:

   /**
    * Specific SQLlite [rest of that line that was removed]
    *
    * @ingroup database
    */
   

7. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -90,7 +91,6 @@ public function __construct(\PDO $connection, array $connection_options) {
      }
   -
      /**
       * {@inheritdoc}
   

   This blank line should NOT be removed in this patch.

8. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -170,6 +170,16 @@ public function __destruct() {
   +   * @return string
   +   *   The generated SQL string.
       */
      public static function sqlFunctionIf($condition, $expr1, $expr2 = NULL) {
        return $condition ? $expr1 : $expr2;
   

   Um, is this actually returning SQL ? I think this is misleading. Why not just say that it returns $expr1 if $condition is TRUE, and $expr2 if not, rather than this "the generated SQL string", which tells me nothing?

9. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -229,6 +261,16 @@ public static function sqlFunctionSubstring($string, $from, $length) {
   +   * @param string $string
   +   *   The subject for the substring.
   +   * @param string $delimiter
   +   *   A start point for the returned substring occurring after $count.
   +   * @param int $count
   +   *   An offset of $string.
   +   *
   +   * @return string
   +   *  A substring of the provided string.
    

   I have no idea what this means. Can you rewrite these docs so they actually tell me what the function does and what the parameters mean?

10. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
    @@ -289,7 +331,23 @@ public static function sqlFunctionLikeBinary($pattern, $subject) {
    +   * We don't use prepared statements at all at this stage. We just create
    +   * a Statement object, that will create a PDOStatement
    +   * using the semi-private PDOPrepare() method below.
    +   *
    

    Punctuation or grammar here is a bit off in the second sentence... I guess just remove the comma, or replace "that" with "which". But what is a "semi-private" method??? There is no such thing...

11. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    

    Most of the doc blocks in this class should I think just be @inheritdoc for the methods, rather than duplicating a base class or interface? This may apply to the others too. We can cut down on a lot of maintenance and duplication this way...

Taking this up.

Rerolling #31 patch with modification mentioned in #33

Thanks, getting better!

From #33, the following still need attention:

item 1 - this is all one paragraph and it needs to be rewrapped:

+   *   The maximum existing ID.
    *   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.
+   *   so by passing in the maximum existing ID, it can be assured that we
+   *   never issue the same ID.

item 5 - not fixed

item 6 - my suggestion had a typo, oops!

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -12,7 +12,9 @@
 use Drupal\Core\Database\Connection as DatabaseConnection;
 
 /**
- * Specific SQLite implementation of DatabaseConnection.
+ * Specific SQLlite implementation of DatabaseConnection.

SQLite should not be SQLlite. Sorry about that!

#9 - not fixed. I still do not understand the documentation for this function.

#11 - not fixed. If you don't want to go back and change most everything in this patch to @inheritdoc, I can review the rest of the docs, but really it is WAY better to use @inheritdoc because then the docs are just in one place and if something needs to be changed it changes only once.

I think I fixed everything in #36, plus several new {@inheritdoc}s and fixes for a couple of other undocumented methods.

Added a blank line by mistake in #37

The interdiffs look probably OK... can someone else review this patch though? I am short of time. With all the new stuff this patch is getting very big and is also a moving target, because more keeps getting added. With @inheritdoc you really need to verify that the methods actually exist on a parent class/interface... takes significant time to review...

I identified all the inherited methods with PHPStorm and wrote new docblocks for the non-inherited ones. But, I can undo that if it would help move this forward quicker?

Thanks for taking this on! It's much-needed :)

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1262,9 +1262,9 @@ public function commit() {
   +   *   the statement, returns FALSE or emits \PDOException (depending on error
   

   emits must be throws.

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -272,6 +283,15 @@ public function getFieldTypeMap() {
   +   * @param array $spec
   

   @param array is too generic. It needs a more specific type (array[] perhaps) and a description of the array structure or a reference to documentation elsewhere.

3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -350,6 +370,17 @@ protected function shortenIndex(&$index) {
   +   * @param array $fields
   

   Same here. This should be string[]|array[]

4. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -533,6 +606,14 @@ public function prepareComment($comment, $length = NULL) {
   +   * @param $table
   

   Missing parameter type.

5. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -533,6 +606,14 @@ public function prepareComment($comment, $length = NULL) {
   +   * @param $column
   

   Missing parameter type.

6. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php
   @@ -283,10 +301,19 @@ public function mapConditionOperator($operator) {
   +   * @param $existing
   

   Missing parameter type.

7. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -169,11 +170,12 @@ protected function resetTableInformation($table) {
   +   * @return mixed[]
   

   Yeah!

8. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -207,11 +209,11 @@ public function queryFieldInformation($table, $field) {
   +   * @param array $table
   

   array is too generic. It needs a more specific type, and preferably a description of the contents or a reference to documentation elsewhere.

9. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -268,9 +270,12 @@ protected function createTableSql($name, $table) {
       * @param $name
   

   Missing parameter type.

10. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -268,9 +270,12 @@ protected function createTableSql($name, $table) {
        * @param $spec
    

    Missing parameter type.

11. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -400,6 +405,17 @@ function getFieldTypeMap() {
    +   * @param array $fields
    

    @param string[]|array[]

12. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -762,12 +823,35 @@ public function changeField($table, $field, $field_new, $spec, $new_keys = array
    +   * @param array $fields
    

    @param string[]|array[]

13. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -762,12 +823,35 @@ public function changeField($table, $field, $field_new, $spec, $new_keys = array
    +   * @param array $new_keys
    

    Needs more specific type.

14. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -786,6 +870,14 @@ protected function _createKeys($table, $new_keys) {
    +   * @param $table
    

    Missing parameter type.

15. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
    @@ -786,6 +870,14 @@ protected function _createKeys($table, $new_keys) {
    +   * @param $column
    

    Missing parameter type.

16. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
    @@ -177,6 +189,12 @@ public static function sqlFunctionIf($condition, $expr1, $expr2 = NULL) {
    +   * @return mixed|null
    +   *   The maximum of the given arguments.
    

    If NULL is explicitly specified (it's already covered by mixed), I'd expect the description to mention it has specific function?

17. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -39,11 +45,12 @@ public function fieldExists($table, $column) {
    +   * @param array $table
    

    Not specific enough.

18. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -39,11 +45,12 @@ public function fieldExists($table, $column) {
    +   * @return array
    

    Not specific enough.

19. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -54,6 +61,14 @@ public function createTableSql($name, $table) {
    +   * @param array $schema
    

    Not specific enough.

20. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -73,6 +88,14 @@ protected function createIndexSql($tablename, $schema) {
    +   * @param array $schema
    

    Not specific enough.

21. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -355,13 +389,13 @@ public function addField($table, $field, $specification, $keys_new = array()) {
    +   * @param array $old_schema
    

    array[], and we should probably link to the schema documentation.

22. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -355,13 +389,13 @@ public function addField($table, $field, $specification, $keys_new = array()) {
    +   * @param array $new_schema
    

    array[], and we should probably link to the schema documentation.

23. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -355,13 +389,13 @@ public function addField($table, $field, $specification, $keys_new = array()) {
    +   * @param array $mapping
    

    string[]|array[]

24. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -420,10 +454,13 @@ protected function alterTable($table, $old_schema, $new_schema, array $mapping =
    +   * @return array
    

    array[], and we should probably link to the schema documentation.

25. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php
    @@ -568,6 +611,9 @@ public function changeField($table, $field, $field_new, $spec, $keys_new = array
    +   * @return array
    

    Not specific enough.

Rerolling first. Will work on comments in #41 once the tests pass.

I fixed for the comments in #41 and many related issues I found in these files. I don't think it is done yet but I am posting this for an incremental review.

Oh, and the patch.

Checking the automatic assign credits feature.

This issue is too undefined, and the patch is too big (and too full of problems) to really be feasible to (a) review it and (b) ever get it to the point where it's free enough of problems to be able to commit it.

It is vastly preferable to have targeted issues, where one type of problem is fixed throughout core, rather than generic "fix up everything on a set of files" issues, where when you are reviewing it (or making the patch), you have to think about all of the rules, standards, and accuracy at the same time.

So. I don't think this was a good way to break up the parent issue. We need to break it up by type of fix, not by file.