Add @param and @return or fix types in @param and @return in core/lib/Drupal/Core/Database/

Patch applies and changes make sense to me.
One remark, in StatementPrefetch:setFetchMode() you added @param $a2 and @param $a3 without a type.

I don't see how we can justify doing this issue during the Beta phase? It is documentation, but it seems like it would be disruptive to other patches. I think we should probably postpone until after 8.0.x is out? See https://www.drupal.org/contribute/core/beta-changes

I don't think that "may conflict with other patches" counts as "disruptive".

A change is disruptive if it:

• Introduces a BC break that will affect many contributed modules, or require some contributed modules to make non-trivial changes
• Will require internal refactoring or rewriting of core subsystems, as these changes tend to introduce technical debt and regressions
• Will require widespread documentation or code style updates which are likely to conflict with other patches in the queue
• Will require changes across many subsystems and patches in order to make core consistent.

Let us please stop unnecessarily holding this up.
Poorly documented code = unpleasant to review = the perfect hiding place for vulnerabilities.

@alexpott recently killed another attempt to make a big "fix a lot of documentation" as disruptive:
#2329703-84: [meta] Spellchecking Drupal
So, which is it?

To clarify, I am not an authority on this stuff :)
I am just quite impatient allergic to see this being held up, and I would say that the linked doc page does clearly not imply that docblock changes are disruptive.
I also don't think that the database system is being changed a lot. Maybe something is going on there currently (dunno), but I think it is one of the more stable (or boring) parts of Drupal core.

This is very different than spellchecking.
This is adding missing types or fixing incorrect types.
And, according to the beta handbook, it is allowed.
If it is not allowed, then the handbook page should change.

