Every class should have a doc block. There are a bunch of database driver classes that do not:
- Nearly all of the classes under /core/lib/Drupal/Core/Database/Driver/pgsql
- About half of the classes in /core/lib/Drupal/Core/Database/Driver/sqlite
- Nearly all of the classes under /core/lib/Drupal/Core/Database/Driver/mysql

Given the scope of the missing doc blocks, this is Major.

However, since the added doc blocks can be fairly simple, like:

PostgreSQL implementation of [whatever it is].

I think this is a good Novice issue.

Comments

jhodgdon created an issue. See original summary.

cilefen’s picture

Issue tags: +rc eligible

As I understand it, this is rc eligible.

dpopdan’s picture

Assigned: Unassigned » dpopdan
dpopdan’s picture

StatusFileSize
new10.66 KB
dpopdan’s picture

Status: Active » Needs review
cilefen’s picture

@dropdan Nice work! What is the significance of the different wording for the SQLite classes?

  1. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Update.php
    @@ -11,6 +11,9 @@
    +/**
    + * Implements the Update query for the PostgreSQL database driver.
    + */
    
  2. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Merge.php
    @@ -9,4 +9,7 @@
    +/**
    + * SQLite specific implementation of \Drupal\Core\Database\Query\Merge.
    + */
    
dpopdan’s picture

For core/lib/Drupal/Core/Database/Driver/sqlite/Upsert.php comment was already set and it is "SQLite specific implementation of" and for core/lib/Drupal/Core/Database/Driver/pgsql/Upsert.php and mysql too, is "Implements the [class] query for the [*]SQL database driver." so the only option was to keep the wording template or to change it for the Upsert class.

jhodgdon’s picture

Status: Needs review » Needs work

Let's make all of the database driver docs consistent with each other.

Personally I prefer this one because it gives a link to the base class:

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Merge.php
@@ -9,4 +9,7 @@
+/**
+ * SQLite specific implementation of \Drupal\Core\Database\Query\Merge.
+ */

Although it is a bit annoying about the grammar/punctuation...

How about taking out the word "specific":

SQLite implementation of \Drupal\Core\Database\Query\Merge.

That is one word more compact, and has I think the maximum information.

er.manojsharma’s picture

Please review my patch

er.manojsharma’s picture

Status: Needs work » Needs review
er.manojsharma’s picture

Please ignore last patch , uploading correct patch

Status: Needs review » Needs work

The last submitted patch, 12: d8_core-database_driver_docs-2594845-12-d8.patch, failed testing.

er.manojsharma’s picture

StatusFileSize
new10.62 KB

Please find updated patch

er.manojsharma’s picture

