CommentFileSizeAuthor
#70 statistics-cleanup-1313510-70-D7.patch11.15 KBNROTC_Webmaster
#70 statistics-interdiff.txt3.28 KBNROTC_Webmaster
#68 statistics-cleanup-1313510-68-D7.patch11.15 KBNROTC_Webmaster
#63 statistics-whitespace-cleanup-63.patch1.6 KBNROTC_Webmaster
#59 statistics-cleanup-1313510-59.patch11.72 KBNROTC_Webmaster
#59 statistics-interdiff.txt7.57 KBNROTC_Webmaster
#57 statistics-cleanup-1313510-57.patch11.89 KBNROTC_Webmaster
#57 statistics-interdiff.txt5.07 KBNROTC_Webmaster
#55 statistics-cleanup-1313510-55.patch11.55 KBNROTC_Webmaster
#53 statistics-cleanup-1313510-53.patch11.81 KBNROTC_Webmaster
#53 statistics-interdiff.txt4.36 KBNROTC_Webmaster
#49 statistics-cleanup-1313510-49.patch11.57 KBoriol_e9g
#49 interdif.txt865 bytesoriol_e9g
#44 statistics-clean-up-documentation-1313510-44.patch11.54 KBNROTC_Webmaster
#44 statistics-interdiff.txt5.67 KBNROTC_Webmaster
#42 statistics-clean-up-documentation-1313510-42.patch11.38 KBNROTC_Webmaster
#42 statistics-interdiff.txt11.83 KBNROTC_Webmaster
#38 statistics-clean-up-documentation-1313510-38.patch10.23 KBNROTC_Webmaster
#34 statistics-clean-up-documentation-1313510-34.patch10.59 KBNROTC_Webmaster
#34 statistics-interdiff.txt5.42 KBNROTC_Webmaster
#27 statistics-clean-up-documentation-1313510-27.patch12.21 KBaenw
#15 statistics-clean-up-documentation-1313510-15.patch12.26 KBaenw
#11 statistics-clean-up-documentation-1313510-11.patch13.41 KBaenw
#6 statistics-clean-up-documentation-1313510-6.patch11.91 KBaenw
#3 tracker-clean-up-documentation-1315214.patch11.37 KBaenw
#1 clean-up-documentation-1313510.patch11.56 KBaenw
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aenw’s picture

Assigned: aenw » Unassigned
Status: Active » Needs review
FileSize
11.56 KB

Updated / improved the documentation for the statistics module. (It was in pretty good shape to begin with.)

Status: Needs review » Needs work

The last submitted patch, clean-up-documentation-1313510.patch, failed testing.

aenw’s picture

Uploading patch again, after learning that patches for Drupal core must be created relative to the drupal base directory. (duh).

aenw’s picture

Status: Needs work » Needs review
aenw’s picture

Status: Needs review » Needs work

sigh. uploaded wrong file.

aenw’s picture

Status: Needs work » Needs review
FileSize
11.91 KB

uploading patch file again

jhodgdon’s picture

Status: Needs review » Needs work

Thanks!

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:

- * Menu callback; Displays recent page accesses.
+  * Displays recent page accesses. This is a menu callback.

(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)

+ *   Returns the associative array with the schema information.

Nitpick: Should be Returns an associative array with the schema information.

d)

+ *   Returns HTML with help info.

Nitpick: info -> information
This also applies elsewhere in the file -- max -> maximum, and things like that.

e)

- * @param $dbfield
+ * @param string $dbfield
  *   one of
  *   - 'totalcount': top viewed content of all time.
  *   - 'daycount': top viewed content for today.
  *   - 'timestamp': last viewed node.

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)

+ * @param int | 35
+ *   The width to set the displayed text of the path

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 a link for text to a path, truncating width of the text if needed.

Create -> Creates -- check the rest of your functions, there are a few others with this kind of problem too.

h)

+ * @return array $build | drupal_not_found()

Nope... We don't put anything but the data type here. Just @return array

i)

 /**
  * @file
- * Tests for statistics.module.
+ * Tests for the statistics module.
  */

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.

aenw’s picture

Assigned: Unassigned » aenw

Thanks 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.

jhodgdon’s picture

I 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?

jhodgdon’s picture

Also I liked your suggestion and just added a note to http://drupal.org/node/1354#functions that says "note, there are exceptions" essentially. :)

aenw’s picture

Assigned: aenw » Unassigned
Status: Needs work » Needs review
FileSize
13.41 KB

I 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!

jhodgdon’s picture

I think we're close to adopting a standard there, so I'll review then...

jhodgdon’s picture

