Should statistics be collected on every page?

So this would be global for all accesslog hits as opposed to on a field-by-field basis, is that correct?

I'm a little weary to include this type of functionality by default for a couple of reasons:

• Path functionality is nice, but what if someone wants to run Statistics (or not) based on more advanced conditions? Like... I only care about collecting stats on pages served to robots. Or I only care about collecting stats on authenticated users.
• Seems to defeat the idea of an "access" log.

I toyed with the idea of integration with Context, but realized we'd run into similar issues we're having now with Drupal's incomplete bootstrap state on cached pages.

All of that being said, I recognize limiting statistics as a legitimate need in some cases. What if we added a hook like hook_statistics_skip(). Where by default, statistics will run on all pages, but if any function implementing hook_statistics_skip() returns true, it doesn't?

Sorry for the delayed response recently. Yes, I think I'd go for that.

I think we'll need to refactor where the files are included, but that should definitely be the goal.

Is this something you'd be able to take on? Shall I do the work to add the hook and refactoring mentioned previously, and then from there you can port your work from #2 to use the hook?

This should pass tests; if it does, feel free to run with it. Also includes hook documentation to get you started on hook_better_statistics_skip().

By the way, it'd be great if your path-based implementation could take some cues from how Drupal's default block configuration screen does it. E.g. checkbox for inclusive vs. exclusive (but no PHP filter option).

After you're done, I'll add tests for the role, path, and cached page stats skipping functionality.

The error I'm getting when running the tests locally is...

Fatal error: Call to undefined function truncate_utf8() in /path/to/better_statistics/better_statistics.statistics.inc on line 145

Looks like there's a call to truncate_utf8 in your hook_better_statistics_skip() implementation before unicode.inc is loaded. If we move the session bootstrap and unicode.inc include before the hook invocation, we shouldn't see any problems. Updated patch attached. Have not reviewed any of the other code, though.

Here's a screenshot of the path-based configuration for blocks; we should continue that pattern to minimize confusion.

Hmm... Before we open it up to multiple admin pages/forms, I wonder how it would look if we created a separate fieldset (collapsed by default) for filtering? And maybe clarify the current "Better Statistics Settings" fieldset to read "Access Log Data Collection" or something.

Also, I might suggest using "Access Log Restrictions" instead of "filters." "Filter" implies to me that the data is still being collected, it's just not being shown (like in a View), and that if I 'unfilter" it, the data will be there all of a sudden.

This looks excellent.

Couple of notes:

• I think I'd still prefer to have the contents of Access Log - Restrictions be in a vertical tab set (like on the blocks configuration page),
• The "pages to be logged" radio seems superfluous. Like the block configuration page, seems like we should be able to have a set of radios with options (All pages except those listed, Only the listed pages), with a single textfield. That textfield respects the radio selection. By default, we could have it set to "all pages except those listed" and the textfield blank.
• The "roles to be logged" radio also seems superfluous. Seems like we could get rid of it entirely, and just assume that if no roles are checked, all roles should be logged (might be good to have a description that indicates that too).
• Some of the description strings could use a little more polish. I don't have specific comments, but I can take a crack at them.

The tests look good to me. I think I'd like to add a couple of admin configuration page tests too (for example: the switching of cache enabled vs. cache disabled).

Getting closer!

This still needs work, but just verifying Testbot still likes it.

Most importantly, I changed how the API will work. I think it makes more sense to mirror drupal_page_is_cacheable() with a better_statistics_request_is_loggable() and providing a hook_better_statistics_prelog() to allow modules to react and set it if necessary.

I also tried (but failed) to put a vertical tab inside of a fieldset. I'm definitely not in love with it. I also did not do any of the additional tests/block configuration form copying I noted above.

I cannot for the life of me get this to pass locally, even though it's working as expected. Checking to see what Testbot thinks.

Thanks very much, mondrake! Must be an oddity of the test framework. I added one more test (for the cache enabled/disabled), and in doing so, discovered it was broken.

If everything is working on your end I'll go ahead and commit this and likely put out the next point release.

It's possible d.o's tsetbot might need a $this->refreshVariables(); after changing those configurations...

Yes, shortly.

Ah. Was checking for strings that only appear on the page when page cache is enabled.

Great. Committed: http://drupalcode.org/project/better_statistics.git/commit/6460aaf

Thanks as always for your contribution, mondrake!

Per #1892876: META: 1.x Roadmap, I'll be releasing 7.x-1.2 shortly.

Are the page-based restrictions still working and just barking a notice, or are they not working at all? Perhaps they're only broken in a multilingual site?

Thanks for the report. For my sanity, I'm closing this and opening up a follow-up with your notes.

#1907684: Notice: Trying to get property of non-object in drupal_lookup_path()