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.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Comment | File | Size | Author |
---|---|---|---|
#70 | statistics-cleanup-1313510-70-D7.patch | 11.15 KB | NROTC_Webmaster |
#70 | statistics-interdiff.txt | 3.28 KB | NROTC_Webmaster |
#68 | statistics-cleanup-1313510-68-D7.patch | 11.15 KB | NROTC_Webmaster |
#63 | statistics-whitespace-cleanup-63.patch | 1.6 KB | NROTC_Webmaster |
#59 | statistics-cleanup-1313510-59.patch | 11.72 KB | NROTC_Webmaster |
Comments
Comment #1
aenw CreditAttribution: aenw commentedUpdated / improved the documentation for the statistics module. (It was in pretty good shape to begin with.)
Comment #3
aenw CreditAttribution: aenw commentedUploading patch again, after learning that patches for Drupal core must be created relative to the drupal base directory. (duh).
Comment #4
aenw CreditAttribution: aenw commentedComment #5
aenw CreditAttribution: aenw commentedsigh. uploaded wrong file.
Comment #6
aenw CreditAttribution: aenw commenteduploading patch file again
Comment #7
jhodgdonThanks!
A few things need to be fixed:
a) We have standards for how to document form builder functions:
http://drupal.org/node/1354#forms
b) I guess we haven't really adopted a standard for how to indicate that something is a menu callback. :(
But we don't ever want to have two sentences on the first line of a function. So these things aren't really OK:
(Also, this particular one has an extra space at the beginning of the line.)
I think it would be better to say
Menu callback: displays recent page accesses.
This is close to what is already in core mostly, with a grammar/style fix.
c)
Nitpick: Should be Returns an associative array with the schema information.
d)
Nitpick: info -> information
This also applies elsewhere in the file -- max -> maximum, and things like that.
e)
Nitpick: If you're cleaning this up, you could bring the list up to our list standards too:
http://drupal.org/node/1354#lists
f)
This is not correct. We are documenting *types*, not values, so the "| 35" should not be there. And the next line should end with ".". This comment also applies elsewhere in the file. And by the way, if you are indicating different @param types, the | does not have spaces around it.
g)
Create -> Creates -- check the rest of your functions, there are a few others with this kind of problem too.
h)
Nope... We don't put anything but the data type here. Just
@return array
i)
I think this should either be left as statistics.module (which is fine, and actually good because it should turn into a link on api.drupal.org I think), or "Statistics module" (capitalized). I prefer the former. Applies throughout the file.
Comment #8
aenw CreditAttribution: aenw commentedThanks for the great feedback. :-) I'll get these incorporated.
Re: menu callbacks -- Starting the summary line with "Menu callback:" is really helpful for developers IMO. If I'm looking at a list of functions on api.drupal.org, having "Menu callback:" is a really informative clue to see in the summary line for a function. But this does deviate from the standards that stress starting a summary with a verb.
If this becomes the standard, I think it'll be important to point out this deviation in the standards docs. As someone coming up the learning curve on the documentation standards (trying to really grok them all, trying to move past constantly reading and re-reading the standards page), this would be helpful point. I went whole-hog with "All summaries must start with a verb!!!" Knowing that there are exceptions (or at least potentially this one) would be a key piece of info when trying to learn all the standards.
(Perhaps I should put that feedback into the docs issue queue? Let me know.)
Meanwhile, I'll get these changes into the files for the Statistics module and submit again.
Comment #9
jhodgdonI just filed:
#1315992: No standard for documenting menu callbacks
for the menu callbacks. Perhaps wait until we get that sorted out, and comment there with your thoughts?
Comment #10
jhodgdonAlso I liked your suggestion and just added a note to http://drupal.org/node/1354#functions that says "note, there are exceptions" essentially. :)
Comment #11
aenw CreditAttribution: aenw commentedI like the note about functions. I'm sure others will find it helpful, too.
Re: #1315992: No standard for documenting menu callbacks -- it'll be good to get that sorted out. (One more thing sorted out = good!). For now, I just went with the
'[callback type] callback: [function desc]"
format. Assuming I've got everything else right, I guess you get to decide what to do for now: keep this at 'needs work' and then I can come back and finish it after the callback standard is decided, or go ahead with what we have now and move this issue along to RBTC, then make another issue (if needed) to go back and fix the documentation for callback functions. (Either is fine with me, and I don't mind getting any follow-up issue assigned to me -- it'll be a quick thing to do.)So, with that said, I present: the next patch iteration!
Comment #12
jhodgdonI think we're close to adopting a standard there, so I'll review then...
Comment #13
jhodgdonWe have that standard now on http://drupal.org/node/1354#menu-callback -- looks like you adopted the right one. :) So now reviewing your patch... Overall, looks pretty close, and your effort is appreciated! (By the way, I would normally leave an issue assigned to myself if my intention was to shepherd it through until it is committed. I hope you'll make a re-patch...)
A few things to clean up:
a)
What is this? If it's a function, use () after it. If it's a variable, say so (it probably is, and doesn't need to mention the settings form in the next sentence I think? This is API doc, not user doc.) Also, I am not understanding from reading this sentence how a time interval is associated with these reports in the first place?
b)
Normally, we don't include "Returns an..." in a @return section, but just start with "An associative array...".
c) The sentences like this:
"This is a menu callback to display the top visitors."
are probably not necessary. The first line summary already contains this information.
d) Form constructors do not need @return docs. On the other hand, they should have @param docs if there are any params beyond form and form_state.
e) Hook implementations such as statistics_schema() and statistics_help() should not include the @return. This is documented on the hook definition only. see http://drupal.org/node/1354#hookimpl
f)
This is generally a good cleanup. I'm not sure each line needs "A string for showing"... couldn't be just
@param string $dbfield
Which type of report should be generated. One of:
- 'totalcount': The top viewed content of all time.
(etc.)
Also 'totalcount' and the others should be in 'quotes', I think, because they are the actual strings that need to be supplied here as the argument? We don't use quotes for array keys, but I think we should here?
g)
This is missing the param variable name $width
h)
The only thing after @return should be a data type. I think this should be string, not link?
i)
Needs "the": truncating the width...
j)
I think this should be "for testing statistics.module", as that will make a link to the module page on api.drupal.org. Also, I think we prefer "Defines a base class" to "Sets up a ..."? http://drupal.org/node/1354#classes
k)
The preferred format here is a one-line saying "Overrides DrupalWebTestCase::setUp()." only. See classes page referenced in (j) above. Same for the getInfo() overrides later in the file.
l)
The @var line should only contain a data type. Put other information in regular text.
Comment #14
aenw CreditAttribution: aenw commentedThanks for the feedback -- again!
I changed the "assigned to" because I was extrapolating from the process with the meta issue for this. Now I know, though.
I'll get back to this tomorrow.
Comment #15
aenw CreditAttribution: aenw commentednew patch with corrections
Comment #16
xjmHi @aenw, I reviewed #15 based on @jhodgdon's remarks in #13 and noticed a few minor things:
This sentence is missing a verb ("This set on..." should be "This is set on..."), but actually, I think jhodgdon suggested above that we can just remove this sentence.
I had to read these a couple times before I understood. Maybe "A renderable array containing..."? Also, for the last two, you can exclude "Returns $build."
See this comment Lars Toomre made: #1315886-42: Clean up API docs for includes directory, files starting with A-C. For my issue, I am deferring this cleanup to a followup, but I thought I'd point it out here. Edit: Just saw this above:
So we have two different possibilities. Personally I think what jhodgdon says makes sense; if a specific string is needed as the array key, I'd understand it better if it were in single quotes. If the key is numeric or just a descriptor of what the key's value is, then it would make sense not to use quotes. Not sure though and we clearly have it both ways currently. :)
Missing periods.
This is a little awkward... Maybe just "A page (node) for which to test access statistics"? I think that covers both clauses.
Also, just a general question: From #711918: Documentation standard for @param and @return data types, I thought we were postponing adding the datatype documentation, because otherwise it would be scattered in some parts of the docs and not others. Perhaps it's more reasonable now that we are not backporting any of the cleanup to D7, but should those changes be postponed so that they're done together and the docs are consistent?
Comment #17
xjmOh, one other little thing... maybe we could put single quotes around 'statistics_flush_accesslog_timer'? That makes it a little more clear.
Comment #18
Lars Toomre CreditAttribution: Lars Toomre commentedRegarding @xjm's comment in #16 about list formatting, I have just created #1326574: Correct list stuff in docs for includes/database with an attached patch. That patch contains a correction for the malformed list in the statistics module as well as approximately 35 other core files where list formatting does not conform to core documentation standards.
Depending upon which patch gets committed first, this patch may need to be re-rolled. I am happy to re-roll mine to keep up as the documentation sprint patches are committed.
Comment #19
jhodgdonGood idea - let's do the list formatting on that other issue instead of in these patches.
Comment #20
xjmNote that this patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Comment #21
aenw CreditAttribution: aenw commentedI'm back to this after a little delay.
Onward!
Comment #22
xjmRe: Renderable arrays. The reason I made this suggestion is that "renderable array" is a Drupalism for a specific thing. "This array can be rendered" makes me go "eh?", so I wanted to make it clearer that these are, in fact, the massive associative arrays used with the Render API. However, while the handbook doc at http://drupal.org/node/930760 says:
...it seems to prefer "Render Array" in most of the document (and in the page title).
So how about we simplify it to: "A render array containing..." ?
Maybe we also want an @see to some Render API reference.
Comment #23
jhodgdonI am with xjm on the render array thing. It's a definite drupalism to call it a render array, and we shouldn't change all the docs to say "an array that can be rendered".
Regarding rerolling patches for the new directory structure going into the meta-issue... It is not necessary for new issues, since people would start with a git repository with the right structure -- only affects people who started out with the previous structure. So I don't think it needs to be there as a task, but if you want to add it and think it's important, you can edit the issue summary and add the text that you think should be there.
Comment #24
Lars Toomre CreditAttribution: Lars Toomre commented@anew regarding #21.3 - Please feel free to adjust the issue summary in #1331240: [Meta] Correct documentation for 'list' related issues as you see fit.
I hope to roll a patch this weekend containing list formatting changes for all of the modules that are completed or nearing commpletion in the docs API documentation sprint.
I will make sure to add 'statistics' changes in there. Are 'tracker' and 'entity' also pretty far along, or will there be many more changes to come in those sub-issues?
(It helps me to not have too much of a moving target that I am trying to wrap around. I am taking on the responsibility to make only incremental changes that are in addition to what the main doc sprint effort is completing.)
Comment #25
aenw CreditAttribution: aenw commented@Lars #24: I still have some changes for entity and tracker. I can take on the list issues for those and make the patches for lists for those. HTH keeping your scope clear and un-creepy as possible. :-)
@Lars #23.1: Will do.
@xjm and @jhodgdon #22: Okey-doke. "A render array containing..." it is.
Comment #26
Lars Toomre CreditAttribution: Lars Toomre commented@anew #25: Thanks for the update. I will include the entity and tracker updates from my mega patch for one of the later of about six forthcoming smaller formatting patches.
For now, I will keep the list updates for entity and tracker in the mega changes with the understanding that I will defer first to what you come up. My concern is that we catch all of the list stuff and there is alot of it.
Let me know if I can help you in any way. Cheers!
Comment #27
aenw CreditAttribution: aenw commentedLatest revisions.
statistics_flush_accesslog_time
to try to keep it in line with the standards for variables in lists (it's not a string). (I do think it's clear that it's a variable since it has the underscores in there.) I did try to make the wording a little clearer.Comment #28
aenw CreditAttribution: aenw commentedComment #29
Lars Toomre CreditAttribution: Lars Toomre commented@anew - I have been waiting for this to go RBTC before rolling lists wrap around patch. Happy to do when this ready.
Comment #30
xjmThanks @aenw! One thing; we need to drop the docblocks on
setUp()
andgetInfo()
for now, until there is consensus about it. See #338403: Use {@inheritdoc} on all class methods (including tests) and #1339054: DrupalTestCaseInterface to properly require getInfo().Also, I wonder if we want to do the hybrid form constructor/page callback docblock thing. (See the latest patch in #1313980: Clean up API docs for node module.) In most cases the form constructor is actually a callback for
drupal_get_form()
, which in turn is the page callback.Comment #31
xjmOh, one other thing I noticed:
Comma splice here. Also, the summary seems incorrect, actually. See: http://api.drupal.org/api/drupal/modules--statistics--statistics.module/...
_statistics_link() does the actual link generating and truncating. This one formats an item for display, including both the item title and the link.
Comment #32
xjmHi @aenw,
Thanks for taking this on. Are you still working on this issue? If not, we'll unassign it in a day or two so that someone else can give it a try. (Feel free to assign it back to yourself if you'd still like to work on it, as well.) Thanks!
Comment #33
xjmPutting it back in the queue.
Comment #34
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI took the patch and comments from before and made the changes listed. Other than that should @see and @ingroup have a space between them or not? It is listed both ways in the documentation but I'm not sure which way is correct.
* @ingroup forms
* @see system_settings_form()
or
* @ingroup forms
*
* @see system_settings_form()
Comment #35
jhodgdonGenerally if there are just a couple @see and @ingroup, they don't need a space between them. If there are a ton of one or the other, they might need a space for clarity. It's kind of a "use your own judgement" thing.
Meanwhile, sorry this escaped review... I'll hit retest and then we can see about getting it reviewed/committed. Thanks for the patch!
Comment #36
jhodgdon#34: statistics-clean-up-documentation-1313510-34.patch queued for re-testing.
Comment #38
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHere is an updated version of the patch, although I'm not sure that is why it failed before.
Comment #39
jhodgdonThe previous failure was due to a glitch in git yesterday. Hopefully it will not fail today. :)
Comment #40
xjmWe decided to take these out. For the gory details, see #1315992: No standard for documenting menu callbacks.
Comment #41
xjmOh, a few more things. I notice some data types being added with
@param/@return
in this patch; I think we were avoiding that in the cleanup sprint patches to make the patches easier to review.Plus a couple other observations:
This is kind of weird. Are they integer keys, or string keys? Is the order relevant? If not let's get change this to "An associative array with the following keys:" or something.
How about "for the Statistics module"? I think that's what we're using in most cases.
Let's add a comma before "or" for clarity.
I don't think the patch making node entities classed is in yet, so this should just be
object
(and it would need to be capitalized otherwise, I think).Comment #42
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHere is the latest version.
Comment #43
jhodgdonThis is mostly great, thanks! Just a couple of things to address:
a) statistics.admin.inc
Needs @param.
b) statistics.module
A couple of things here:
- In lists of actual literal strings to use for parameters, I would leave in the ''
- I think the $dbfield needs to say something like ... hmmm... what is it actually used for? This needs to be clarified. The name implies it's a database field, but the doc doesn't even mention that or say what it does in the function.
- $dbrows param doc needs to be brought up to formatting standards.
- In the @return, I think a comma before "or" would help.
c) statistics.pages.inc - Page callbacks should have @see statistics_menu() in them.
d) statistics.test
I think if something is a PHP stdobj, @var object should be lower-case not Object. That would make it consistent with
http://drupal.org/node/1354#param-return-data-type
Comment #44
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHopefully this is what you were looking for with $dbfield.
Comment #45
jhodgdonYes, much better! I noticed one small thing in the dbfield area:
show -> shows
Also on (c) above, I think statistics_user_tracker() still needs an @see to statistics_menu().
And there is one other thing in statistics.test that I didn't notice last time:
We shouldn't refer to a $user object. $user refers to the global variable $user, so here we should just say "a fully-loaded user object".
Other than that I think it's ready to go!
Comment #46
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI'll get those taken care of but I'm not sure what you noticed in the test.
Comment #47
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedUnless you are talking about the spacing for the @var description
Comment #48
jhodgdonOh, sorry about that. I had a missing end tag > and my comment about the test didn't display. It's there now in comment #45.
Comment #49
oriol_e9gComment #50
xjmThis patch looks great overall. I read the new docs and I have a few additional suggestions:
For the last bit I'd say "drupal_not_found() is called instead" (as the return value itself is not calling anything).
I think we prefer the final serial comma here (after "update").
I'd rephrase this slightly:
Returns the most viewed content for all time, today, or the last-viewed node.
(I think that fits 80 chars; please confirm.)
I think the single quotes around the values should be removed here, as per our list formatting standards: http://drupal.org/node/1354#lists
The word "to" here should be removed.
I'd say "permission to administer and access statistics" for clarity.
"Fully loaded" need not be hyphenated (as "fully" is already an adverb).
The two halves of this description are not grammatically parallel. I'd suggest:
A page node for which to check access statistics.
(I think that covers it.) :)
I also applied the patch locally and looked for anything that was missed. The only thing I noticed is that there's a missing blank line after
statistics_update_index()
instatistics.module
.Once again, an interdiff showing the changes would be good. Thanks!
Comment #51
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedIn regards to #4 jhodgdon recommended to leave the ' s in #1313510-43: Clean up API docs for statistics module part b)
Just let me know which was is preferred and I will make the changes.
Comment #52
jhodgdonOur list standards say to omit the quotes for array keys. But as noted above, these are literal strings, not array keys, so I think they should have quotes.
Comment #53
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedFor #3 it was 1 character over so it changed it to
Returns the most viewed content of all time, today, or the last-viewed node.
Comment #54
jhodgdonI'm not sure we should be talking about "persistent variables" in Drupal 8 docs. Isn't the config management initiative getting rid of variables per se? All of the new docblocks in statistics.admin.inc have things like:
I think that information is likely to be wrong now or very soon... right?
I think for all of these, we should just say something like "the site-wide statistics time interval setting" and leave it at that, because that is generic enough that it doesn't depend on variable_get() or a specific variable name. The code below would tell programmers more about the details, if they needed it.
Comment #55
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedLet me know how this looks.
Comment #56
jhodgdonI'm not too excited about this wording either...
I took a look at statistics_settings_form() and the name of the variable/setting as it is now. This really isn't an "interval" setting, but a "flush" setting -- you're setting up the amount of time page access statistics are kept before flushing, and even then I think you would be subject to when cron runs, right? Also, I'm not sure whether that information about the interval really applies well to the "recent hits" page, since that page just shows you the 30 most recent hits (if there have been 30 of them that aren't yet flushed).
So I think the wording may need to be different for the different statistics report pages, and I would be happier if it used the word "flush", didn't call it an "interval", said something about the reports including all page access records that haven't been flushed yet, and was tailored to the different reports.
Comment #57
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI think I finally have something worth reviewing. It is a little generic but I think it does a better job of explaining the time interval (which I stuck with because that is how it is listed in the code) for flushing.
Let me know if you think it should be more specific for each function.
Comment #58
xjmI think it's probably better to not use quotes here, so that there is just one standard people need to follow for list formatting. Edit: Sorry, I did not realize I said this already in a previous review, and @jhodgdon says in #52 to disregard.
The words "the" and "and" are missing from the list of fields. It would probably also better to describe the values rather than listing database field names:
Regarding the new docblocks, it's a little weird and confusing to have "The site-wide statistics show..." repeated each time. I think the second sentence is a bit redundant with the first in each case.
Comment #59
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedComment #60
jhodgdonThis looks good, and it doesn't look like any of the critical/majors are touching these areas, so I committed it to 8.x. Thanks to all who contributed to this patch! :)
Backport to 7.x comes next...
Comment #61
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedjhodgdon,
I hang my head in shame while I admit that the last patch had end of line whitespace on several of the lines. Did you remove it before committing or do I need to make a new patch to remove them.
Comment #62
jhodgdonUh oh, I didn't remove them...
Comment #63
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHere is the correction.
Comment #64
jhodgdonThanks! That's in 8.x, on to 7.x for real now. :)
(Note to self: check whitespace when committing.)
Comment #65
xjmThis cleanup is fine. (The textual improvements in the committed patch look good to me also.)
Comment #66
jhodgdonUm. Already done. :)
Comment #67
xjmSorry, xposted. :)
Comment #68
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedSince the documentation doesn't specify what the D7 standard was, it simply states
I'm not exactly sure what to do in those cases but I tried to leave it as close to what it was before but added the details equivalent to the D8 version.
Here is a first attempt.
Comment #69
jhodgdonThe items that are standards for Drupal 8+ did not have any standards for D7 and before, which is to say that nothing can be enforced standards-wise.
Anyway... what we've been doing:
a) Things like
I think we are going ahead and changing this to Page callback: but leaving out the @see to the hook_menu() function.
b) param/return data types - leave out (looks like you did that mostly, and really I'm not going to say to remove them when it's useful...).
Were there questions other than these? I think the patch (at first glance anyway) looks good except for (a), but I didn't look at it extremely carefully yet.
Comment #70
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedThanks for the clarification. With that here is an updated version that hopefully meets the requirements.
Comment #71
jhodgdonSorry for the delay! This looks good to me, and as its scope is limited (not much going on in statistics.module and this month-old patch still applied), I went ahead and committed it. Thanks to all who worked on this issue!