I am getting errors when I try to run the Top Pages and Top Referrers reports in the Statistics module. I completely uninstalled the Statistics module and re-enabled to make sure (I had trouble with some other modules because I enabled them at the same time as the Overlay module, which was causing hook_enable not to run, and I wanted to make sure that wasn't the reason for the problem, so that is why I disabled, uninstalled, and reinstalled Statistics). And for reference, I don't currently have any content on the site (I was just testing/verifying the help screens, so this is a new install).

The admin/reports/referrers page is just showing the following error messages and no page content (for reference, the site URL is http://bird/~eclipsework/drupal/ on my local box):

* Warning: Illegal offset type in isset or empty in SelectQuery->addJoin() (line 1190 of /opt/www/eclipsework/drupal/includes/database/select.inc).
* Warning: Illegal offset type in SelectQuery->addJoin() (line 1195 of /opt/www/eclipsework/drupal/includes/database/select.inc).
* PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE (LOWER(url) NOT LIKE '%bird%') AND (url <> '')' at line 1: SELECT COUNT(DISTINCT url) AS expression FROM WHERE (LOWER(url) NOT LIKE :host) AND (url <> :db_condition_placeholder_0) ; Array ( [:host] => %bird% [:db_condition_placeholder_0] => ) in PagerDefault->execute() (line 86 of /opt/www/eclipsework/drupal/includes/pager.inc).

admin/reports/pages shows the following error messages (and no page content):

* Warning: Illegal offset type in isset or empty in SelectQuery->addJoin() (line 1190 of /opt/www/eclipsework/drupal/includes/database/select.inc).
* Warning: Illegal offset type in SelectQuery->addJoin() (line 1195 of /opt/www/eclipsework/drupal/includes/database/select.inc).
* Warning: Illegal offset type in isset or empty in SelectQuery->addJoin() (line 1190 of /opt/www/eclipsework/drupal/includes/database/select.inc).
* Warning: Illegal offset type in SelectQuery->addJoin() (line 1195 of /opt/www/eclipsework/drupal/includes/database/select.inc).
* PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1: SELECT COUNT(DISTINCT path) AS expression FROM ; Array ( ) in PagerDefault->execute() (line 86 of /opt/www/eclipsework/drupal/includes/pager.inc).

CommentFileSizeAuthor
#18 659578.patch6.71 KBjhodgdon
#17 659578_statistics_admin_select.patch6.9 KBmcarbone
#11 659578_statistics_admin_select.patch6.94 KBmcarbone
#6 659578_statistics_admin_select.patch5.76 KBmcarbone
#3 659578_statistics_admin_select.patch1.85 KBmcarbone

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Title: Top Pages and Top Referrers modules giving errors » Top Pages and Top Referrers reports giving errors
mcarbone’s picture

mcarbone commented
Assigned: Unassigned » mcarbone
Priority: Normal » Critical

I can recreate, and will take a look. Can't ship with this, so this is critical.

mcarbone’s picture

mcarbone commented
Status: Active » Needs review
StatusFileSize
new 659578_statistics_admin_select.patch1.85 KB

A couple of arguments were missing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Shouldn't there also be a test to verify at least that the reports return an actual page, and better yet, that after requesting a page, that page shows up in Top Pages? I'm not sure how the Top Referrers report could be tested, as I don't think the framework supports referrers... and maybe the way the internal browser works there is no hit to stats either... but at a minimum the test could verify that the report comes up, so that next time someone makes a change in an API that breaks these pages, it would at least show up as a test failure?

I'll test the patch itself tomorrow...

mcarbone’s picture

mcarbone commented
Status: Needs review » Needs work

Makes sense. We can always add rows to the accesslog table directly in order for the test to pull something up.

mcarbone’s picture

mcarbone commented
Status: Needs work » Needs review
StatusFileSize
new 659578_statistics_admin_select.patch5.76 KB

Added a suite of tests for the various report pages.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

The patch applies fine and works for me. The tests look reasonable (at least there's a test to make sure the reports run and basic functionality, which is MUCH better than no tests).

However, there are a couple of problems with doc headers:
a) The new test classes you've added don't have doc headers at all. They need to.
b) At least one doc header for a class member function needs to be word-wrapped to 80 characters (there is one that you didn't create that you could fix in this patch too).
c) Function headers should start with a 3rd-person verb. E.g. "Verifies that..." rather than "Verify that...".

Other than that, I think it looks excellent.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Also, is it possible for the stats module tests to test that pages are logged correctly? The approach being used here adds something directly to the database, which doesn't test that on a page hit, the module works to log the hit. That seems to be a testing hole. One that was not introduced by this patch, ...

aaron’s picture

aaron commented

That normally happens in statistics_exit(). Not sure if we could just call that rather than updating the db manually, since the function calls drupal_bootstrap(), but maybe it's ok.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Ah... well that question (whether stats module should be tested to verify stats are taken on page visits) should probably be filed as a separate issue, to be discussed in more detail whether it's feasible or not. Will do so later, but I have to run out now... if someone else files an issue, please comment here and link.

Anyway, this latest patch still needs minor doc header cleanup. Just as a note, each class, function, etc. should have an 80-character line ending in a period and followed by a blank line, explaining what it does.
See http://drupal.org/node/1354

I think that is the only thing wrong with the patch above.

mcarbone’s picture

mcarbone commented
Status: Needs work » Needs review
StatusFileSize
new 659578_statistics_admin_select.patch6.94 KB

Fixed the doc headers. Also, testing access logging was fairly easy, since it works correctly when drupalGet() is called, so I added a test for that as well.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work

Hmmm....

I don't see the new test for access logging in your patch... am I just being dense?

The doc headers are much better, but the one-sentence description that starts them is supposed to be just one 80-char line, not wrapped to the next line, according to the doc standards. We're trying to get the older function headers up to standards -- not all there yet -- but definitely anything new going in should follow the standards.

mcarbone’s picture

mcarbone commented
Status: Needs work » Needs review

It's in "function testAccessLogging()".

The doc headers are much better, but the one-sentence description that starts them is supposed to be just one 80-char line, not wrapped to the next line, according to the doc standards

You're saying that they need to be 80 characters max? Interesting -- not sure why it's worth sacrificing length for clarity, but I'll post a new patch up tomorrow.

mcarbone’s picture

mcarbone commented

oic, it's a summary, followed by more details if needed.

mcarbone’s picture

mcarbone commented
Status: Needs review » Needs work
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

OK, I see the test now. Must have been blind there. :)

mcarbone’s picture

mcarbone commented
Status: Needs work » Needs review
StatusFileSize
new 659578_statistics_admin_select.patch6.9 KB

One more time, with 80 chars max doc headers.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 659578.patch6.71 KB

That version was pretty close... I made some minor tweaks to the doc headers, and will take the liberty of marking it RTBC.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Great to see the patch came with tests. Thanks.

Status: Fixed » Closed (fixed)

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