Just a question of checking an additional area in statistics_exit

    if (((arg(0) == "node") && (arg(1) == "view") && arg(2)) || ((arg(0) == "queue") && arg(1))) {
      if (arg(0) == "node") {
         $nid = arg(2);  // node/view/X
      }
      else {
         $nid = arg(1);  // queue/X
      }

Comments

jeremy’s picture

Assigned: Unassigned » jeremy
StatusFileSize
new3.1 KB

The attached patch applies against HEAD, and provides a new 'node_statistics' hook that modules can use to define their method of displaying a node. For example, the node module returns array('node','view'), meaning it displays nodes at 'node/view/#'. The queue module returns array ('queue'), meaning it displays nodes at 'queue/#'.

The attached patch modifies the statistics.module, node.module, queue.module, and book.module (the latter three being core modules that display nodes each with a different path).

Any overhead associated with the new hook is tolerable as it only happens after a page is displayed, in statistics_exit().

jeremy’s picture

StatusFileSize
new4.77 KB

Further cleanup.

Overview:
o statistics.module:
- introduces 'node_statistics' hook, allowing modules to define node view paths
- view counter and access logic are both hook aware to properly log node accesses
- removed unused '$recent_activity' global variable
o node.module:
- defines 'node/view/' path to viewing a node
o book.module:
- defines 'book/view/' path to viewing a node
o queue.module:
- defines 'queue/' path to viewing a node

jeremy’s picture

StatusFileSize
new4.93 KB

When Dries' tabs usibility patch is merged, this would have broken my new statistics hook. The attached patch makes a minor change to no longer assume that the nid is at the end of the path. (A module could, for example, pass in 'node/<nid>/view' as a node view path.)

In this updated patch, modules specifically define where to find the nid in the path.

jeremy’s picture

StatusFileSize
new5.33 KB

Update of previous patch introducing a new hook for the statistics module, allowing other modules to define node view paths. Prior to this patch, the module simply assumed that a node would be viewed at 'node/<nid>'. Thus, if a node is viewed in 'queue/<nid>' or 'book/view/<nid>', the statistics module wouldn't track it. With this patch applied, the module now knows about these alternate paths and thus all node views can be properly tracked. Future/contributed modules can easily teach the statistics.module about new view paths.

Resyncronized with cvs head. Also, now can support modules that need to define multiple node view paths.

If the accesslog and content-view-counter are disabled, the hook is not used. If either are enabled, it is used but only after the page is fully displayed (called from statistics_exit()).

Overview:
o statistics.module:
- introduces 'node_statistics' hook, allowing modules to define node view paths
- view counter and access logic are both hook aware to properly log node accesses
- removed unused '$recent_activity' global variable
o node.module:
- defines 'node/<nid>' path to viewing a node
o book.module:
- defines 'book/view/<nid>' path to viewing a node
o queue.module:
- defines 'queue/<nid>' path to viewing a node
jeremy’s picture

StatusFileSize
new5.33 KB

Previous patch mistakingly disabled the view counter when the throttle was 5. The view counter should never be throttled, only the access log.

Fixed by adjusting the parenthesis in statistics_exit() as they were intended to be.

moshe weitzman’s picture

on a related issue, it seems that the access log only shows the page title for node views. It is now possible to retrieve the page title for all pages using drupal_get_title(). IMO we should log that for all page views.

jeremy’s picture

That will have to wait for a future patch, as it will require modifying the accesslog table. At this time, there is no field for the title (just the nid).

Actually, after this patch is merged (or determined not worth merging), I was intending to work toward adding a 'path' field. Combine this perhaps with 'title' as you've suggested, and you'd be able to see what every user of your site is viewing.

dries’s picture

I'd love to get rid of multiple URLs for the same node (they should at least use the 'node/x' prefix). I have been talking about this with JonBob earlier today and wish to postpone this patch until we figured out whether this is feasible or not. Please don't hesitate working on the path-improvements.

jeremy’s picture

I'd be interested in hearing more about this idea to get rid of the multiple paths. From your brief mention, it sounds like there will still be multiple paths, but they'll all share the same root. Thus would there be 'node/x', 'node/x/queue', 'node/x/book', etc...? This would be problematic, as what would the path be to edit versus view? Perhaps you're thinking something like 'node/x/view', 'node/x/view/queue', 'node/x/view/book', etc...?

Anyway, I'll wait to hear the end determination, as I'm not interested in maintaining separate patches against the same file/code at this time.

Steven’s picture

"book/view" was removed and merged into "node/<id>". The only remaining issue is for "queue", and I'm not entirely sure that queue views count for regular views.

Setting back to 'active' for more discussion.

Stefan Nagtegaal’s picture

Status: Active » Closed (won't fix)

While queue.module currently lives in the Contributions, I'll set this to "Won't fix"