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.
| Comment | File | Size | Author |
|---|---|---|---|
| #30 | interdiff-2594845.txt | 647 bytes | snehi |
| #30 | many_database_driver-2594845-30.patch | 15.62 KB | snehi |
| #28 | interdiff-2594845.txt | 625 bytes | snehi |
| #28 | many_database_driver-2594845-28.patch | 15.63 KB | snehi |
| #26 | many_database_driver-2594845-26.patch | 15.6 KB | anil280988 |
Comments
Comment #2
cilefen commentedAs I understand it, this is rc eligible.
Comment #3
dpopdan commentedComment #5
dpopdan commentedComment #6
dpopdan commentedComment #7
cilefen commented@dropdan Nice work! What is the significance of the different wording for the SQLite classes?
Comment #8
dpopdan commentedFor 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.
Comment #9
jhodgdonLet'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:
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.
Comment #10
er.manojsharma commentedPlease review my patch
Comment #11
er.manojsharma commentedComment #12
er.manojsharma commentedPlease ignore last patch , uploading correct patch
Comment #14
er.manojsharma commentedPlease find updated patch
Comment #15
er.manojsharma commentedComment #16
cilefen commentedRe #9, this should be "MySQL implementation of DatabaseConnection.", and similarly with the ones following. We want them all on the same pattern.
Comment #17
er.manojsharma commentedPlease check updated patch
Comment #18
cilefen commentedShould this be "MySQL implementation of Delete."?
Should this be "MySQL implementation of Insert."?
Should this be "MySQL implementation of Merge."?
Should this be "MySQL implementation of Select."?
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.
Comment #19
tarekdj commentedComment #20
cilefen commentedThat is much better.
Comment #21
jhodgdonLooks great! Just one that might not be right:
I think this is actually the implementation of Statement. It happens to extend StatementPrefetch, but my understanding is that this is an implementation detail?
Comment #22
tarekdj commentedUmm! You're right! reverting Statement.php.
Comment #23
jhodgdonHm. Questionable interdiff file. Seems to be the patch file instead? Anyway... so I just looked at the sqlite Statement class... still not quite right:
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!
Comment #24
tarekdj commented@jhodgdon what about :
Comment #25
jhodgdonNo, 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.
Comment #26
anil280988 commentedHi Jhodgdon,
Changed "SQLite implementation of DatabaseConnection." to "SQLite implementation of Statement." Is this what you suggested. If not, could you suggest on this.
Comment #27
jhodgdonRight! 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
Comment #28
snehi commentedAdding namespace.
Comment #29
jhodgdonOK, 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.
Comment #30
snehi commentedDone.
Comment #31
jhodgdonThanks! Looks right now.
Comment #32
alexpottCommitted 7e245c9 and pushed to 8.0.x. Thanks!