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

+++ b/core/lib/Drupal/Core/Database/Query/Condition.php
@@ -280,10 +285,10 @@ function __clone() {
    *   The extra handling directives for the specified operator, or NULL.

This function can not return NULL. So the patch is correct, but the existing comment is not.

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

NULL should be lowercase
The existing comment does not mention returning the StatementInterface object, I think this should be added.

+++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php
@@ -135,9 +141,11 @@ public function getCountQuery() {
+   * @param int $limit

According to the comment this also accepts FALSE. So this should be int|false

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

Add a comment that this is the element id that is used to differentiate different pager queries.

+++ b/core/lib/Drupal/Core/Database/Query/SelectInterface.php
@@ -29,7 +29,7 @@
+   * @return array[]

Since this is also by reference should an "&" be appended (as is done in Query.php)?

Same for some other @return comments in SelectInterface.php

Please review my patch

1. +++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php
   @@ -135,7 +135,8 @@ public function getCountQuery() {
   +   * @param int|false $limit
   

   Doesn't look like we need this twice.

2. +++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php
   @@ -160,7 +161,8 @@ public function limit($limit = 10) {
   +   * This his is the element id that is used to differentiate different pager queries
   

   Redundant 'his' should be removed.

UPloaded new patch and interdiff

patch apply Condition.php,Insert.php,PagerSelectExtender.php only.others are comment lines included.

Sorry, but this patch isn't quite right:

a)

-   *   The extra handling directives for the specified operator, or NULL.
+   *   The extra handling directives for the specified operator.
    */
   protected function mapConditionOperator($operator) {

This change needs a careful review. Can the function return NULL?

b)

+++ b/core/lib/Drupal/Core/Database/Query/Insert.php
@@ -186,7 +186,7 @@ public function from(SelectInterface $query) {
   /**
    * Executes the insert query.
    *
-   * @return
+   * @return  \Drupal\Core\Database\StatementInterface|int|null
    *   The last insert ID of the query, if one exists. If the query
    *   was given multiple sets of values to insert, the return value is
    *   undefined. If no fields are specified, this method will do nothing and

There should only be one space beween @return and the type. Also, given the text below saying the return value is undefined, I don't really think we should be giving a return type here? Very odd to be specifying a return type and then saying it's undefined. ?!?

c)

+++ b/core/lib/Drupal/Core/Database/Query/PagerSelectExtender.php

@@ -160,7 +160,8 @@ public function limit($limit = 10) {
    * explicitly, so it is possible for two pagers to end up using the same ID
    * if both are set explicitly.
    *
-   * @param $element
+   * @param  $element
+   * This is the element id that is used to differentiate different pager queries
    */
   public function element($element) {

The spacing is wrong ehre. One space between @param and $element, and the next line indented 2 spaces.
Also "id" should be "ID", and we don't normally say "This is...". And it needs to end in .

Apology , uploaded correct patch

Thanks for the patch! It still needs some work.

a) Condition.php

   * @return
-   *   The extra handling directives for the specified operator, or NULL.
+   *   The extra handling directives for the specified operator.
    */
   protected function mapConditionOperator($operator) {

I looked at the function code. It actually returns an empty array if there are no extra handling directives. The @return line should be "@return array" and the description should say this.

b) Insert.php -- After the patch, this says:

   * @return \Drupal\Core\Database\StatementInterface|int|null
   *   The last insert ID of the query, if one exists. If the query
   *   was given multiple sets of values to insert,
   *   If no fields are specified, this method will do nothing and
   *   return NULL. That makes it safe to use in multi-insert loops.
   */
   public function execute() {

Read it. It doesn't make sense, and isn't wrapped as a paragraph. Also it lost the information that said "If the query was given multiple sets of values to insert, the return value is undefined", which is still correct and needs to be there. In which case, that "@return \Drupal\Core\Database\StatementInterface|int|null" is just wrong. Take out the type, we don't know what the return value is.

c) PagerSelectExtender

   * @param int|false $limit
    *   An integer specifying the number of elements per page.  If passed a false
    *   value (FALSE, 0, NULL), the pager is disabled.

If we are going to fix up the docs, let's take out the extra space before "If" in the middle line here.

d) Same class

    * @param $element
+   * element ID that is used to differentiate different pager queries
    */
   public function element($element) {

After @param, the next line should start with a capital letter and be indented two spaces, and end with ".".

Corrected with suggestions from #17.

Insert::execute() still isn't formatted correctly:

+   * @return \Drupal\Core\Database\StatementInterface
+   *     The last insert ID of the query, if one exists. If the query
+   *   was given multiple sets of values to insert, the return value
+   *   is undefined. If no fields are specified, this method will do
+   *   nothing and return NULL. That makes it safe to use in 
+   *   multi-insert loops.
    */
   public function execute() {

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

And the return type still just needs to be omitted. It is not returning a StatementInterface necessarily.

The rest of the changes look good, thanks!

@jhodgdon So do I remove the "@return" altogether or just the "\Drupal\Core\Database\StatementInterface" and what do you mean as "isn't wrapped as a paragraph" in #17? I looked at other paragraph inside Insert.php and they're all no indented.. Thanks for the review btw!

No, we need the @return, just not the class that follows it. Thanks for asking for clarification!

Regarding #17, I think you already fixed that in the latest patch, but what I meant about paragraph wrapping was from this example:

  *   The last insert ID of the query, if one exists. If the query
   *   was given multiple sets of values to insert,
   *   If no fields are specified, this method will do nothing and
   *   return NULL. That makes it safe to use in multi-insert loops.

If a bunch of documentation lines are together in a paragraph, we want each line to extend to as close to 80 characters as possible without going over. That second line needs more words on it to comply with that standard.

Okay, changed that! Thanks for the info :)

Looks good, missing the word "makes" on the last line of this though:

+   * @return
+   *   The last insert ID of the query, if one exists. If the query was given
+   *   multiple sets of values to insert, the return value is undefined. If no
+   *   fields are specified, this method will do nothing and return NULL. That
+   *   it safe to use in multi-insert loops.

should be "That makes it safe..."
:)

Done implemented

Looks good now, thanks!

Committed 8723012 and pushed to 8.0.x. Thanks!

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