Fix documentation style in core/lib/Drupal/Core/Database/StatementPrefetch.php

Note that in general, coding standards cleanups are not in scope during the beta, but patches that only touch documentation are unfrozen, so this is okay to work on. Thanks!

I am new to documentation standards, so forgive me if I'm not seeing everything. The only issue I can see right off is lines 101 and 149 where the comment goes past 80 columns. Is that everything wrong with it?

EDIT: Disregard. Found my answer.

Here's a patch to get this started.

As per the patch https://www.drupal.org/node/2478355#comment-9907191, the patch didn't worked. Please find the log message:

Hunk #2 succeeded at 87 (offset -4 lines).
Hunk #3 succeeded at 95 (offset -4 lines).
error: while searching for:
   */
  public $allowRowCount = FALSE;

  public function __construct(\PDO $dbh, Connection $connection, $query, array $driver_options = array()) {
    $this->dbh = $dbh;
    $this->connection = $connection;

error: patch failed: core/lib/Drupal/Core/Database/StatementPrefetch.php:135
error: core/lib/Drupal/Core/Database/StatementPrefetch.php: patch does not apply

Please find the updated patch for review.

While reviewing this patch I:
1. Downloaded and applied this patch successfully.
2. Manually reviewed the changes and have listed down the findings below -

1. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -131,6 +132,19 @@ class StatementPrefetch implements \Iterator, StatementInterface {
   +
   

   Remove extra empty line.

2. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -327,16 +344,26 @@ public function current() {
    
   

   Remove extra empty line.

3. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -327,16 +344,26 @@ public function current() {
   +   * Gets the next record.
   

   This function rather seems to Set the next record rather than getting one. I think the documentation should suggest that this function 'Sets the next record.'

4. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -438,6 +497,19 @@ public function fetchAssoc() {
   +   * @param  mixed $fetch_style
   

   Mark all params of fetchAll as optional.

@umarzaffer. Appreciate your review comments.

+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -131,6 +132,19 @@ class StatementPrefetch implements \Iterator, StatementInterface {
+

Remove extra empty line. As per the documentation https://www.drupal.org/node/1354#var class declaration is followed by a blank line.

+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -327,16 +344,26 @@ public function current() {
+   * Gets the next record.

This method does not gets the next record, rather it return the current record.

Rest of the changed are applied as per suggestion.

Good work @leeotzu. I have validated the reviews given by umarzaffer.

While reviewing this patch I:
1. Downloaded and applied this patch successfully.
2. Manually reviewed the changes and have listed down the findings below -

+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -131,6 +132,19 @@ class StatementPrefetch implements \Iterator, StatementInterface {
+

Remove extra empty line.
+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -327,16 +344,26 @@ public function current() {

Remove extra empty line.
+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -327,16 +344,26 @@ public function current() {
+ * Gets the next record.

This function rather seems to Set the next record rather than getting one. I think the documentation should suggest that this function 'Sets the next record.'
+++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
@@ -438,6 +497,19 @@ public function fetchAssoc() {
+ * @param mixed $fetch_style

Mark all params of fetchAll as optional.

All seems good
Moving this to RTBC

I think test has passed .. so moving to RTBC based upon #11 review by @AsianBot.

It's good see some documentation work going on in a part of the system that doesn't get many people looking at it...

1. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -327,16 +343,26 @@ public function current() {
   -  /* Implementations of Iterator. */
   +  /**
   +   * Implementations of Iterator.
   +   */
    
      public function key() {
   

   This looks odd. I think we just need to remove the comment and add

     /**
      * {@inheritdoc}
      */
   

   to document the key() method.

2. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -327,16 +343,26 @@ public function current() {
   +  /**
   +   * Rewinds the DatabaseStatement.
   +   *
   +   * This statement will do nothing. DatabaseStatement cannot be rewound.
   +   */
      public function rewind() {
   ...
   +  /**
   +   * Gets the next record.
   +   */
      public function next() {
   
   @@ -347,7 +373,9 @@ public function next() {
   +  /**
   +   * Validation to check if current row exists.
   +   */
      public function valid() {
   

   Just needs to be

     /**
      * {@inheritdoc}
      */
   

   these are documented on the IteratorInterface.

3. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -365,6 +393,9 @@ public function rowCount() {
   +  /**
   +   * {@inheritdoc}
   +   */
      public function fetch($fetch_style = NULL, $cursor_orientation = \PDO::FETCH_ORI_NEXT, $cursor_offset = NULL) {
   

   I can't find what this is inheriting from.

4. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -386,6 +417,13 @@ public function fetch($fetch_style = NULL, $cursor_orientation = \PDO::FETCH_ORI
   +   * Returns an entire single column of a result set as an indexed array.
   +   * @param  int $index
   

   Needs an new line in between.

5. Whilst reviewing this it looks like we have some bugs in StatementPrefetch::current(). $class_name = array_unshift($this->currentRow); looks broken since unshift is supposed to add something on to an array and return $this->currentRow[$k][$this->columnNames[$this->fetchOptions['column']]]; I'm not sure how $k can be defined. Can someone check if there is an existing issue and if not file a new issue to address an test this.

I have done the 1, 2, 4 changes as Alex suggested

@leeotzu Can you Please look into the 3rd and 5th point in #17

@alexpott: Your observation about unresolvable {@inheritdoc} is right. This is due to the fact that public function fetch in class StatementInterface is commented.

Initialisation of the variable $k is missing in public function current()

The line noted in #17.5 is definitely broken, no question. However, this class is only used by the SQLite driver, and that particular fetch mode is almost never used in core. That's why it's not been caught.

I have filed a separate bug here: #2499875: Fix bug with prefetch drivers and FETCH_CLASS.

The last test as per #19 passed. Moving to RTBC

Re #17.3 and #20. If it is commented then {@inheritdoc} is incorrect. Why is it commented?

@alexpott, I checked the commented method public function fetch in , it seems to have commented by Larry Garfield on 2011-12-27.
IMHO, public function fetch($fetch_style = NULL, $cursor_orientation = \PDO::FETCH_ORI_NEXT, $cursor_offset = NULL) does not need second and third parameter defined in the file StatementPrefetch.php.

+  /**
+   * Constructor for StatementPrefetch object.
+   *

Should be "Creates a new StatementPrefetch instance." - core generally prefers that style nowdays.

@@ -141,11 +154,12 @@ public function __construct(\PDO $pdo_connection, Connection $connection, $query
   /**
    * Executes a prepared statement.
    *
-   * @param $args
-   *   An array of values with as many elements as there are bound parameters in the SQL statement being executed.
-   * @param $options
+   * @param array $args
+   *   An array of values with as many elements as there are bound parameters
+   *   in the SQL statement being executed.
+   * @param array $options
    *   An array of options for this query.
-   * @return
+   * @return bool
    *   TRUE on success, or FALSE on failure.
    */

We need an empty line before @return.

+  /**
+   * Returns an entire single column of a result set as an indexed array.
+   *
+   * @param  int $index
+   *    The index of the column number to fetch.
+   * @return mixed
+   *    An indexed array, or FALSE if there is no result set.
+   */
   public function fetchColumn($index = 0) {

It's generally a good idea to use the verb from the method name. Other fetch methods in the same patch use the "Fetch" verb, we should do the same here (instead of "Returns") to make it consistent.

@bojanz: Thanks for a quick review of the patch. Please find the updated the patch file as per your suggestion.

Couple minor observations-

1. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -386,6 +426,15 @@ public function fetch($fetch_style = NULL, $cursor_orientation = \PDO::FETCH_ORI
   +   *    The index of the column number to fetch.
   

   Mark $index as Optional.
   Also, does it make sense to specify explicitly that if no value is specified then by default first column would be returned?

2. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -438,6 +507,21 @@ public function fetchAssoc() {
   +   *    If $mode is PDO::FETCH_COLUMN, the index of the column to
   

   Replace $mode with $fetch_style.

.

@Umar, thank you sharing you observations. Updated the patch file.

Minor fixes to the patch, which was missed in earlier comment patch.

@bojanz : Thanks for sharing your observation over IRC. Modified the patch accordingly.

Just a review on the interdiff.

1. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -398,8 +398,8 @@
   +   * @param $fetch_style
   +   *  (optional) One of the \PDO::FETCH_* constants.
   

   the indent on this line was fine. ( should be under the a in @param

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

2. if you are going to fix the indent on

   result as an object or FALSE.
   

   then you should probably make it cased correctly also.

   https://www.drupal.org/node/1354#return
   pattern would recommend something more like.

   An object or FALSE.
   

   but that is not super useful.
   maybe try and say what the object is, and when it returns false.

3. +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php
   @@ -429,12 +429,12 @@
   +   *   An indexed array, or FALSE if there is no result set.
   

   here is an example where it says when it returns false.

I'm working on this issue, I'll apply the patch before at the current version and fix the remaining problems related at #35.

Here is my patch.

I did the reroll from the comment #34 applying the modifications at the most recent drupal version. Changing to needs review.

Fixed the phpcs errors.

New patch passed in all tests, small changes applied in the latest version, it all looks good to me.

@beatrizrodrigues and @anagomes, Welcome to Drupal! Thank you for working to bring Drupal code up to standards.

However, we have a number of issues dealing with coding standards fixes and the community has decided that the best way to approach this is by fixing a rule at a time, rather than a file at a time.

Please see #2571965: [meta] Fix PHP coding standards in core, stage 1 where this effort is being organized. There you can find a variety of coding standard issues to work on.

The work adding a function doc block is part of #3212066: [meta] Fix Drupal.Commenting.FunctionComment.Missing . Closing this as a duplicate of that one.

thank you, @quietone