The database driver task classes have missing or outdated documentation.

Files: 
CommentFileSizeAuthor
#18 drupal_2084659_18.patch579 bytesXano
PASSED: [[SimpleTest]]: [MySQL] 59,098 pass(es).
[ View ]
#13 drupal-2084659-13.patch4.44 KBju1iet
PASSED: [[SimpleTest]]: [MySQL] 58,557 pass(es).
[ View ]
#13 interdiff.txt484 bytesju1iet
#9 drupal_2084659_9.patch4.41 KBXano
PASSED: [[SimpleTest]]: [MySQL] 58,537 pass(es).
[ View ]
#5 interdiff.txt1008 bytesXano
#5 drupal_2084659_5.patch4.36 KBXano
PASSED: [[SimpleTest]]: [MySQL] 58,531 pass(es).
[ View ]
#1 drupal_2084659_1.patch5.17 KBXano
PASSED: [[SimpleTest]]: [MySQL] 58,950 pass(es).
[ View ]

Comments

Assigned:Xano» Unassigned
Status:Active» Needs review
StatusFileSize
new5.17 KB
PASSED: [[SimpleTest]]: [MySQL] 58,950 pass(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal_2084659_1.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

Thanks! Good idea to fix up this documentation. A few issues in the patch:

a) This is not complete documentation: Return statements need a description.

+ *
+ * @return \Drupal\Core\Database\Install\Tasks
  */
function db_installer_object($driver) {

b) This is not a documentation change -- if you mix documentation and non-documentation changes in a patch like this, I'll have to move it to "database system" and it will probably take more review etc. before it can be committed:

-  function checkBinaryOutput() {
+  public function checkBinaryOutput() {

Please put these changes in a separate issue.

c) I don't see this on the base class that is being extended:

+
+  /**
+   * {@inheritdoc}
+   */
   protected $pdoDriver = 'pgsql';

This occurs several times in the patch. It needs documentation and can't use inheritdoc unless there is documentation to inherit.

d) If you really want to clean up the documentaiton, most or all of the methods on the abstract base class do not follow our first-line verb coding standards (e.g., they should say "Checks..." not "Check"), and the class itself doesn't either. But if you want to not fix that in this patch, that is OK too.

Status:Needs work» Needs review
StatusFileSize
new4.36 KB
PASSED: [[SimpleTest]]: [MySQL] 58,531 pass(es).
[ View ]
new1008 bytes

About the {@inheritdoc}, the patch adds code comments to the parent properties and methods.

Status:Needs review» Needs work

#4 (a) still is a problem here. I'll review the patch once that is fixed.

Do the coding standards require return value descriptions? I believe (and some other developers whose opinion I asked with me) that if an object of a particular interface or class is returned, unless there is additional context information, people should just look up the interface's or class's documentation.

Yes, return value descriptions are required. Something simple is fine, like "The node that was added." for a function like node_add().

Status:Needs work» Needs review
StatusFileSize
new4.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,537 pass(es).
[ View ]

+++ b/core/includes/install.inc
@@ -1103,6 +1103,9 @@ function db_run_tasks($driver) {
  *
  * @param $driver
  *   The name of the driver.
+ *
+ * @return \Drupal\Core\Database\Install\Tasks
+ *    Database-type specific installation tasks.
  */
function db_installer_object($driver) {

How about for the @return description: A class defining the requirements and tasks for installing the database.

Everything else looks great.

Status:Needs review» Needs work

Assigned:Unassigned» ju1iet

Assigned:ju1iet» Unassigned
Status:Needs work» Needs review
StatusFileSize
new484 bytes
new4.44 KB
PASSED: [[SimpleTest]]: [MySQL] 58,557 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

RTBC, since the only change is a code comment, which cannot possibly break any tests.

Status:Reviewed & tested by the community» Fixed

Committed 50ce328 and pushed to 8.x. Thanks!

Status:Fixed» Needs work

Quick follow-up needed:

  *   The name of the driver.
+ *
+ * @return \Drupal\Core\Database\Install\Tasks
+ *    A class defining the requirements and tasks for installing the database.
  */
function db_installer_object($driver) {

This has too many spaces in the line after @return. Should only be indented two spaces beyond the @ in the previous line.

Assigned:Unassigned» Xano

Assigned:Xano» Unassigned
Status:Needs work» Needs review
StatusFileSize
new579 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,098 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Thank you!

Status:Reviewed & tested by the community» Fixed

Thanks Xano - committed to 8.x.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.