Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Proposed resolution
Correct the following in the core dblog module:
- Ensure that all
@return
lines are preceded by a blank line. - Ensure that
@see
and@ingroup
lines are always at the end of docblocks. - Ensure that all lines in doxygen blocks are 80 characters or fewer (excluding the terminal newline).
- Ensure that all functions, classes, interfaces, and methods have one-line summaries that are clear, use the correct verb tense, and follow specific standards in http://drupal.org/node/1354.
- Make incidental style and grammar corrections only to those docblocks already being updated.
Remaining tasks
Patch needs to be rolled.
User interface changes
None.
API changes
None.
Follow-up issues
None.
Comment | File | Size | Author |
---|---|---|---|
#55 | 1326600-55-dblog-docs-cleanup.patch | 28.16 KB | dcam |
#49 | 1326600-49-dblog-docs.patch | 4.48 KB | Lars Toomre |
#47 | 1326600-47-dblog-docs.patch | 3.98 KB | Lars Toomre |
#44 | cleanup_dblog_module_docs-1326600-44.patch | 492 bytes | batigolix |
#41 | cleanup_dblog_module_docs-1326600-41.patch | 487 bytes | batigolix |
Comments
Comment #1
Lars Toomre CreditAttribution: Lars Toomre commentedAttached is a first pass at bringing the dblog module up to Drupal 8 documentation standards. I relaize this patch will need to be re-rolled once the files are moved to /core. However, comments in the meantime are welcome.
While creating this patch, I encountered a number of questions which also may necessitate re-rolling this patch. My questions include:
Should members of an associative array have their variable type documented in a specific manner?
For instance, should the variable $variables in theme_dblog_message() be documented something akin to the following?
are covered in tests in DBLogTestCase::verifyReports()? Should one add '@see DBLogTestCase::verifyReports()' at the end of the docblock?
Known issues requiring follow-up
Comment #2
jhodgdonIn answer to your questions:
1) @see - no . -- this is on http://drupal.org/node/1354 (search for @see on the page)
2) We are not cleaning up data types in these cleanup patches. And we don't have a standard to answer the question you posed - can you please file an issue to discuss?
3) No blank line is necessary.
4) We're not fixing up coding standards, only docs standards.
5) I don't think functions should have @see to point to their tests.
6) That is under discussion. There are two different standards that conflict. See issue:
#338403: Use {@inheritdoc} on all class methods (including tests)
(latest comment that I added a few minutes ago)
Comment #3
Lars Toomre CreditAttribution: Lars Toomre commented@jhodgdon Thanks for the answers to my questions in item #1. I will go back and review this patch in the next couple of days after /core files are moved. Also to answer your question in a couple of other issues, I currently am working on patches for documentation API clean up in three of the modules (dblog, image and locale) as well as the lists formatting issue. I will turn to other modules once these four issues are further along.
Comment #4
jhodgdonOK. Going forward, it would probably be best to wait on filing the issues until you're ready to work on them. Also, one of them didn't get into the list on the meta issue.
I also took a quick look at part of your patch and noticed a couple of other things to address:
a) It looks like it needs a few "the", "of", etc. words in the one-line function descriptions. For instance:
Creates list dblog administration filters that can be applied.
==>
Creates a list of dblog administration filters that can be applied.
b) Two dimension array ==> Two-dimensional array
c) Please don't touch the code lines, such as:
This cleanup should be documentation fixes only.
d)
Returns a HTML-formatted ... => an HTML
(this needs to be fixed in several places)
e) Most of the form generating function docs are cleaned up correctly. Missed this one:
Submission callback for dblog_clear_log_form().
(should be Submission handler for...)
f) " The dblog module monitors ..." This should either be "The Database logging module" or "dblog.module".
g) Regarding your @todo -- yes, each class, function, method, constant, etc. requires a docblock.
Comment #5
jhodgdonLars Toomre: Are you still planning to work on this? If so, please respond. If not, we'll unassign it so someone else can.
Also, tagging.
Comment #6
Lars Toomre CreditAttribution: Lars Toomre commentedI thought that I had unassigned myself from this already. I am not working on this and do not plan to.
Comment #7
xjmComment #8
kid_icarus CreditAttribution: kid_icarus commentedRerolled #1. This patch applies cleanly at commit 5e0eac1.
Comment #9
kid_icarus CreditAttribution: kid_icarus commentedComment #10
kid_icarus CreditAttribution: kid_icarus commentedThis should address everything in #4 :)
Comment #11
jhodgdonHi kit_icarus -- thanks for taking this on! I found a few things that need to be fixed in this patch:
a) In between the time of this first patch and now, the standards for hook_menu() callbacks have changed, and we're not putting in lines like:
any more. Current standards are at:
http://drupal.org/node/1354#menu-callback
b) Right at the top of the patch:
The first sentence needs to be compressed into one 80-character-or-less line, see
http://drupal.org/node/1354#general
c)
Should follow standards at http://drupal.org/node/1354#themeable (doesn't need @return).
d) Another standards change:
Should be "Database Logging module" (capitalization).
e)
This last sentence is very terse. Add "the", punctuation, etc. to make it read better.
f)
First line should follow 2nd example in this section http://drupal.org/node/1354#hookimpl
g)
Should say "Database Logging module" here, in both places... Really, that should be done throughout the docs in this module instead of saying "dblog" when it refers to the module, and in other places, maybe it should say "database log" or just "log" if the non-word "dblog" is being used to refer to the log rather than the module.
h)
Sentence is terse, needs "that" "the", etc.
Comment #12
kid_icarus CreditAttribution: kid_icarus commentedThanks for the review!
Here are my attempts to address everything in #11. I found it difficult to limit one-line function descriptions to 80 meaningful characters without being terse, and it felt awkward writing the terms 'log' and 'database log' interchangably.
Comment #13
xjmThanks @kid_icarus! This looks close to ready. I found only a couple minor things to fix:
The @see is not needed here (since there is a link in the summary).
While we're changing this line (which is technically out of scope, along with all the other inline comment changes, but they are good changes and I guess it is OK to fix them here): "Clean up" should be two words when it is a verb.
I think this should say "...the Database Logging module."
For this, I think we can say:
Tests the database log filter functionality at admin/reports/dblog.
The path line should be removed here (we reverted that addition to the documentation standard).
Also, can whoever rerolls the patch with these changes include an interdiff along with the full patch? Thanks!
Comment #14
kid_icarus CreditAttribution: kid_icarus commentedThis patch addresses #13. I attached an interdiff as requested :)
Thanks for the review @xjm! Do you know of any docs that describe various udpates to the comment formatting conventions?
Comment #15
xjmThanks @kid_icarus! The changes in the interdiff look good and address everything in my review. I did notice that there are a few stray points from @jhodgdon's review to address yet (not sure why I missed them before):
We should probably change these "dblog" to "log" or "database log" as we have done elsewhere.
These comments are still a bit terse and should have some articles ("a" or "the") added for clarity.
I looked up this method and I think this description wasn't actually accurate. I don't think they're actually nodes at all?
Maybe:
"Confirms that database log reports are displayed at the correct paths."
I also applied the patch locally and checked for any missed corrections. The CSS files need
@file
docblocks, and the docblock fordblog_clear_log_form()
is not quite to standard for submission handlers. Reference: http://drupal.org/node/1354#formsFinally, I noticed the patch does not apply currently, so it should be rebased before making further changes and the interdiff for them. Thanks!
Comment #16
xjmOh, regarding the history of our doc standards, there's two good resources I can think of: the revision history for 1354 and the general list of change notifications.
Comment #17
kid_icarus CreditAttribution: kid_icarus commentedRe-rolled. Patch applies cleanly at b4c77c0.
Comment #18
kid_icarus CreditAttribution: kid_icarus commentedOnce again, thanks for the review @xjm!
This should address #15, as well as other edits I saw as necessary.
Comment #19
filijonka CreditAttribution: filijonka commenteddblog.admin.inc
General: Functions/methods one-line summary should start with a verb in the right tense
function dblog_overview()
function dblog_top
function dblog_event
function theme_dblog_message
function dblog_filter_form
dblog_filter_form_submit
dblog_clear_log_form
* missing @return
function dblog_filter_form
function dblog_filter_form_validate
dblog_filter_form_submit
dblog_clear_log_form
* missing @params
dblog.module
shouldn't page level docblock always come first?
dblog.test
missing focblocks for the protected variables
missing docblock for function getInfo()
private functions shouldn't be shown.
Comment #20
filijonka CreditAttribution: filijonka commentedComment #21
jhodgdonRE #19... Not everything in that review is quite correct...
a) We don't put docblocks in for getInfo() in tests.
b) I'm not sure what you mean by "private functions shouldn't be shown".
c) When reviewing it's very helpful to point out specifics. So for instance if there are verbs in the wrong tense, please say which ones. There are exceptions to the policy. See http://drupal.org/node/1354 for details.
d) Some functions do *not* include params and return value... such as theme functions. See http://drupal.org/node/1354 for details.
So I think I'll set this back to Needs Review, and wait for specifics and correctlys.
Comment #22
filijonka CreditAttribution: filijonka commentedsorry for tryingto help
Comment #23
jhodgdonfilijonka: Sorry you took my hopefully-constructive suggestions as a request that you not help. Nothing could be further from the truth! We value all attempts to help, but at the same time, I needed to make sure that the suggestions you made were not acted on, in the cases where they weren't correct. Also, I wanted to offer some suggestions to you for the next time you attempt a review.
Comment #24
jhodgdonSorry for the delay in getting this patch reviewed! I took a fresh look at the patch in #18 (just looking at the patch itself -- I didn't yet apply the patch to see if there were other things that needed fixing). I think there are a couple of things we should fix:
a) In the CSS files, I think we've been putting a blank line between the @file block and the beginning of the CSS.
*** dblog.admin.inc
b) The word entirety is misspelled here:
Also, shouldn't it be "Full-length messages"?
c) Whenever you use "e.g.", it should be followed by a comma, as in:
should be (e.g., 'search').
d)
I'm not happy with the @return section here:
- I believe all arrays in PHP are two-dimensional. I think you should just call it "array" or "associative array".
- The grammar here is misleading -- it says the *keys* are arrays. Should probably say "each of whose elements are arrays with the following keys". But actually, how about just leaving out the details of 'type' and 'severity -- since the function description is more generic (a list of filters that can be applied), can't the return value description be generic? So how about:
Associative array of filters. The top-level keys are used as the form element name for the filters, and the values are arrays with the following keys:
e)
Actually, the original was better than the patch. See
http://drupal.org/node/1354#themeable
f)
Why did this line get deleted? At a minimum it is out of scope for this issue... and I'm not sure our coding standards even suggest not having blank lines here.
g) The "word" dblog is used quite a few times in dblog.admin.inc documentation. Please replace with something that is actually a word, such as "database logging", or if it refers to the module, "the Database Logging module".
*** dblog.test ***
h) We aren't generally cleaning up in-code comments in the cleanup patches, but if you are going to fix this line:
you could also fix "ids". "id" is a psychological term. Use "ID" for identifier, and its plural is "IDs".
Comment #25
batigolixi ll give this a shot
Comment #26
batigolixattached patch incorporates the comments from #24
Comment #28
batigolixcannot reroll the patch from #18
Comment #29
jhodgdonWhat problem are you having rerolling the patch? See
http://drupal.org/patch/reroll
And if you no longer plan to work on this issue, please go ahead and un-assign it. Thanks for trying!
Comment #30
jhodgdonComment #31
batigolixi followed patch-reroll instructions from xjm's blog. i ll try with the info in the link above
Comment #32
batigolixHere is a new patch that is a reroll of #18 and that incorporates the comments from #24.
Comment #33
jhodgdon#32: cleanup_dblog_module_docs-1326600-32.patch queued for re-testing.
Comment #34
jhodgdonI'm sorry for the delay in reviewing this! The patch is mostly excellent, but I have a couple of concerns:
a) dblog_overview()
We don't usually want to have specific paths mentioned in functions. They are always in flux and it gets hard to maintain them. Maybe an @see for the dblog_event() function would be better?
b) dblog_event()
I don't think we should call this a "watchdog" ID, unless you are talking about a specific field of the {watchdog} table, in which case {watchdog} should be in {} followed by .fieldname.
c) dblog_filters() return value
- ...as the form element name... => names
- ...with the following keys: => elements
- I think on the 'options' key I would say "Array of options for the select list for the filter".
- That last line... "if no records exist in the table" -- that doesn't say what table it's talking about, and nowhere else does it say anything about what filters should normally be returned. Maybe it would be best just to leave this out... either that or to say something more about what filters should normally be returned?
d) dblog_clear_form()
Should not say "dblog"... maybe something like "Form constructor for the form that clears out the log"?
e)
I don't think we want a blank line at the top of the file, and there should be one after the @file block. Applies to the RTL CSS file too.
f)
At the moment, our standard is not to document test setUp() methods. Sigh.
g) Generally this cleanup is only supposed to do /** */ comments, not in-code // comments... I would prefer to have those left out of the patch (they are difficult to review). I definitely would *not* do changes like this in code at all:
h) This cleanup effort is also not supposed to be adding param/return types:
Again, it makes the patch longer and more difficult to review.
Comment #35
batigolixProcessed the feedback from #34
Regarding your remark:
There are many in-code comments in this patch. Do you prefer to have them removed?
Comment #37
batigolixnew attempt
Comment #39
batigolixarr. new attempt
Comment #40
jhodgdonThanks! What is in the patch looks good, so I went ahead and committed it to 8.x.
There is one small follow-up needed: In the test file lib/Drupal/dblog/Tests/DBLogTest.php, the test class has no doc block.
Comment #41
batigolixAdded the doc block.
There is a spelling mistake elsewhere in the file (not in the api docs section):
Comment #42
batigolixchanged status
Comment #43
Cameron Tod CreditAttribution: Cameron Tod commentedThis is missing the definite article, and some punctuation. How about:
"Tests logging messages to the database."
Thanks!
Comment #44
batigolixfixed
Comment #45
Cameron Tod CreditAttribution: Cameron Tod commentedLooks great, thanks mate.
Comment #46
jhodgdonCommitted to 8.x. Now I think we can port this entire thing to 7.x. Thanks!
Comment #47
Lars Toomre CreditAttribution: Lars Toomre commentedI did a review of the complete Dblog module and must say that this is in the best shape yet from an API docs perspective. I found a couple of small items that are addressed in the attached patch. Hopefully, these can be added to D8 before the full backport to D7.
Comment #48
jhodgdonThanks! This is mostly good cleanup... A couple of things:
a)
I wouldn't say "the $id". Either "the ID" or just "$id".
b)
Should be "installation *profile* to use...".
Other than that, looks great, thanks!
Comment #49
Lars Toomre CreditAttribution: Lars Toomre commentedIn the last week, the member $profile was eliminated from DBLogTest.php, hence #47 needed to be re-rolled anyways. This locally untested patch addresses the point in 48 a) by using 'the ID'.
Comment #51
Lars Toomre CreditAttribution: Lars Toomre commentedI think the fail in #49 is unrelated to this patch since it is occurring in UserAccccountLinksTest. Let's try the bot again.
Comment #52
Lars Toomre CreditAttribution: Lars Toomre commented#49: 1326600-49-dblog-docs.patch queued for re-testing.
Comment #53
jhodgdonThis looks good, thanks! I'll get it committed shortly (hope it still applies).
Comment #54
jhodgdonThanks! That last patch is in 8.x now. We have several patches to port to d7 as much as possible (they can be combined).
Comment #55
dcam CreditAttribution: dcam commentedBackported #'s 39, 44, and 49 to D7.
Comment #57
jhodgdon#55: 1326600-55-dblog-docs-cleanup.patch queued for re-testing.
Comment #58
jhodgdonThank you! This looks fine -- I'll get it committed in the next few days.
Comment #59
jhodgdonThanks! Committed this patch to 7.x and this issue is DONE! If you have follow-ups, at this point open another issue.
Comment #60.0
(not verified) CreditAttribution: commentedAdded follow-up issues header to summary.