Yes, this is definitely more important than spellchecking.
(though I don't mind if spellchecking fixes are also committed)

Since this is going slow, I moved over to a single-file issue at #2342521: Docblock fixes for core/lib/Drupal/Core/Database/Connection.php. Maybe we can get this one committed?
I still think we should do more than one file per issue. But for a start, I don't mind a one-file issue.

Going to work on this with Eda.

1. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -190,6 +190,8 @@
   +   * @param string $key
   

   Needs parameter description

2. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -356,6 +358,8 @@
   +   * @return \Drupal\Core\Database\Connection
   

   Needs parameter description

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

   Needs parameter description

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

   Needs a return value description

5. +++ 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 parameter description

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

   Needs parameter description

7. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -135,6 +135,12 @@ class StatementPrefetch implements \Iterator, StatementInterface {
   +   * @param \Drupal\Core\Database\Connection $connection
   

   Needs parameter description

8. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -135,6 +135,12 @@ class StatementPrefetch implements \Iterator, StatementInterface {
   +   * @param string $query
   

   Needs parameter description

9. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -135,6 +135,12 @@ class StatementPrefetch implements \Iterator, StatementInterface {
   +   * @param array $driver_options
   

   Needs parameter description

10. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
    @@ -241,12 +248,19 @@ protected function getStatement($query, &$args = array()) {
    +   * @return string
    

    Needs return description

11. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
    @@ -241,12 +248,19 @@ protected function getStatement($query, &$args = array()) {
    +   * @param array $a2
    

    Needs parameter description

12. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
    @@ -241,12 +248,19 @@ protected function getStatement($query, &$args = array()) {
    +   * @param array $a3
    

    Needs parameter description

Re-rolled the patch and adding the updated patch.

A few instances of missing punctuation on descriptions of params that should be quick to fix:

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -190,6 +190,9 @@
+   * Database connection key in settings.php which should be set as active

@@ -356,6 +359,9 @@
+   * Connection object to perform database queries

+++ b/core/lib/Drupal/Core/Database/Schema.php
@@ -185,16 +190,25 @@
+   * SQLite object for DatabaseConnection
...
+   * Implements the magic __clone function

+++ b/core/lib/Drupal/Core/Database/Statement.php
@@ -36,6 +36,10 @@ class Statement extends \PDOStatement implements StatementInterface {
+   * Connection object to perform database queries

+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -135,6 +135,19 @@ class StatementPrefetch implements \Iterator, StatementInterface {
+   * Php Database object
...
+   * Connection object to perform Database operations
...
+   * Query string to fetch data
...
+   * Connection Driver specific options

missing . at end of statement.

Rerolled after adding "." at end of statement.

Thanks a lot for working on this! Additional documentation is much-needed :)

1. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -190,6 +190,9 @@
   +   * Database connection key in settings.php which should be set as active.
   

   Such descriptions must always be indented with two extra spaces (so three spaces between the asterisk and the description). This occurs multiple times.

2. +++ b/core/lib/Drupal/Core/Database/Log.php
   @@ -107,11 +107,11 @@ public function end($logging_key) {
   +   * @param StatementInterface $statement
   

   Missing namespace.

3. +++ b/core/lib/Drupal/Core/Database/Log.php
   @@ -137,7 +137,7 @@ public function log(StatementInterface $statement, $args, $time) {
   +   * @return array
   

   @return array is never acceptable documentation, because gives absolutely no indication of what the array contains.

   In this case the array items can be of mixed types, so the documentation should read @return mixed[].

4. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -195,6 +207,8 @@ public function __construct($connection) {
   +   * @return string
   ...
      public function __clone() {
   

   __clone cannot return anything.

5. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -202,6 +216,8 @@ public function __clone() {
       * Implements PlaceHolderInterface::uniqueIdentifier().
   +   *
   +   * @return string
   

   This entire docblock should be changed to {@inheritdoc} and the typehint should be added to the method definitions in PlaceholderInterface.

6. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -217,13 +233,13 @@ public function nextPlaceholder() {
   +   * @return array
   

   As before, @return array is not specific enough.

7. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -324,11 +340,11 @@ public function tableExists($table) {
   +   * @return string[]
   

   Exactly! :)

8. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -671,10 +687,12 @@ public function createTable($name, $table) {
   +   * @param array $fields
   

   Just like @return array is not specific enough, @param array is not specific enough either.

9. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -241,12 +255,19 @@ protected function getStatement($query, &$args = array()) {
   +   * @param array $a2
   +   * @param array $a3
   

   I don't know if these parameters can be arrays, but they can definitely (also) be NULL, as the default value proves.

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

We are going to reroll patch form #17 (old) because patch in #21 is intented for another issue

reroll of patch #17

removing trailing spaces . reroll #25

Additional documentation rerolled from #17 in log.php and database.php

added improvements in documentation as suggested in #18

this patchs gathers all modifications from #25,#26,#27,#28

this patchs gathers all modifications from #25,#26,#27,#28

sorry for double post...

I'm working on this issue at DrupalCon Barcelona.

The documentation for parameter $key in function openConnection in Database.php says that it defaults to 'default', because it's called from getConnection where the parameter is defaulted to 'default'.

But the parameter is not defaulted explicitly to 'default':
final public static function getConnection($target, $key = NULL) {

Wouldn't it be better to specify this explicitly to avoid any potential future problem? I.e.:
final public static function getConnection($target = 'default', $key = NULL) {

Reviewed @param and @return doc for Database.php and added a few.

Full patch with all modifications from #30 (#25,#26,#27,#28) and interdiff attached.

Thanks for the patch!

Here's the beginning of a review:

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -127,7 +127,7 @@
    * @param string $key
    *   The database connection key for which we want to log.
    *
-   * @return array
+   * @return mixed[]
    *   The query log for the specified logging key and connection.

If you check the code of that method, you see that it returns either NULL or the return value of Log->get().

So if you look at *that,* you see that Log.php is missing quite a bit of docblock love itself.

Anyway, in this case, I'm pretty sure you'd say:

@return null|Log

...except with the fully-namespaced name of Log.

I'd wager there are other issues like this in the patch, but this is the first one I found.

Done as suggested in #36

Sorry, this issue was forgotten, and it is improperly scoped so at this point I am closing it as Won't Fix.
https://www.drupal.org/core/scope