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 includes
files beginning with N-Z:
- Ensure that each file has an
@file
docblock. - 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
Create new patch following the "defacto standard" for line wrapping once core files are moved to the core
directory.
Comment | File | Size | Author |
---|---|---|---|
#45 | n-z-clean-up-1317628-45.patch | 35.57 KB | Albert Volkman |
#41 | n-z-clean-up-1317628-41.patch | 44.78 KB | Albert Volkman |
#41 | interdiff.txt | 7.01 KB | Albert Volkman |
#39 | n-z-clean-up-1317628-39.patch | 43.82 KB | Albert Volkman |
#39 | interdiff.txt | 1.47 KB | Albert Volkman |
Comments
Comment #1
xjmCleanup for character limit, lines above
@return
, and@see
/@ingroup
positioning. No fixes to summaries or file docblocks yet. Waiting on #1315992: No standard for documenting menu callbacks and perhaps #1317826: Provide general documentation standard for callbacks.Comment #1.0
xjmUpdated issue summary.
Comment #2
jhodgdonStandard is there... :)
http://drupal.org/node/1354#menu-callback
And we should probably not try to backport these issues to d7, as webchick does not appear to be receptive to this type of cleanup.
Comment #3
xjmLars: Don't spend time reviewing this!
Comment #4
xjmPostponing for core directory change.
Comment #4.0
xjmUpdated issue summary.
Comment #5
jhodgdonThere has also been a start to the select.inc cleanup on another issue:
#1255524: select.inc needs more doxygen
which I'm closing as a duplicate of this one.
And this shouldn't be postponed any more?
Comment #6
jhodgdonSorry, that is wrong -- that select.inc is in the database issue, not this one, whoops!
Comment #7
jhodgdontagging.
Also xjm do you plan to do this one or should we unassign?
Comment #8
xjmYep, just waiting on #1317620: Clean up API docs for includes directory, files starting with D-G and then I will do H-M, and then this one, and then Taxonomy.
Comment #8.0
xjmUpdated issue summary.
Comment #9
jhodgdonI'm going through these issues and un-assigning any without activity for several weeks. If you still want to work on this issue, please feel free to assign it back to yourself. Thanks!
Comment #10
mjonesdinero CreditAttribution: mjonesdinero commentedwill work on this issue and hopefully attached the patch Maximum -- June 25..thanks
Comment #11
mjonesdinero CreditAttribution: mjonesdinero commentednot so long, since have time to do it now
will wait for feedback if accepted
Comment #13
mjonesdinero CreditAttribution: mjonesdinero commentedanother attempt, it is okie with my test on my localhost, hope the testbot is okie
Comment #15
mjonesdinero CreditAttribution: mjonesdinero commentedi dont know why the test bot is making my patch error will work with this again..
in my local it is working
Comment #16
mjonesdinero CreditAttribution: mjonesdinero commentedif someone could tell me why this is happening thanks in advance
Comment #17
disasm CreditAttribution: disasm commentedIt's failing to apply because it's searching for some text that doesn't exist anymore in theme.inc. Did you merge with latest 8.x before generating the patch?
Checking patch core/includes/theme.inc...
error: while searching for:
* executed (if they exist), in the following order (note that in the following
* list, HOOK indicates the theme hook name, MODULE indicates a module name,
* THEME indicates a theme name, and ENGINE indicates a theme engine name):
* - template_preprocess(&$variables, $hook): Creates a default set of variables
* for all theme hooks.
* - template_preprocess_HOOK(&$variables): Should be implemented by
* the module that registers the theme hook, to set up default variables.
* - MODULE_preprocess(&$variables, $hook): hook_preprocess() is invoked on all
* implementing modules.
* - MODULE_preprocess_HOOK(&$variables): hook_preprocess_HOOK() is invoked on
* all implementing modules, so that modules that didn't define the theme hook
* can alter the variables.
* - ENGINE_engine_preprocess(&$variables, $hook): Allows the theme engine to
* set necessary variables for all theme hooks.
* - ENGINE_engine_preprocess_HOOK(&$variables): Allows the theme engine to set
error: patch failed: core/includes/theme.inc:760
error: core/includes/theme.inc: patch does not apply
I'm attempting a re-roll (went all the way back to April, and it's still failing to apply). If I get this to re-roll, I'll post.
Comment #18
disasm CreditAttribution: disasm commentedreroll
Comment #19
mjonesdinero CreditAttribution: mjonesdinero commentedhi disasm
Thanks for the re-roll, yup i have notice it last nyt, but since you have do it for me thanks..
but i am still assigning this to me, for further feedbacks from @jhodgdon
Comment #20
mjonesdinero CreditAttribution: mjonesdinero commentedComment #21
jhodgdon#18: n-z-clean-up-1317628-18.patch queued for re-testing.
Comment #25
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll.
Comment #26
jhodgdon#25: n-z-clean-up-1317628-23.patch queued for re-testing.
Comment #28
jhodgdonLooks like this one needs a reroll again, sorry for the delay in reviewing! I'm trying to get through these...
Comment #29
Gaelan CreditAttribution: Gaelan commentedReroll.
Comment #30
jhodgdonThis patch is going to have to wait for full review/commit until #1168246: Freedom For Fieldsets! Long Live The DETAILS. (a large patch tagged with 'avoid commit conflicts') is taken care of.
Comment #31
jhodgdon#29: n-z-clean-up-1317628-29.patch queued for re-testing.
Comment #32
jhodgdon(retesting because that patch went in)
Comment #34
Albert Volkman CreditAttribution: Albert Volkman commentedRe-rolled to handle password.inc move.
Comment #35
jhodgdonThanks for all the re-rolls! I finally gave this patch a thorough review. It looks mostly good; just needs a bit of further clean-up:
a) schema.inc
The next line should be moved up to the end of the rewrapped text.
b)
This doesn't make any sense to me. How do you handle a function? I think these are functions for handling user sessions, so the previous @file was better. We don't require @file lines to start with a verb. Let's just leave this one as it was.
c) theme.inc not sure which function:
Needs one-sentence summary. Move next sentence to new paragraph.
This one too:
And this one a bit farther down:
Maybe the rest of theme.inc should be checked for this? I only looked at the patch itself, so only lines that were already being changed. There were a couple more places like this in the patch, and there might be more in the file too?
theme.maintenance.inc has the same problem too, and unicode.inc, and update.inc.
d) theme.inc still
probably that last "build" should be "builds"?
e) I think some of the re-wrapping that was done by this patch wasn't really necessary, but I guess it can be left in the patch. There were a bunch of lines of exactly 80 characters that were rewrapped to be less than 80... they could be left alone and the patch would be a *lot* smaller!
f)
This @file change is also wrong. See (b). And this one:
g) token.inc
This change incorrectly altered the meaning of the documentation.
h) unicode.inc
The original doc was saying that this is the complement function to mime_header_encode(). The new doc doesn't say that. Probably the function doc should actually instead say what the function does and have an @see pointing to the other function.
Similarly, this was not a good change:
And this one:
And this one in update.inc:
And there are several other "Helper" -> "Helps" changes that need attention.
Comment #36
Albert Volkman CreditAttribution: Albert Volkman commentedThis should be all of the issues except for the helper items.
Comment #37
jhodgdonOn the "helper" functions, the function doc should say what the function does. Then probably have an @see to the main function, or a separate line saying "Helper function for xyz()."
Comment #39
Albert Volkman CreditAttribution: Albert Volkman commentedChristmas vacation patchin' :)
Comment #40
jhodgdonThanks for the new patch!
I started at the top and gave this a thorough review. It all looks good, except:
a)
I realize we have a standard that we should start with a verb, but this change has lost information that I think was useful. Either revert this change or put "forcefully" back into the line somewhere.
b) There are a couple of functions at the end of theme.inc that don't remotely have one-line verb-starting descriptions (this patch touches them but doesn't fix that problem): template_preprocess_maintenance_page(), template_process_maintenance_page
and the next one, template_preprocess_region(), still has the wrong verb tense.
c) I don't think this change is at all necessary:
d) typo (arrayof is one word in last line):
e) same file, typo (being vs. begin):
f) Unicode should be capitalized:
g) unicode.inc
The param says it is encoding instead of decoding.
h) still unicode.inc
This description doesn't seem right. Why would you need to decode data that wasn't encoded?!? Also, the return value is not an entire header, just a piece of data, right? I also think this should follow our standards for callbacks:
http://drupal.org/node/1354#callbacks
i) still unicode.inc
That @return description doesn't describe the return value.
j) unicode.inc
Characters don't have "amounts", they have "numbers" as they are countable. This terminology appears several times in unicode.inc.
k) update.inc
That is not a good description. Should probably be "Installs a new ...".
l) update.inc
- store -> stores
- And what is "finished page" anyway? Maybe needs to say 'the "finished" page" or 'the results page'?
m) update.inc
store -> stores
n) update.inc
- First line needs to end in .
- In the next paragraph, either provide a subject or keep the same verb tense as first line.
Comment #41
Albert Volkman CreditAttribution: Albert Volkman commentedGot 'em.
Comment #42
jhodgdonLooks good -- I'll get it committed -- thanks!
Comment #43
jhodgdonWhew! Committed to 8.x. Let's see how much of this we can get ported to 7.x. Thanks all!
Comment #44
jhodgdonComment #45
Albert Volkman CreditAttribution: Albert Volkman commentedFirst pass at a D7 backport.
Comment #46
jhodgdonLooks great! Committed to 7.x -- another one of these issues is DONE!
Comment #47.0
(not verified) CreditAttribution: commentedUpdated issue summary.