Status: Needs review » Needs work

We 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)

The time interval used is the statistics_flush_accesslog_timer.

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)

+ * @return array
+ *   Returns an associative array...

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)

+ * @param string $dbfield
+ *   One of:
+ *   - totalcount: A string for showing the top viewed content of all time.
+ *   - daycount: A string for showing the top viewed content for today.
+ *   - timestamp: A string for showing only the last viewed node.

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)

+ * @param int

This is missing the param variable name $width

h)

+ * @return link
+ *   Returns the text truncated to the width, linked to the given $path.

The only thing after @return should be a data type. I think this should be string, not link?

i)

+ * Creates a link for text to a path, truncating width of the text if needed.

Needs "the": truncating the width...

j)

 /**
- * Sets up a base class for the Statistics module.
+ * Sets up a base class for testing the statistics module.
  */
 class StatisticsTestCase extends DrupalWebTestCase {

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)

+  /**
+   * Sets up the test class; initializes data needed for testing.
+   *
+   * @see DrupalWebTestCase::setUp()
+   */
   function setUp() {

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)

+   * @var A fully-loaded $user object or FALSE if user creation failed.

The @var line should only contain a data type. Put other information in regular text.

aenw’s picture

Assigned: Unassigned » aenw

Thanks 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.

aenw’s picture

Status: Needs work » Needs review
FileSize
12.26 KB

new patch with corrections

xjm’s picture

Hi @aenw, I reviewed #15 based on @jhodgdon's remarks in #13 and noticed a few minor things:

+++ b/modules/statistics/statistics.admin.incundefined
@@ -6,7 +6,17 @@
+ * This set on the statistics settings form.