Status: Needs work » Needs review
cilefen’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -21,6 +21,9 @@
+/**
+ * Implements the DatabaseConnetion for the MySQL database driver.
+ */
 class Connection extends DatabaseConnection {

Re #9, this should be "MySQL implementation of DatabaseConnection.", and similarly with the ones following. We want them all on the same pattern.

er.manojsharma’s picture

Status: Needs work » Needs review
StatusFileSize
new14.99 KB

Please check updated patch

cilefen’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Delete.php
    @@ -9,4 +9,7 @@
     
    +/**
    + * MySQL implementation of DeleteQuery.
    + */
     class Delete extends QueryDelete { }
    

    Should this be "MySQL implementation of Delete."?

  2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Insert.php
    @@ -9,6 +9,9 @@
    +/**
    + * MySQL implementation of InsertQuery.
    + */
     class Insert extends QueryInsert {
    

    Should this be "MySQL implementation of Insert."?

  3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Merge.php
    @@ -9,4 +9,7 @@
     use Drupal\Core\Database\Query\Merge as QueryMerge;
     
    +/**
    + * MySQL implementation of MergeQuery.
    + */
    

    Should this be "MySQL implementation of Merge."?

  4. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Select.php
    @@ -9,4 +9,7 @@
    +/**
    + * MySQL implementation of SelectQuery.
    + */
     class Select extends QuerySelect { }
    

    Should this be "MySQL implementation of Select."?

  5. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Transaction.php
    @@ -9,4 +9,7 @@
     use Drupal\Core\Database\Transaction as DatabaseTransaction;
     
    +/**
    + * MySQL implementation of DatabaseTransaction.
    + */
     class Transaction extends DatabaseTransaction { }
    

    This should be "MySQL implementation of Transaction.".

I think there are more, but each of these comments should refer to the real class name being implemented, not the alias from the use statement.

tarekdj’s picture

Status: Needs work » Needs review
StatusFileSize
new15.63 KB
new16.16 KB
cilefen’s picture

That is much better.

jhodgdon’s picture

Looks great! Just one that might not be right:


+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php
@@ -11,7 +11,7 @@
 use Drupal\Core\Database\StatementInterface;
 
 /**
- * Specific SQLite implementation of DatabaseConnection.
+ * SQLite implementation of \Drupal\Core\Database\StatementPrefetch.

I think this is actually the implementation of Statement. It happens to extend StatementPrefetch, but my understanding is that this is an implementation detail?

tarekdj’s picture

StatusFileSize
new15.61 KB
new15.54 KB

Umm! You're right! reverting Statement.php.

jhodgdon’s picture

Status: Needs review » Needs work

Hm. Questionable interdiff file. Seems to be the patch file instead? Anyway... so I just looked at the sqlite Statement class... still not quite right:

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php
@@ -11,7 +11,7 @@
- * Specific SQLite implementation of DatabaseConnection.
+ * SQLite implementation of DatabaseConnection.

No, actually, this one should be that it's an implementation of Statement, right? It's definitely not an implementation of DatabaseConnection, which I doubt even exists.

Thanks!

tarekdj’s picture

@jhodgdon what about :

/**
 * SQLite implementation of \Drupal\Core\Database\StatementPrefetch.
 *
jhodgdon’s picture

No, it's the SQLite implementation of Statement. It uses StatementPrefetch for implementation.

So really if you want to get technical, all of these are implementations of various interfaces. We're using the base class names as kind of a shorthand, and I don't think we should do otherwise.... but if you want to get really technical every single one of the doc blocks should say they're implementations of an interface, not of a class.

anil280988’s picture

Status: Needs work » Needs review
StatusFileSize
new15.6 KB

Hi Jhodgdon,
Changed "SQLite implementation of DatabaseConnection." to "SQLite implementation of Statement." Is this what you suggested. If not, could you suggest on this.

jhodgdon’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php
@@ -11,7 +11,7 @@
+ * SQLite implementation of Statement.

Right! But this also needs to have the namespace on the Statement class name.

Everything else looks good in this patch.

PLEASE next time (and in general), when you upload a new patch on an issue that already had a patch, make an interdiff file. Thanks!
https://www.drupal.org/documentation/git/interdiff

snehi’s picture

Status: Needs work » Needs review
StatusFileSize
new15.63 KB
new625 bytes

Adding namespace.

jhodgdon’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Statement.php
@@ -11,7 +11,7 @@
+ * SQLite implementation of \Drupal\Core\Database\StatementInterface.

OK, but... Every other line in this patch says "[Driver] implementation of [Class]". This one is "[Driver] implementation of [Interface]."

We should be consistent.

Really Interface is more correct. However, I didn't want to say anything earlier because the patch had the class names everywhere and it would be annoying to have to change it. So I think we should just change this line from StatementInterface to Statement. Then they'll all be consistent.

snehi’s picture

Status: Needs work » Needs review
StatusFileSize
new15.62 KB
new647 bytes

Done.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Looks right now.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7e245c9 and pushed to 8.0.x. Thanks!

  • alexpott committed 7e245c9 on 8.0.x
    Issue #2594845 by er.manojsharma, tarekdj, snehi, dpopdan, anil280988,...

Status: Fixed » Closed (fixed)

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