@@ -45,7 +55,17 @@ function statistics_recent_hits() {
+ * statistics_flush_accesslog_timer. This set on the statistics settings form.

@@ -90,7 +110,17 @@ function statistics_top_pages() {
+ * This is set on the statistics settings form.

@@ -143,7 +173,17 @@ function statistics_top_visitors() {
+ * statistics_flush_accesslog_timer. This set on the statistics settings form.

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.

+++ b/modules/statistics/statistics.admin.incundefined
@@ -6,7 +6,17 @@
+ * @return array
+ *   An associative array that can be rendered with information about the most
+ *   recent hits.

@@ -45,7 +55,17 @@ function statistics_recent_hits() {
+ * @return array
+ *   An associative array with the top pages information; this array can be
+ *   rendered.

@@ -90,7 +110,17 @@ function statistics_top_pages() {
+ * @return array
+ *   An associative array with the top visitors information. This array can be
+ *   rendered.

@@ -143,7 +173,17 @@ function statistics_top_visitors() {
+ * @return array
+ *   An associative array with the top referrers information. This array can be
+ *   rendered.

@@ -189,7 +229,13 @@ function statistics_top_referrers() {
+ * @return array
+ *   An associative array with page access statistics that can be rendered. If
+ *   information for the page was not found, calls drupal_not_found().

+++ b/modules/statistics/statistics.pages.incundefined
@@ -2,9 +2,19 @@
+ * @return array
+ *   Returns $build, an associative array containing node statistics. This
+ *   associative array can be rendered. If information for the node was not
+ *   found, this will deliver a page not found error via drupal_not_found().

@@ -52,6 +62,16 @@ function statistics_node_tracker() {
+ * @return array
+ *   Returns $build, an associative array containing user statistics. This
+ *   associative array can be rendered. If information for the user was not
+ *   found, this will deliver a page not found error via drupal_not_found().

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."

+++ b/modules/statistics/statistics.moduleundefined
@@ -246,20 +246,19 @@ function statistics_cron() {
+ * @param string $dbfield
+ *   One of:
+ *   - 'totalcount': Show the top viewed content of all time.
+ *   - 'daycount': Show the top viewed content for today.
+ *   - 'timestamp': Show only the last viewed node.

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:

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?

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. :)

+++ b/modules/statistics/statistics.moduleundefined
@@ -371,8 +370,15 @@ function statistics_block_view($delta = '') {
+ *   The path to generate the link for
...
+ *   The width to set the displayed text of the path

Missing periods.

+++ b/modules/statistics/statistics.testundefined
@@ -260,12 +279,29 @@ class StatisticsBlockVisitorsTestCase extends StatisticsTestCase {
+   * A page (node) to access and then verify the access statistics for it.

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?

xjm’s picture

Status: Needs review » Needs work
+++ b/modules/statistics/statistics.admin.incundefined
@@ -6,7 +6,17 @@
+ * interval used is the persistent variable statistics_flush_accesslog_timer.

Oh, one other little thing... maybe we could put single quotes around 'statistics_flush_accesslog_timer'? That makes it a little more clear.

Lars Toomre’s picture

Regarding @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.

jhodgdon’s picture

Good idea - let's do the list formatting on that other issue instead of in these patches.

xjm’s picture

Note 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.

aenw’s picture

I'm back to this after a little delay.

  1. Yes, I'm guilty of doing the @param and @return information. You're absolutely right -- we should postpone that so that the reviews and work are contained. I'm also working on the tracker and entity modules and will make sure to do better with those.
  2. Ditto with list formatting. (Thanks for making the issue, Lars.)
  3. Putting single quotes around variables: The standard for that definitely needs to be worked out. Glad that it's started with #1331240: [Meta] Correct documentation for 'list' related issues. Should a note about that also be put into the text for the meta issue? I agree about putting single quotes around 'totalcount' and the others since they are explicitly strings.
  4. @xjm Re: "A renderable array containing..." I just have a different opinion. :-) I find "A renderable array..." really awkward and "This associative array can be rendered" much smoother and nice and concise. My beef is with the word "renderable," which I find really ungainly. But I'm certainly open to changing it. Other opinions? Shall we flip a coin? (If you flip a coin and tell me I lose, then I'm fine with changing it. :-) )
  5. Rerolling due to a different directory structure: Glad that was pointed that out, else I wouldn't have known. (I'm working just on the info in the meta issue and the documentations standards page.) @jhodgdon, can that be noted in the text for the meta issue (#1310084: [meta] API documentation cleanup sprint)? I think it'd be helpful to put that info above list item #6. (Or insert a new #6 that is essentially about just that.) I'll see if I can re-roll the patch and if not, I'll see if I can get some help there.

Onward!

xjm’s picture

Re: 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:

"Render Arrays" or "Renderable Arrays" are the building blocks of a Drupal page...

...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.

jhodgdon’s picture

I 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.

Lars Toomre’s picture

@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.)

aenw’s picture

@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.

Lars Toomre’s picture

@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!

aenw’s picture

Latest revisions.

  • I didn't change the formatting of the list items; I'll address that in a separate patch as suggested in #19.
  • I didn't put quotes around 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.
  • I did re-roll with the new file structure (/core directory).
aenw’s picture

Status: Needs work » Needs review
Lars Toomre’s picture

@anew - I have been waiting for this to go RBTC before rolling lists wrap around patch. Happy to do when this ready.

xjm’s picture

Status: Needs review » Needs work

Thanks @aenw! One thing; we need to drop the docblocks on setUp() and getInfo() 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.

xjm’s picture

Oh, one other thing I noticed:

+++ b/core/modules/statistics/statistics.moduleundefined
@@ -383,6 +389,17 @@ function _statistics_link($path, $width = 35) {
+ * Creates a link for text to a path, truncates the width of the text if needed.
...
 function _statistics_format_item($title, $path) {

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.

xjm’s picture

Hi @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!

xjm’s picture

Assigned: aenw » Unassigned

Putting it back in the queue.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
5.42 KB
10.59 KB

I 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()

jhodgdon’s picture

Generally 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!

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7, +docs-cleanup-2011

The last submitted patch, statistics-clean-up-documentation-1313510-34.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
10.23 KB

Here is an updated version of the patch, although I'm not sure that is why it failed before.

jhodgdon’s picture

The previous failure was due to a glitch in git yesterday. Hopefully it will not fail today. :)

xjm’s picture

Status: Needs review » Needs work
+++ b/core/modules/statistics/statistics.admin.incundefined
@@ -6,7 +6,17 @@
+ * Callback path: admin/reports/hits

@@ -45,7 +55,17 @@ function statistics_recent_hits() {
+ * Callback path: admin/reports/pages

@@ -90,7 +110,17 @@ function statistics_top_pages() {
+ * Callback path: admin/reports/visitors

@@ -143,7 +173,17 @@ function statistics_top_visitors() {
+ * Callback path: admin/reports/referrers

@@ -189,7 +229,13 @@ function statistics_top_referrers() {
+ * Callback path: admin/reports/access/%

+++ b/core/modules/statistics/statistics.pages.incundefined
@@ -2,9 +2,18 @@
+ * Callback path: node/%node/track

@@ -52,6 +61,15 @@ function statistics_node_tracker() {
+ * Callback path: user/%user/track/navigation

We decided to take these out. For the gory details, see #1315992: No standard for documenting menu callbacks.

xjm’s picture

Oh, 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:

  1. +++ b/core/modules/statistics/statistics.moduleundefined
    @@ -291,11 +290,11 @@ function statistics_title_list($dbfield, $dbrows) {
    + *   An array with three entries: [0]=totalcount, [1]=daycount, [2]=timestamp:
    

    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.

  2. +++ b/core/modules/statistics/statistics.pages.incundefined
    @@ -2,9 +2,18 @@
    + * User page callbacks for statistics.module.
    

    How about "for the Statistics module"? I think that's what we're using in most cases.

  3. +++ b/core/modules/statistics/statistics.testundefined
    @@ -284,10 +284,24 @@ class StatisticsBlockVisitorsTestCase extends StatisticsTestCase {
    +   * A fully-loaded $user object or FALSE if user creation failed
    

    Let's add a comma before "or" for clarity.

  4. +++ b/core/modules/statistics/statistics.testundefined
    @@ -284,10 +284,24 @@ class StatisticsBlockVisitorsTestCase extends StatisticsTestCase {
    +   * @var node
    

    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).

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
11.83 KB
11.38 KB

Here is the latest version.

jhodgdon’s picture

Status: Needs review » Needs work

This is mostly great, thanks! Just a couple of things to address:

a) statistics.admin.inc

@@ -189,7 +222,11 @@ function statistics_top_referrers() {
 }
 
 /**
- * Menu callback; Displays recent page accesses.
+ * Page callback: Gathers page access statistics suitable for rendering.
+ *
+ * @return array
+ *   A render array containing page access statistics. If information for the
+ *   page was not found, calls drupal_not_found().
  */
 function statistics_access_log($aid) {

Needs @param.

b) statistics.module

@@ -249,20 +249,19 @@ function statistics_cron() {
...
+ * @param string $dbfield
+ *   One of:
+ *   - totalcount: Integer that shows the top viewed content of all time.
+ *   - daycount: Integer that show the top viewed content for today.
+ *   - timestamp: Integer that show only the last viewed node.
+ * @param int $dbrows
  *   number of rows to be returned.
  *
- * @return
- *   A query result containing n.nid, n.title, u.uid, u.name of the selected node(s)
- *   or FALSE if the query could not be executed correctly.
+ * @return SelectQuery|FALSE
+ *   A query result containing n.nid, n.title, u.uid, u.name of the selected
+ *   node(s) or FALSE if the query could not be executed correctly.
  */
 function statistics_title_list($dbfield, $dbrows) {

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

@@ -284,10 +284,24 @@ class StatisticsBlockVisitorsTestCase extends StatisticsTestCase {
...
+  /**
+   * A page (node) to access and then verify the access statistics for it.
+   *
+   * @var Object
+   */
   protected $test_node;

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

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
5.67 KB
11.54 KB

Hopefully this is what you were looking for with $dbfield.

jhodgdon’s picture

Status: Needs review » Needs work

Yes, much better! I noticed one small thing in the dbfield area:

+++ b/core/modules/statistics/statistics.module
...
@@ -249,20 +249,19 @@ function statistics_cron() {
...
+ *   - 'timestamp': Integer that show only the last viewed node.

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:

@@ -294,10 +294,24 @@ class StatisticsBlockVisitorsTestCase extends StatisticsTestCase {
 }
 
 /**
- * Test statistics administration screen.
+ * Tests the statistics administration screen.
  */
 class StatisticsAdminTestCase extends DrupalWebTestCase {
+
+  /**
+   * A user that has permissions necessary to administer and access statistics.
+   *
+   * @var object|FALSE
+   *
+   * A fully-loaded $user object, or FALSE if user creation failed.
+   */
   protected $privileged_user;

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!

NROTC_Webmaster’s picture

I'll get those taken care of but I'm not sure what you noticed in the test.

NROTC_Webmaster’s picture

Unless you are talking about the spacing for the @var description

jhodgdon’s picture

Oh, 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.

oriol_e9g’s picture

Status: Needs work » Needs review
FileSize
865 bytes
11.57 KB
xjm’s picture

Status: Needs review » Needs work

This patch looks great overall. I read the new docs and I have a few additional suggestions:

  1. +++ b/core/modules/statistics/statistics.admin.incundefined
    @@ -189,7 +222,14 @@ function statistics_top_referrers() {
    + *   A render array containing page access statistics. If information for the
    + *   page was not found, calls drupal_not_found().
    

    For the last bit I'd say "drupal_not_found() is called instead" (as the return value itself is not calling anything).

  2. +++ b/core/modules/statistics/statistics.installundefined
    @@ -2,7 +2,7 @@
    + * Install, update and uninstall functions for the Statistics module.
    

    I think we prefer the final serial comma here (after "update").

  3. +++ b/core/modules/statistics/statistics.moduleundefined
    @@ -249,20 +249,19 @@ function statistics_cron() {
    + * Returns top viewed content of all time, for today, or the last viewed node.
    

    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.)

  4. +++ b/core/modules/statistics/statistics.moduleundefined
    @@ -249,20 +249,19 @@ function statistics_cron() {
    + *   - 'totalcount': Integer that shows the top viewed content of all time.
    + *   - 'daycount': Integer that shows the top viewed content for today.
    + *   - 'timestamp': Integer that shows only the last viewed node.
    

    I think the single quotes around the values should be removed here, as per our list formatting standards: http://drupal.org/node/1354#lists

  5. +++ b/core/modules/statistics/statistics.pages.incundefined
    @@ -52,6 +62,16 @@ function statistics_node_tracker() {
    + * @see to statistics_menu()
    

    The word "to" here should be removed.

  6. +++ b/core/modules/statistics/statistics.testundefined
    @@ -294,10 +294,24 @@ class StatisticsBlockVisitorsTestCase extends StatisticsTestCase {
    +   * A user that has permissions necessary to administer and access statistics.
    

    I'd say "permission to administer and access statistics" for clarity.

  7. +++ b/core/modules/statistics/statistics.testundefined
    @@ -294,10 +294,24 @@ class StatisticsBlockVisitorsTestCase extends StatisticsTestCase {
    +   * A fully-loaded user object, or FALSE if user creation failed.
    

    "Fully loaded" need not be hyphenated (as "fully" is already an adverb).

  8. +++ b/core/modules/statistics/statistics.testundefined
    @@ -294,10 +294,24 @@ class StatisticsBlockVisitorsTestCase extends StatisticsTestCase {
    +   * A page (node) to access and then verify the access statistics for it.
    

    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() in statistics.module.

Once again, an interdiff showing the changes would be good. Thanks!

NROTC_Webmaster’s picture

In regards to #4 jhodgdon recommended to leave the ' s in #1313510-43: Clean up API docs for statistics module part b)

- In lists of actual literal strings to use for parameters, I would leave in the ''

Just let me know which was is preferred and I will make the changes.

jhodgdon’s picture

Our 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.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
4.36 KB
11.81 KB

For #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.

jhodgdon’s picture

Status: Needs review » Needs work

I'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:

+ * This displays the pages with recent hits in a given time interval. The time
+ * interval used is the value of the persistent variable
+ * statistics_flush_accesslog_timer. That value is set on the statistics
+ * settings form.

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.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
11.55 KB

Let me know how this looks.

jhodgdon’s picture

I'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.

NROTC_Webmaster’s picture

I 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.

xjm’s picture

+++ b/core/modules/statistics/statistics.moduleundefined
@@ -249,20 +249,19 @@ function statistics_cron() {
+ *   - 'totalcount': Integer that shows the top viewed content of all time.
+ *   - 'daycount': Integer that shows the top viewed content for today.
+ *   - 'timestamp': Integer that shows only the last viewed node.

I 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.

+++ b/core/modules/statistics/statistics.moduleundefined
@@ -249,20 +249,19 @@ function statistics_cron() {
+ *   A query result containing n.nid, n.title, u.uid, u.name of the selected
+ *   node(s), or FALSE if the query could not be executed correctly.

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:

A query result containing the node ID, node title, user ID, and username for the selected nodes(s)...

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.

NROTC_Webmaster’s picture

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

This 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...

NROTC_Webmaster’s picture

jhodgdon,

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.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs work

Uh oh, I didn't remove them...

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
1.6 KB

Here is the correction.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

Thanks! That's in 8.x, on to 7.x for real now. :)
(Note to self: check whitespace when committing.)

xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Reviewed & tested by the community

This cleanup is fine. (The textual improvements in the committed patch look good to me also.)

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Um. Already done. :)

xjm’s picture

Sorry, xposted. :)

NROTC_Webmaster’s picture

Status: Patch (to be ported) » Needs review
FileSize
11.15 KB

Since the documentation doesn't specify what the D7 standard was, it simply states

This standard was added for Drupal version 8.

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.

jhodgdon’s picture

Status: Needs review » Needs work

The 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

- * Menu callback; presents the "recent hits" page.
+ * Menu callback; Displays the "recent hits" page.

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.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
3.28 KB
11.15 KB

Thanks for the clarification. With that here is an updated version that hopefully meets the requirements.

jhodgdon’s picture

Status: Needs review » Fixed

Sorry 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!

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