I do not know how many sites out there use core's statistics module, but there are certainly some that do. There are a lot of 3rd party services that do the same thing (probably even better), but I've met webmasters that prefer integrated statistics, since it fits better into their workflow.
Statistics module is, from the other hand, not very flexible and not really performance friendly. There is no way to change or extend it's behaviour at all. It can also become a performance issue on high-traffic websites since it performs a DB write on every page request (webkenny have spoken about that topic in his session @ DrupalCon Chicago).
Simple performance optimisation could be done via cache. Statistics module could track visits in Drupal's cache and keep it there until first cron, when data will be written to DB. This wouldn't offer any benefit for default DB cache, but it will be much better when memcached or APC are used, which are, frankly, used on most high-traffic Drupal sites. This is currently, unfortunately, impossible to do without patching core.
Another possibility would be to move this module more towards a simple framework for statistics, that would log statistics via custom "engines". Each engine would be responsible for all "dirty work" behind the scenes. Core would in that case implement just default implementation, that can be similar than current one. Other engines could be implemented by contrib modules. This implementation could be similar to core's cache, where we have default DB cache in core and contrib modules that implement other cache backends.
Are there any plans in this direction? Are there any other ideas about this module?
Comment | File | Size | Author |
---|---|---|---|
#247 | 1209532-ajax-stats-not-working-IIS.patch | 667 bytes | david_garcia |
#228 | drupal-1209532-statistics_via_ajax-228.patch | 6.66 KB | sdrycroft |
#217 | drupal-1209532-statistics_via_ajax-217.patch | 6.56 KB | iamEAP |
#215 | drupal-1209532-statistics_via_ajax-215.patch | 6.38 KB | iamEAP |
#207 | 1209532-206.patch | 1.53 KB | timmillwood |
Comments
Comment #1
timmillwoodhttp://drupal.org/node/1018716
Comment #2
EvanDonovan CreditAttribution: EvanDonovan commentedI like the idea of making it a pluggable framework; I think that's been proposed for Search module as well?
Comment #3
RobLoachSeriously? There's already so many solutions in the contrib world that already put the core Statistics module to shame. What would a Statistics framework provide? Drupal already has quite an amazing framework for retrieving information from itself. Not to mention all this doesn't work if you're running your website correctly on top of Varnish.
I'd suggest setting this to won't fix, #1018716: Remove Statistics module from D8 core, and recommending people look at contrib solutions via Open Web Analytics, Piwik or Statistics Pro. I'm sure there are more too.
Looking through the Statistics commit log, it doesn't look like any Statistics-related features haven't been added in quite a while. If it hasn't been improved in that long, do you really foresee it improving within the next year when there are already better contrib solutions available? Most people who use Drupal would just end up wanting Google Analytics anyway.... This is also avoiding the issue of it not even working behind Varnish/Boost anyway.
Comment #4
droplet CreditAttribution: droplet commentedMalware wanring for the OWA site :(
www.openwebanalytics.com contains malware. Your computer might catch a virus if you visit this site.
Comment #5
Dave Reid@RobLoach the idea would be that the statistics framework provides the re-usable things like 'the user doesn't want to be tracked' or 'exclude tracking on these paths' that *EVERY SINGLE* module duplicates. Why should a site that switches from GA to Piwik have to have their users opt-out all over again? This is why we can provide something in core that can easily be used by contrib.
Comment #6
jcisio CreditAttribution: jcisio commentedA framework sounds interesting, but I'm not sure how to do it correctly, and have something that works behind a reverse proxy.
There is a long discussion on gdo about how to make statistics better for high traffic websites http://groups.drupal.org/node/94609. In brief, statistics module now can work as expected with two modifications:
- For better precision: use ajax callback that pass the reverse proxy. This callback url is not neccessary on the same Drupal installation, but to a simple Drupal install that reads/writes statistics.
- For better performance: as described in the issue description, we put every page stats record in a cache table, and aggregate/update our statistics on cron.
Comment #7
webkenny CreditAttribution: webkenny commentedA big +1 to a pluggable back end to statistics. I am more than willing to be active in that effort. What this should be is something akin to Views and Views UI, so it's hard to know if it should be in core or in contrib (as side issues state) - But I think the concept of doing this as a framework is huge.
When I land in the states, I'll address specifically some ideas. But want to toss in my support early.
Comment #8
slashrsm CreditAttribution: slashrsm commentedI was playing a bit with "the Varnish problem". I implemented a draft layer around core statistics module, that sends stats data via AJAX request after page load. The code can be found in a sandbox: http://drupal.org/sandbox/slashrsm/1247306.
I understand that most of the sites use Google Analytics, but there are use cases, when you also need internal Drupal Statistics. I have some websites at the moment, where customer uses GA, but we still use Drupal's Statistics for internal statistics information, which they need for their editorial decisions. We also need Drupal's stats to build lists of most read/most popular content. You cannot do this using GA, though.
I am prepared to get involved in this (and also to co-maintan/maintain) if Statistics stays in core or if it is moved into contrib. At the end of the day it is basically the same, but I still think that we should have al least some kind of stats in standard profile.
Comment #9
meba CreditAttribution: meba commentedIt's good but my issue is that Radioactivity module is just way better and already in contrib...
Comment #10
xmacinfoRadioactivity is overkill. We need to simplify statistics to keep only the basic view counter. The rest will should go to contrib.
Comment #11
bojanz CreditAttribution: bojanz commented+1 for empowering slashrsm in any way.
From a programming perspective, the counter is anything but simple and pretty much requires relying on memcache and / or doing some JS magic like the node.js solution. A SQL INSERT per page view is murder. It can't scale.
Comment #12
cweagansHonestly, I think the best improvement we can do is `rm -rf modules/statistics` and leave it to contrib. This is not something that should be in core.
Comment #13
jcisio CreditAttribution: jcisio commented@bojanz:
I don't know why it can't scale. Every web server does the same thing for access log, every analytics tool does the same thing. And it scales. The problem is that how we do that INSERT.
Comment #14
cweagans@jcisio, think about a scenario where a site is getting hundreds of thousands of page views per minute.
With the current setup, that's hundreds of thousands of queries per minute.
Does anybody really have the access log turned on in production? Analytics tools can be load balanced across multiple servers (Google Analytics is, and it's trivial to do the same with any other analytics setup).
That's why a bunch of people are saying that it's better offloaded to a 3rd party provider. 3rd party providers are going to be able to do analytics much better than we will ever be able to, and their tool will be able to evolve and grow all the time, unlike something that's locked into core for n years.
Comment #15
webkenny CreditAttribution: webkenny commentedI think it's important we distinguish here between traditional access_log writes and database writes. @cweagans makes a good point. Folks don't use the DB intensive access log capabilities on production sites. Instead, the file system gets those writes which will always be faster. It's the same reason people use syslog and not dblog on their production sites.
Bottom line. In support, daily, and at many performance chats folks recommend that the Statistics module be turned off. Way too much activity on higher traffic sites.
Now, I believe we still have to have some option for folks to have the most basic statistics on their sites but why not do this in a file on the file system (not necessarily in sites/default but somewhere) where they can parse these? It's an option for the lowest common denominator of sites. Much like an example which other modules could build from. You could have this as option that it writes from core to a faster backend or that it doesn't and some other module/service will handle the captures.
Let's get real though. Here's how I would see something like this working. Providing the API for modules to hook into and do their own processing would be excellent but if we were to look at this at a high level:
Borrowing from the design patterns of our platform and basically "broadcasting" statistical events that modules could consume would be an excellent way to standardize and then allow all the fancy processing to take place somewhere else be it in a module or a module that connects to a third party service.
Drupal's core shouldn't be responsible for that. The less monolithic we can make the Drupal requests and the more we can plug into existing systems which already do the work well, the better IMO.
Love where this conversation is headed, btw. Great stuff in here.
Comment #16
jcisio CreditAttribution: jcisio commented@cweagans I tested with my Atom-like server (2 GB RAM, VIA Nano processor U2250 = half of an Atom 330), and it can do 5000 inserts per second or 300.000 inserts per minute. If your site has hundreds of thousands of pageviews a minute, your site has more pageviews than all Wikipedia sites aggregated (6B pageviews/month or 140.000 pageviews per minute).
However, we even don't need to deal with all of that. What we want to do is to make sure that it can scale. And yes, db insert can scale.
If your production server does not log, it's a personal choice. Many servers have it enabled. It costs nearly nothing to write log, except when you want a single server to serve ten thousands of hits per second. Only in that case it may affect your performance.
About an API for statistics, it's great but it seems too hard. As you need to write record directly, or fallback to Ajax request when behind a reverse proxy, and that's the tricky part.
Comment #17
Bojhan CreditAttribution: Bojhan commented@davereid Is it not the role of a search API module to provide that kind of functionality? I am a bit split on the fact that all discussion here seems related to providing better API's and high-end website performance problems.
Comment #18
timmillwoodI really think this module needs to be deleted and rewritten from the ground up
http://www.millwoodonline.co.uk/blog/my-views-drupal-statistics-module
Here it what I would like to see:-
My personal next step would be to remove the statistics module and start an initiative to rewrite the module from the ground up.
The first steps for the new module would be to define a new hook, maybe hook_statistics. This hook would be called at every page load and passed details / statistics about the page being loaded and machine loading it via Javascript / AJAX. Many modules could then use hook_statistics to save this data to the local database or any datasource.
Comment #19
fgmOne thing to consider is that some contrib modules may be using data from statistics.module (esp. content views) as it is the only reliably available source of content views on any drupal site, which can be required since it is part of core. Of course, larger sites will have professionally set up analytics from Google, Xiti or whatever, but this is only the tip of the iceberg when it comes to drupal deployments.
The problems with performance are very real, of course, but having "something" in core (or standard profile) to count content views seems difficult to avoid, and likely difficult to put outside of core (kernel) due to the performance implications. And if a "hook_statistics" exists, even these could be offered as a standard way to query for these data even when the core/kernel/standard module is replaced by something with a better performance profile on higher-level sites.
Comment #20
giorgio79 CreditAttribution: giorgio79 commented#fgm "One thing to consider is that some contrib modules may be using data from statistics.module (esp. content views) as it is the only reliably available source of content views on any drupal site, which can be required since it is part of core. "
Any contrib module can require another contrib module. Views requires CTools. This shouldn't be an argument for keeping it in core :)
What matters is whether there is a real need for it. If a module satisifes it, the webmaster will install it whether it is core or contrib.
It seems the point is Statistics needs to grow stronger and it is difficult to develop it as fast as needed in core...
#DaveReid "@RobLoach the idea would be that the statistics framework provides the re-usable things like 'the user doesn't want to be tracked' or 'exclude tracking on these paths' that *EVERY SINGLE* module duplicates. Why should a site that switches from GA to Piwik have to have their users opt-out all over again? This is why we can provide something in core that can easily be used by contrib."
This can be provided in contrib as well. I never switched from Google Analytics to anything else. :) Although I may not be representative of the average Drupal webmaster.
Comment #21
slashrsm CreditAttribution: slashrsm commentedI totally agree with things pointed out in #15 and #18. Is there any interest for a sprint, where we'd plan this and start with initial implementation?
Comment #22
RobLoachInstead of improving the Statistics module, what if we were to improve the Radioactivity module?
Comment #23
timmillwoodGreat idea Rob, created another issue to track this idea. http://drupal.org/node/1277710
/me goes to install Radioactivity module.
Comment #24
fgmMaybe it has improved since 2010 but at the time, Radioactivity just was not an option for high-traffic sites: too much of a hog on resources.
Comment #25
timmillwood@fgm - maybe that's something we can help with when moving it to core. I have not extensively tried the module yet, but from what I hear it is better than the core statistics module.
Comment #26
jcisio CreditAttribution: jcisio commented#22 I'm not sure if Radioactivity does the same thing as statistics.module does, i.e. it counts node views.
Even radioactivity can technically count node views (by using a very long lifetime), I don't think that it's something that could go in core.
Comment #27
fgmOne thing that could be improved would be to bring in counting/listing 403/404 if these are to be lost when evolving the statistics/dblog couple, as was suggested on the monster module dropping issue.
Comment #28
xmacinfoSemantically, we do not need statistics in core. Statistics helps giving an overview, a dashboard-like view of the current statistics or past ones. With statistics we can do all types of reporting.
All statistics reporting engine should live outside core.
What we need in core, though, is a simple counter to at least duplicate what's in Drupal 6 and 7, ie. counts node views, counts 404 errors, etc. Hence, this is why we need to scale down the statistics module (and maybe rename it to simplecounter!).
As for radioactivity, this is overkill, and even if we take this and move it to core, at the end we would need to scale it down.
Comment #29
tcmug CreditAttribution: tcmug commentedActually, perfect timing for this:
Radioactivity is heading towards a slimmer and lighter set of basic features that will be easy to extend: it will in its most basic form track "views" and have cache piercing and be as fast as drupal can (memcache storage, non-drupal-boostrap storage would all be extensions).
The current Radioactivity 2.x in core sounds way silly. I'd go for having the extendable basic functionality in core instead that is in the (unwritten) plans for 3.x.
Comment #30
fgmIn a way, this could be like the current tripled of
includes/bootstrap.inc#watchdog()
vsmodules/dblog
vsmodules/syslog
:- a common API to record information
- one or more core implementations, at least one geared towards small sites and easy debugging, and possibly another one geared towards larger sites
Comment #31
xmacinfo@tcmug: “…(unwritten) plans for 3.x”. Well, can someone actually write the plan? :-)
Comment #32
tcmug CreditAttribution: tcmug commentedSort of what @fgm said on #30. I should point out that at the moment Radioactivity works pretty much solely on fields; a field that contains the number of views - or energy as it is in R - and that is then used in various manners to calculate popularity or just plain number of views. A field makes it useful on all entities and with a bit of Rules magic you can count views per week, per month etc. or as an example with Drupal commerce there is of course the number of sales that you could be interested in :)
We also use it to track user activity (login emits energy, commenting, blog posts etc.) and this is where the decay profiles come handy: since the energy decays over time only those who are continuously active have high energy and are thus rewarded somehow. So, its quite a versatile thing and I'd love to see something that would be the backbone in core.
@xmacinfo #31 sure, why not - I'll write em up :) But I'll of course make them bendy ones so that it'll be easy porting to D8 hehe
Comment #33
chx CreditAttribution: chx commentedWell, that must be MyISAM. Cos you wont commit more than 120 transactions per second unto a 7200RPM disc (cos you have 120 rotations a second and you can't do more than one fsync per rotation). And, if you happen to SELECT from other parts of the disk (ie need to seek) then you won't even get a commit for every rotation either. Or does this low-end server have a fast SSD? 5000 IOPS is not a small amount even for an SSD but def. doable.
Comment #34
joachim CreditAttribution: joachim commentedSee also #1120928: Move "history" table into separate History module.
Comment #35
jcisio CreditAttribution: jcisio commented@chx: yes it was MyISAM with a low-end HDD (I guess, as this server costs 15 EUR/month). I don't think we need transaction support for this table, by the way.
Comment #36
timmillwoodHi,
I would like to get back on to this issue. I really feel that a patch for the statistics module to do everything I would like to see would alter 99% of the lines. Therefore, can we remove the module, then work in contrib to make something better. If this is better module exists and works well for D7, we could then look at porting it to D8 and moving it to core.
Tim
Comment #37
timmillwoodStep 1) merge http://drupal.org/project/statistics_ajax into the statistics module so that it works with Varnish.
Step 2) reduce database writes.
Comment #38
jcisio CreditAttribution: jcisio commentedThere is also a new module http://drupal.org/project/jstats that is known to work quite well, too. The problem is not only Varnish, the bigger issue is that a direct update to node_counter does not scale (each update requires a whole index rebuild).
Comment #39
timmillwoodjstats looks interesting, I had not seen it.
Maybe I will take the best from everything and try to get an AJAX / less database writes patch together.
Comment #40
slashrsm CreditAttribution: slashrsm commentedI agree with #37. Apart from that I'd also add more flexibility. Currently we count node views just for one day and entire existence of a node. I see a powerful and flexible statistics framework not accepting this kind of assumptions. Why couldn't I track node views for last 3 days, 2 hours, ...
I've tried a lot of contrib modules offering statistics functionalitites, and none of them completely meets my requirements. The closest for now is Radiocativity, though. Mostly because of it's support for light bootstraps and Memcached support.
I think it is time to take a decision:
- should we remove statistics from core and create new, more powerfull and flexible, module in contrib (or extend one of existing ones to be really powerfull - even better)
- rewrite core's statistic to offer a sufficient degree of flexibility, scalability, plugability and extensibility.
This should, of course, also be affected by general orientation of core development in future, but I think that having a statistics module "as it is" in D8 is not wise at all.
Comment #41
timmillwoodMy biggest concern with the statistics module is how many database writes it makes. I am trying find a place to store the data until cron runs, then write it to the table every hour.
Although, I am struggling to find somewhere to store it. Using another table would help a little as the indexes would not need to be cleared as often, but it would still require a lot of writes. I have also been thinking of how it could be stored in memory via php or MySQL.
I am open for other ideas.
Comment #42
slashrsm CreditAttribution: slashrsm commentedRadioactivity uses Memcached, which works great (data is stored into memcache on emmit and agregated and stored in DB at cron). They also implement a simple script, that is used at JS emmits. Advantage of this script is ability to save data without bootstraping Drupal, which is much lighter.
There are some other options besides Memcached: APC user cache, MongoDB, Cassandra, any other NoSQL solution or even a txt file. It totally depends upon requirements and limitations you have. This is why I see storage as a pluggable framework, which lets user decide which technology to use.
Comment #43
jcisio CreditAttribution: jcisio commentedStore it in another table (maybe with Memory engine) without index would be a good solution. We can aggregate data periodically. Memcache does not allow that, at least without an ugly loop through every node.
Comment #44
tcmug CreditAttribution: tcmug commentedWe wrote Radioactivity to be as light as possible with being flexible at the same time - you can do much more than register node views with it.
I don't see a valid reason why core should have a statistics module - however (and I think I stated it up there before) I see a need for adding simple tasks to drupal without drupal bootstrap.
Currently I'm in the process (but lacking the time) in separating the Radioactivity incident api to a module of its own - it would be a fairly small module and provide only the gears for sending an action to drupal for processing with multiple ways (ajax, nodejs, rules, etc.) (http://drupal.org/sandbox/tcmug/1364084 no commits yet though...)
Comment #45
timmillwoodHere's a patch which makes the node_counter use ajax.
I realise it's not a full solution to the statistics issues, but it's a step in the right direction.
Comment #46
timmillwoodAdding need's review
Comment #47
cweagansPlease don't add new standalone PHP files. In this case, there's no reason to do so: it can easily be added to statistics_menu().
Comment #48
timmillwoodMy reason for adding a standalone php file is so that I didn't have to fully bootstrap drupal every ajax call.
Is there a way to do this with hook_menu?
Comment #50
bojanz CreditAttribution: bojanz commented@timmillwood
No. And there should be. Because without that the tiny ajax requests (registering a page visit, or a VBO checkbox selection) are almost useless because they are too slow.
EDIT: D6 contrib had sun's http://drupal.org/project/js
Comment #51
timmillwoodI have now added the caching of statistics get, the cache is then set to clear every cron run.
Comment #52
timmillwoodComment #54
timmillwoodJust noticed I managed to include my statistics and taxonomy patch into one.
Here's the real addcache patch.
Comment #55
timmillwoodJust noticed I managed to include my statistics and taxonomy patch into one.
Here's the real addcache patch.
Comment #57
timmillwoodurgh, just noticed that the cache is caching the same number of views for all nodes!
*goes back to work*
Comment #58
timmillwoodcaching is a little better now.
Comment #60
timmillwoodI am wondering if the statistics module should count a node view every time a node is loaded even if it's in a view, panel or block.
Comment #61
xmacinfoNode count should represent only the count where the complete node is displayed. If only the teaser or fields are shown, that would not apply for the core count. Panels, views and others could implement their own counters if they also want to count node load vs full node display.
So I'd like to keep a simple node count in core and leave the rest in contrib.
Comment #62
cweagansI'd say that we should only track full node views and expose some API function that will allow other modules to modify that count easily.
That said, if we're down to the point where we only have a simple view tracker in core, is it really worth having in core anymore?
Comment #63
timmillwoodI think it is worth having a node_counter in core, many sites small, large and enterprise want to display "most popular" content. Therefore I don't think we should remove this feature, but it should also work when using external cache and also not be a performance issue.
I do however feel that the accesslog features in the statistics module are a little pointless with tools like Google Analytics offering a better alternative.
Comment #64
timmillwoodI would like to work on a better way of caching the node couter. On pages such as /node, taxonomy listings, and views, the node_counter table could be getting queried 10 times or more. Each of these queries are based on a different node id. Would it be better to store all the node counts in a serialised value as an object or array, then query that one value? Then when counting a node view would it be better to increment the cache directly rather than increment the database table? Does node_counter only really need to be a table as backup for the cache? therefore could the node_counter table just get updated from the cache each cron run?
/me might organise a drupalcon sprint on this (although I won't be in denver).
Comment #65
timmillwoodqueues vs cache?
Comment #66
moshe weitzman CreditAttribution: moshe weitzman commentedI recommend splitting off any read optimizations into a new issue. I would focus here on the great logging improvements you have made.
I think that only node/n views should be logged here.
Comment #67
timmillwoodGood thinking Moshe.
Here's the patch again that added ajax. It still includes statistics.php rather than using hook_menu just because of the performance benefits.
The issue I now have is that simpletest fails because it doesn't do the ajax call.
I doubt it will get committed without passing simpletest?
Comment #69
timmillwoodSee the new statistics performance issue #1446932: Improve statistics performance by adding a swappable backend for items relating to performance.
This issue should be used for discussion around the use of AJAX to count node views allowing the use of external caching such as varnish or a CDN
All other statistics items should have a separate issue.
Comment #70
nod_About the js, as it is now it'll run multiple time if there are other drupal ajax calls on the page,
Comment #71
timmillwoodWell, that's a bit stupid. Whys that?
May be we should fix it rather than use the 'run' workaround.
Comment #72
nod_Ahaha no, that's the way it was designed, it's used to run behaviors init on new content coming from Ajax.
You don't even need to use behaviors here, beside convention maybe.
settings
is reallyDrupal.settings
and it should already be available, just make sure you include that in the footer.Comment #73
joachim CreditAttribution: joachim commentedJust to check I've followed this correctly... the way for a module such as Views to tell statistics module that 'nodes 1, 2, 3 have been looked at' would be to add something to the View which outputs that but with an array of nids:
Comment #74
timmillwoodJoachim,
Currently the patch I did only accepts one value for a nid. Therefore will only count one node per page. Therefore by design it's to could the view of a /node/ page and not the nodes loaded in a view. Although I could allow it to accept an array of nids then a contrib module could add the multiple node counting feature.
Comment #75
timmillwoodQuite a few changes here.
Moved adding of the JS to hook_node_view.
Moved the javascript to the footer and out of a behaviour.
Please review (the simpletest will fail as it doesn't know much about javascript)
Comment #77
timmillwoodForgot to add statistics.php and statistics.js
Comment #78
timmillwoodSorry, did it again
Comment #80
xmacinfoDon't forget blank like at end of files. :-)
Comment #81
joachim CreditAttribution: joachim commented> Although I could allow it to accept an array of nids then a contrib module could add the multiple node counting feature.
I think that would be really useful. It would allow lots of interesting possibilities in contrib.
Comment #82
timmillwoodI will work on allowing this to accept an arrary of NIDs.
I also need to get the tests sorted, any help on the tests would be great.
Comment #83
timmillwoodI have been delayed on this trying to get the tests to work. I hope to get a new patch out soon that passes the tests either by fixing or removing the tests related on my Ajax changes.
Comment #84
timmillwoodThis patch allows the option to count node views even if only the teaser is loaded. It uses the same ajax system as the last patch.
Please review.
Comment #85
timmillwoodThis patch allows the option to count node views even if only the teaser is loaded. It uses the same ajax system as the last patch.
Please review.
Comment #86
timmillwoodThe patch did not attach properly
Comment #87
timmillwoodTrying that again.
Comment #88
timmillwoodPlease ignore the patches in 84, 85 and 86.
Patch in 87 has passed, would be good to get some reviewers.
Comment #89
scitoThe last submitted patch removes only tests. There are no functional changes. It might be a problem.
BTW: I'm really glad that you work on this module. Thanks for your efforts so far.
Comment #90
timmillwoodAww man! I really need to check / test my patches better.
Thanks for looking.
Comment #91
timmillwoodI had committed changes so I could do a pull, and I was only diffing with the local repo rather than the origin.
Here is the full patch to test.
Comment #93
timmillwoodFails due to search.test?
WTF?
Comment #94
xmacinfoI hope this is configurable. Usually we want to add only full nodes to the counter.
Comment #95
timmillwoodYes, the patch adds a new checkbox to the settings page.
https://skitch.com/timmillwood/8md5r/statistics-d8.localhost
This defaults to the current behaviour in statistics.module of only counting full node views.
Comment #96
xmacinfo@timmillwood: awesome!
Comment #97
timmillwoodComment #98
lucascaro CreditAttribution: lucascaro commented#91: statistics-ajax-1209532-91.patch queued for re-testing.
Comment #100
lucascaro CreditAttribution: lucascaro commentedHi, after an initial review of the patch, I've encountered some small issues that I modified and I'm attaching a new patch with these simple issues corrected:
* Removed a console.log in statistics.js
* Removed a comma after data: "nids="+nids in statistics.js
* Removed trailing whitespaces
* Removed ?> at the end of statistics.php
* fixed indentation in statistics.php
Comment #102
lucascaro CreditAttribution: lucascaro commentedCorrected the test case for search.test since it was counting on a non-ajax statistics. Now it works using a post call to statistics.php.
All the changes are in the patch below. please review!
Comment #103
lucascaro CreditAttribution: lucascaro commentedHi guys, I think this patch could have a security vulnerability. The new file, settings.php doesn't check anything about the request and I think it could be vulnerable to a DOS attack (since it makes two database writes per request and the only validation is that $_POST['nids'] is set.
Maybe these two lines fix that?
I just wanted to have someone else look at this just in case.
Also, what would happen when nodes are loaded via ajax?
Cheers!
Comment #104
timmillwoodIt checks if $nids is an array, it then checks if $nid is numeric. I am not sure of a way to exploit this, but others may know better. (/me pings greggles)
Comment #105
chx CreditAttribution: chx commentedI am less sure there is a security problem but from a performance standpoint it's silly to run one query / nid, just collect them nids in an array and run one query. IN is your friend. However, despite what the doxygen says db_merge does not allow multivalue inserts. Commit this as it is, file a database issue to fix this and link a followup issue to convert to a single db_merge.
Comment #106
lucascaro CreditAttribution: lucascaro commented@timmillwood, @chx great. RTBC then?
Comment #107
timmillwoodI would like to get the tests I took out back in, but only having partial success. I am happy for RTBC as long as I start a new issue to work on tests.
Chx: I have have been thinking about performance and was thinking about adding a cache table for statistics. Writing to then within statistics.php then writing to the node_counter table on cron, although not sure if this is the best option.
Comment #108
lucascaro CreditAttribution: lucascaro commented(for the tests)
Comment #109
timmillwoodAdded the tests back in, which are working better, but not perfectly.
Comment #111
timmillwoodHope this one works better for you.
Comment #112
timmillwoodfixing path variable in tests.
Comment #113
lucascaro CreditAttribution: lucascaro commentedreroll with minor spacing corrections and patching search.test
Comment #114
Lars Toomre CreditAttribution: Lars Toomre commentedThere are some formatting issues in statistics.php, including:
1) Incorrect indentation...
2) 'if(' needs to be 'if ('. (needs a space after if)
3) 'foreach(' needs a space after 'foreach'.
Comment #115
lucascaro CreditAttribution: lucascaro commentedrerolled again, this time includen the patch to search.test by @timmillwood and the fixes suggested by @Lars Toomre
Comment #116
lucascaro CreditAttribution: lucascaro commentedComment #117
Lars Toomre CreditAttribution: Lars Toomre commentedHmmm... Not sure what is in #115 patch, but formatting of statistics.php appears to be the same.
Also noticed that there are some indent issues with statistics.js.
Perhaps wrong patch was attached?
Comment #118
timmillwoodfixing the indentation and spacing issues.
Comment #119
lucascaro CreditAttribution: lucascaro commentedjust for the test
Comment #120
lucascaro CreditAttribution: lucascaro commentedmarking as RTBC since it's been checked by various users, and the tests are back in.
Comment #121
timmillwoodSo nice to finally see this RTBC!
Comment #122
Lars Toomre CreditAttribution: Lars Toomre commentedSomething funny is happening with testbot... Only passes 11 tests?
Setting back to needs review until all tests pass.
Comment #123
timmillwood#118: statistics-ajax-1209532-118.patch queued for re-testing.
Comment #124
Lars Toomre CreditAttribution: Lars Toomre commentedSome more coding nits...
The indentation of the first set is one too much it seems.
There needs to be a space between if and '('.
My understanding of coding standards is that there should be a space after each ',' in the array declaration. This is repeated several times throughout patch.
Comment #125
lucascaro CreditAttribution: lucascaro commented#118: statistics-ajax-1209532-118.patch queued for re-testing.
Comment #126
timmillwoodWhen running the "STATISTICS REPORTS TESTS" using mod_fcgi I was getting 500 errors, now using mod_php I get fails.
https://skitch.com/timmillwood/8p8tp/test-result-stats.localhost
I am currently unsure why these are failing or why mod_fcgi / mod_php makes a difference.
Comment #128
timmillwoodFixed the block test, forgot to call statistics.php, also tidied up some of the code.
Comment #129
timmillwoodComment #130
timmillwoodOnce committed to D8 I would like to re-roll this for D7.
Comment #131
lucascaro CreditAttribution: lucascaro commentedHey @timmillwood that's great! I have a few more comments, though. I don't know how much these are enforced but:
According to dreditor, there are still a few minor issues:
No newline at end of file:
two extra spaces:
also here
Also, according to http://drupal.org/coding-standards#phptags the closing ?> should be removed in statistics.php
what do you think?
Comment #132
xmacinfo@lucascaro: You are right. All the points you highlight should be fixed. Thanks for checking these out.
Comment #133
timmillwoodFixing @lucascaro's suggestions, but... also making the new checkbox only appear when the old one is checked using states.
Comment #134
Lars Toomre CreditAttribution: Lars Toomre commentedThanks for the code clean up. It looks good except search.test which is missing spaces in the array definition there.
Comment #135
timmillwoodRe-roll for Lars
Comment #136
timmillwoodchanging status
Comment #137
timmillwoodComment #138
catchThis needs an @file header. It should also use Drupal.behaviors rather than document ready.
Is there a particular reason to make the AJAX post request instead of putting a 1px gif on the page or similar?
We usually avoid the work 'node' in the UI. I don't necessarily agree with this and there's more than one issue trying to standardize, but it should be consistent with settings elsewhere in core.
"Count views of only whole content and not as part of blocks, lists or teasers." - this reads somewhat awkward to me as well, generally we don't have the 'view mode' concept fully embedded everywhere in core yet but referring to that might be better than the examples, or dropping the description altogether.
Comment should be above the line.
If the only reason for the global is to avoid adding the same nids more than once during a request, then it should be a static variable. Also this is calling drupal_add_js() even when we otherwise might not be adding a new nid seems unnecessary - looks like it'd be OK to add the js file once with #attached and then just update the setting.
TBH I think it'd be easier to review this if there was one patch to switch from drupal_exit() to the ajax (or a 1px gif, I really don't know which is preferred at the moment), then a separate one about adding the feature to track statistics for node teasers - I'm not sure about adding that feature to core at all to be honest - it's adding extra complexity and configuration and generally I only want to know if a page was viewed directly, not if someone looked at a list of nodes where it happened to be.
Comment #139
timmillwoodUsing a gif would be a good option, it could allow access_log and node_counter to work, it will also work for users who do not have Javascript enabled (unless the gif is added via JS, but I should be able to add it at the bottom of the page before some how.). The only concern I have is making sure it doesn't get cached, although headers could be set to stop it being cached.
I wanted to make statistics.php flexible enough so contrib modules could use it to could single and multiple node views, who while I was making sure it could take an array of nids I thought I might as well build in the checkbox to send multiple nids.
Comment #140
timmillwoodI am not sure using a gif is the cleanest way to do this as seems a little messy and could end up breaking site layouts. The AJAX route seems to be the most sensible and cleanest way to implement this.
I did not use behaviours under advise from nod_, as it means cleaner, simpler javascript that doesn't get run multiple times.
I will create a new patch removing the multiple nids feature, and tidying up the comments.
Comment #141
jcisio CreditAttribution: jcisio commented@timmillwood: when a node is fetched (via AJAX) and display in, e.g., a modal frame, does it count?
Comment #142
nod_The easiest way to address that is provide a JS function doing the actual ajax call. After it'll be the responsibility of whatever display the node in the modal form to integrate with stats JS API.
Comment #143
timmillwoodI'm not sure.
If hook_node_view is called, then the nid gets added to the Drupal.settings ready to be counted, and the statistics.js file is added to the page. So if the node is loaded via AJAX, hook_node_view will get called, but I don't think Drupal.settings and statistics.js will get added correctly.
Comment #144
jcisio CreditAttribution: jcisio commentedWe can use Drupal.behaviors and check for div with class/id to know if a node is rendered in full view mode and get the nid. Of course themer could change the tpl to remove id/class but it's their responsability.
I think it's universal and better if we plan to use JS.
Comment #145
nod_data-
attribute, much better than a class or whatever. there is an issue somewhere in the queue to remove as much stuff fromDrupal.settings
as possible and replace with data- attributes on the relevant element.Comment #146
jcisio CreditAttribution: jcisio commented@nod_: yes it's much better, it will also prevent other problem (duplicate ID etc.). Of course, it is not a blocking issue. Change to use data-nid is fairly easy.
Comment #147
timmillwoodI have taken Catch's comments into account and rolled another patch.
Comment #148
xmacinfo:-)
Comment #149
catchThis should use #attached.
Also if it's only logging on full node pages now, shouldn't it check the view mode?
Comment #150
timmillwoodI have made sure the JS only gets added for 'full' nodes and also moved js to #attached.
Comment #151
lucascaro CreditAttribution: lucascaro commentedLooking good!
So there is no admin settings anymore, right?
a few minor details:
There is trailing space in here.
And maybe we need a @file comment here (as per #138 )
cheers
Comment #152
timmillwoodI have removed the extra white space in statistics.php.
lucascaro, nod_ and I discussed the JS @file comment on IRC and decided it was best not to add comments as it is not yet a standard and only adds to the size of the JS file being loaded.
Comment #153
lucascaro CreditAttribution: lucascaro commentedlooks good
Comment #154
catchOK this looks good now. committed/pushed to 8.x, thanks!
Comment #155
tstoecklerThis makes it impossible to override statistics.module from contrib.
Marking "needs review" at least for that detail.
Comment #156
timmillwood@tstoeckler: because this patch has now been committed, please open a new issue about this.
Comment #157
timmillwoodI am new moving this issue to D7 to work on back porting this patch.
Comment #158
wiifmAs module maintainer for statistics_ajax, I applaud you @timmillwood for spending so much time getting this into core!
Are you okay with the backport of this code to D7? i can help out if you have not already started ;)
Comment #159
timmillwood@wiifm: I am hoping this D8 patch will just work in Drupal 7, the only difference is the file paths. If you are able to test the patch with D7 before I can that would be great.
Comment #160
wiifmAttached is #152 ported to D7 (verbatim with path changes etc). Initial testing showed the statistics updating the {node_counter} table as expected. I just hope the test bot is happy as well
Comment #161
wiifmIt seems my editor also stripped a few bonus trailing spaces as well. Hope this is okay still
Comment #163
RobLoachComment #164
tstoecklerWhile I find it weird to open a follow-up for a regression that was introduced ~2 sec ago :), I don't really care: #1549064: Statistics module is not overridable by contrib
Comment #165
timmillwoodThe patch from #160 works fine for me, therefore there might be an issue in the test.
Looking into it.
Comment #166
timmillwoodIt looks like the statistics.test patch doesn't work right, the rest of the patch is ok.
The D7 test should be ok, just needs the drupal_http_request() added.
Comment #167
wiifmI must confess, the D8 patch did not apply cleanly to the D7 .test file so I did manually patch it, from the testbot failures they should be easy to fix up.
Comment #168
wiifmNew patch that hopefully passes all the tests
Comment #170
wiifmOkay, another try
Comment #171
wiifmsetting status
Comment #172
timmillwoodLooks good, I will try to give it a test today.
Comment #173
timmillwoodLooks good to me!
Comment #174
timmillwood#170: statistics-ajax-1209532-170.patch queued for re-testing.
Comment #175
webchickthis looks great!! however, i think i'm going to hold it until next release just to be on the safe side. ideally this would get more than 2 days' testing before being available in the wild.
Comment #176
timmillwoodPerfect, I would be a bit nervous putting it in with only 2 day to go as well.
Comment #177
RobLoachWe could open up http://drupal.org/project/statisticstng or something similar.
Comment #178
timmillwoodthere are already contrib modules that do similar, would be great to have it in core, wouldn't it?
Comment #179
xmacinfo@Rob Loach: There is no need to create a contrib project. webchick only suggests to wait until after the next Drupal point release before committing this patch. She left it at RTBC. :-)
Comment #180
timmillwoodReady for a D7 commit? It'd be great to get some eyes on this before the next release.
Comment #181
David_Rothstein CreditAttribution: David_Rothstein commentedThis looks like a great improvement, and I think we should get it into Drupal 7 (it's really a borderline bugfix, even).
However, for Drupal 7, I don't think we can make this the default behavior; rather, it should be opt-in. That is per the backport policy at http://drupal.org/node/1527558, but also, in this specific case, I'm imagining a live site running behind Varnish with the Statistics module already enabled (yes, it doesn't work correctly in this setup, but that doesn't mean people don't have it turned on). After the next version of Drupal is released with this patch, every request (to a node page) that hits Varnish is now suddenly going to trigger a direct AJAX call to the Drupal site also, right? That is the kind of thing that can bring down web servers, so I don't think we can spring it on people by default.
The good news is, I don't think it would be too hard to modify the patch to add a variable which controls whether the old or new behavior is used. And given that the statistics module admin page is not exactly a heavily-trafficked part of the Drupal UI, we could probably get away with adding a new checkbox on that page in Drupal 7. Does that make sense?
In addition, some other issues with the patch:
I think we should address #1549064: Statistics module is not overridable by contrib before getting this patch in Drupal 7. I don't know if it's actually an important issue or not, but I don't want to find out later that it actually was :)
And it's hopefully a really easy fix; can't we just call url() on the PHP side, then add a JavaScript setting that contains the final URL? I think that's the standard way of doing this, and it would make the code cleaner also.
This looks like an unintended (?) behavior change to me. The view mode can certainly be 'full' even if you aren't on the node/$nid page (think Views). I think you need to check node_is_page($node) also; see examples elsewhere in core. (And perhaps you need to check
empty($node->in_preview)
too, although I'm not sure about that one.)This looks like it will blow away any JavaScript that was attached by an earlier hook implementation.
Also, a couple more things that aren't quite bugs, but seemed curious:
@catch asked this above and I'm not sure I saw a response; why isn't this using Drupal.behaviors?
This type of code is found throughout the tests. Is there a reason it can't use $this->drupalPost()? If it can, that would be a lot simpler.
Comment #182
David_Rothstein CreditAttribution: David_Rothstein commentedSeveral of the issues above affect Drupal 8 also, so I'm moving this back there for now.
Comment #183
nod_just replying for 4. It's because you want to log the view on page load, not on every ajax call that can be triggered afterwards (which is what Drupal.behaviors will do). Behaviors are not well suited for this kind of things: we'd be adding 4-something lines of code to work around what behaviors do, might as well not use it and make the code simpler.
Comment #184
droplet CreditAttribution: droplet commented- What is the point of use AJAX ? Better Permanence? If DB under high load, it adds more overhead. (to both PHP server and HTTP server)
- #1684976: Better validation for statistics.php
- should not use db_merge. It can fake the script to accept any NID. even node is not existence. (myself consider its a security issue)
Comment #185
nod_AJAX if for having relevant stats when you're using something like varnish or even the drupal cache for anonymous pages. That's pretty much the only simple and reliable way to go about that. You can't really avoid the overhead, usually google analytics takes it.
The issues with the code are valid though.
Comment #186
timmillwoodHere's a bunch of changes based on David_Rothstein's suggestions.
Comment #187
tstoecklerI think node_is_page() already contains a check for $view_mode == 'full' (not sure, though). Maybe we want to leave this check specifically anyway, if we specifically want to check for that view mode, but I don't think so.
Since we now moved the path to PHP can we use drupal_get_path() here? That would solve #1549064: Statistics module is not overridable by contrib
Marking needs work for the second item.
Comment #188
timmillwoodMakes sense!
Comment #189
tstoecklerCool, marked #1549064: Statistics module is not overridable by contrib as duplicate.
Comment #190
timmillwoodComment #191
David_Rothstein CreditAttribution: David_Rothstein commentedI think the $view_mode == 'full' check is actually necessary here, since the same node can easily be displayed twice on a page (e.g., the primary view plus a sidebar view). Anyway, every other call to node_is_page() in core checks $view_mode also (yes, perhaps there should be a helper function for that), so no reason to be different here :)
While rerolling, might as well add a comma at the end of this line (per coding standards).
Comment #192
timmillwood@david_rothstein, thanks for the feedback, rerolled.
Comment #193
nod_Any reason to hardcode the nid parameter instead of just putting
data: Drupal.settings.statistics
? and filter the data in the PHP side of things?Comment #194
timmillwoodDrupal.settings.statistics would make it more flexible wouldn't it?
Comment #195
nod_yep, jquery takes care of building the post string from the object too.
Comment #196
timmillwoodAdding in suggestion from nod_
Comment #197
mikeytown2 CreditAttribution: mikeytown2 commentedComing from #1407524-2: Front End Speed Tweak: Speed up $.post() callback by returning before booting up drupal we can speed up the code a little bit. Patch attached to do that.
Comment #198
jcisio CreditAttribution: jcisio commentedAbout #197: it is ok for D7, but I doubt it is suitable for D8. I think statistics.php will disappear, we'll use the kernel. However I don't know how we apply this trick into Drupal's HttpKernel.
Comment #199
timmillwoodmoved the nid to a data in the JS rather than sending all Drupal.settings.statistics.
Comment #200
timmillwoodComment #201
catch#199: 1209532-199.patch queued for re-testing.
Comment #203
timmillwoodnod_ has been making awesome JS changes again, but these caused my patch not to work, here's a new one.
Comment #204
nod_haha sorry about that. Patch looks good :)
Comment #205
timmillwoodMarking as RTBC then!
Comment #206
nod_There is just one extra "," after Drupal.settings.statistics.data in statistics.js though, beside that it's RTBC I agree :)
Comment #207
timmillwoodRemoved extra comma
Comment #208
timmillwoodPassed tests so marking at RTBC
Comment #209
webchickGosh, I'm really sorry. I have NO idea why this was off my radar for so long. :(
I confess that I don't fully grok this, but it has the +1 of our JS maintainer, so I guess it's good to go. :)
Committed and pushed to 8.x. Thanks! Marking for D7 backport, although it's worth checking w/ David to find out how much of this is actually backportable.
Comment #210
timmillwood@webchick - this patch is really just a few improvements over the original patch which had been committed. No problems with the delay getting this committed, thanks!
Time to work on the D7 version now.
Comment #211
David_Rothstein CreditAttribution: David_Rothstein commentedSince @webchick asked, I'll just point out #181 again... In my opinion, this patch should be backportable as long as in Drupal 7 it's not the default behavior.
Comment #212
webchickOops. That'll learn me to read things before asking dumb questions. Thanks, David!
Comment #213
mikeytown2 CreditAttribution: mikeytown2 commentedWhat about getting #197 in D8 so we can get it in D7?
Comment #214
iamEAP CreditAttribution: iamEAP commentedFor those uninterested in reading through the full history of this ticket, a D7 backport needs to comprise both #152 and #207 with a checkbox and variable to opt-in to (enable) the new functionality (#181).
Mike, sounds like you might be better off creating a followup task.
Comment #215
iamEAP CreditAttribution: iamEAP commentedTest changes made for D8 are largely irrelevant, since the AJAX setting is opt-in for D7. Instead, I just added additional tests:
As per #181, I added a variable and checkbox on the admin form. As David pointed out, this breaks string freeze, but I'll leave it up to him to decide if that's acceptable. Certainly open to feedback on the wording, there.
Added a variable delete in statistics.install for the new variable. Believe once this gets committed, we should add an update function in 8.x to remove the new variable, since it has no meaning there.
Comment #217
iamEAP CreditAttribution: iamEAP commentedWas not reading the tests very well when I wrote them.
Comment #219
iamEAP CreditAttribution: iamEAP commented#217: drupal-1209532-statistics_via_ajax-217.patch queued for re-testing.
Comment #220
iamEAP CreditAttribution: iamEAP commented#217: drupal-1209532-statistics_via_ajax-217.patch queued for re-testing.
Comment #221
mikeytown2 CreditAttribution: mikeytown2 commented@iamEAP
Checkout returning data to browser ASAP in #197. Reports of the browser taking 90ms to get URL without patch, 65ms with patch. I have this logic in a function httprl_background_processing(). The main difference between the patch in #197 and httprl_background_processing is the call to fastcgi_finish_request() which is available on php-fpm only.
Comment #222
David_Rothstein CreditAttribution: David_Rothstein commented@mikeytown2: Sounds like that should be a separate issue?
Also, I remember discussing that kind of technique extensively in #566494: Running cron in a shutdown function breaks the site a long time ago... it looked promising, but as I recall there were many webserver setups where it didn't work correctly. If it can be made to work widely, though, it's pretty neat.
Comment #223
tcmug CreditAttribution: tcmug commentedI wonder if this is kind approach is only of superficial on the server level as the speed is only visible to the user/browser; wont the child process left running the PHP after the connection is closed still take up a connection slot?
If the connection is closed in a totally awesome 10 ms (instead of the usual 500 ms) the server is still busy for the entire 500 ms and unable to process any requests -this sounds kind of bad to my ear.
Comment #224
jcisio CreditAttribution: jcisio commented#223 but both (client and server) don't have to manage that connection, it will free more resource.
Comment #225
tcmug CreditAttribution: tcmug commentedWell the remaining PHP after connection closing still has to be executed so ofcourse it keeps the child process executing (atleast apache). nginx might free the actual nginx process but php-fpm process keeps executing and keepin it occupied.
Comment #226
sdrycroft CreditAttribution: sdrycroft commentedAs far as I can see, there are currently two issues with the patch from #217:
When executing getcwd() from /var/www/web_app_2/index.php /usr/share/drupal-7 is returned. When executing it from /var/www/web_app_2/modules/statistics/statistics.php /var/www/web_app_2/modules/statistics is returned and not /usr/share/drupal-7/modules/statistics.
I am aware that the second issue could easily be resolved by symlinking to files only, but it's still an issue that we need to be aware of.
Comment #227
mikeytown2 CreditAttribution: mikeytown2 commentedgetcwd() could be worked around if using PWD. PWD doesn't work on windows but it's not an issue due to symlinks on windows being 100 times harder to setup.
Check both:
$_SERVER['PWD']
$_ENV['PWD']
Background info #1878454-22: When does something happen?
Comment #228
sdrycroft CreditAttribution: sdrycroft commented$_SERVER['PWD'] is only available when executing PHP from the command line, so that solution will not work. I have tweaked iamEAP's #217 patch to attempt to calculate the correct DRUPAL_ROOT using $_SERVER['SCRIPT_FILENAME'].
Comment #229
JeremyFrench CreditAttribution: JeremyFrench commentedPatch in #228 applies and works for me behind varnish.
Comment #230
davidwhthomas CreditAttribution: davidwhthomas commentedThis is a great enhancement to the core statistics module, allowing statistics to work well on cached pages.
I'm successfully using the D7 patch in #228
Here's a code snippet for anyone looking how to get it working when using Panels for node_view.
There's probably a few ways to do it, I was hoping to use #attached somewhere here, but this works:
Comment #231
Rosamunda CreditAttribution: Rosamunda commentedIs #228 commited?
Comment #232
JordanMagnuson CreditAttribution: JordanMagnuson commentedI'm also wondering if this has been/will be committed?
Comment #233
iamEAP CreditAttribution: iamEAP commentedRe #231, 232: This will only be considered for commit once it's marked "reviewed and tested by the community." If you've deployed this patch, or have otherwise tested, please feel free ti chime in and/or flip that issue status.
I see #229 and #230 have positive reviews.
Also, un-assigning Tim so this is seen more widely.
Comment #234
sdrycroft CreditAttribution: sdrycroft commentedWe're currently using this patch on approximately 500 sites, and have not experienced any issues.
Comment #235
JordanMagnuson CreditAttribution: JordanMagnuson commented@sdrycroft: That sounds pretty RTBC, then.
Comment #236
David_Rothstein CreditAttribution: David_Rothstein commentedLooks great now. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/1bcc0e3
I did change "AJAX" to "Ajax" (especially in the user-facing text) on commit, to match coding standards.
Ideally I wouldn't have committed this so close to the next release (due to the string change), but this patch sat for a while and it's new translatable strings only (not breaking any existing ones), plus buried deep on an administrative page, so I think it's OK.
I'm not sure how much the new checkbox description will really help people with existing sites decide whether to turn this on or not, but we need a change notification anyway so that'll help too.
Comment #237
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedI can only select Use 'AJAX to increment the counter' if I have 'Do not collect content hit statistics'.
do I do that then select one of the other options?
Edit: this is caused by better_statistics module.
Comment #238
iamEAP CreditAttribution: iamEAP commentedAdded a change notice here: https://drupal.org/node/2164069 Feedback welcome.
@SocialNicheGuru, you should only be able to select the Ajax collection method when "Count content views" is also checked.
Comment #239
David_Rothstein CreditAttribution: David_Rothstein commentedLooks great, thanks!
The one thing I thought was missing was information about how this affects sites using Varnish (or similar external caches). Because those are the sites that can get the greatest benefit from this new option, but also the greatest risk. So I added a paragraph about that now: https://drupal.org/node/2164069/revisions/view/6801571/6804873
If that looks good to others, I think we can close this issue.
Comment #240
iamEAP CreditAttribution: iamEAP commentedBelieve that covers it; marking fixed and reverting back to prior issue details. Keeping 7.25 release notes and 7.25 release blocker because I'm unsure of their meaning / intent.
Comment #241
iamEAP CreditAttribution: iamEAP commentedLooks like this wasn't quite ready for prime-time; if you have multiple languages enabled and you're using path-based language negotiation, no statistics are collected for languages with configured path prefixes. It also generates a 404 log entry for each failed attempt.
The data collection callback should not be passed through url(). This issue exists in D8 too. I've created an issue here and tagged it as "needs backport." #2172871: Async node hit counter data collection is broken when path-based language negotiation is used
Comment #243
math_1048 CreditAttribution: math_1048 commentedThat's Great, Thanks for you,
What about to displaying updated {node_count} value using ajax ... my cache module is "BOOST"?
can you help me please in this issue
Thanks and regards
Comment #244
Andre-BI guess quiet a lot of people might stumble in this as well:
I am using Panels based setup, where I have page overrides for various node pages, I use a different view mode than "full" on these variants - for those view modes the statistics are not counted, since it's only added on the full view mode - is this intended behavior?
Comment #245
WillowDigit CreditAttribution: WillowDigit commentedI have encountered a new issue in statistics.php file.
My statistics counters do not increment. I am running on IIS and the define at the top
define('DRUPAL_ROOT', substr($_SERVER['SCRIPT_FILENAME'], 0, strpos($_SERVER['SCRIPT_FILENAME'], '/modules/statistics/statistics.php')));
is not platform directory separator aware.
It should be something like:
Comment #246
mikeytown2 CreditAttribution: mikeytown2 commentedNormalizing the variable is a better option.
Using DIRECTORY_SEPARATOR is another way.
Comment #247
david_garcia CreditAttribution: david_garcia commentedCurrent implementation not working on IIS, I made a patch and tested based on @mikeytown2 proposal.
Comment #248
mikeytown2 CreditAttribution: mikeytown2 commentedRe-opening the issue because of the windows bug.
Edit: Looks like #247 passes the test bot.
Comment #249
David_Rothstein CreditAttribution: David_Rothstein commentedLet's move that to the separate issue which was filed for it - #2327953: statistics.php is not platform directory separator aware - since this issue was fixed long ago.
(I'll leave a comment there pointing to the patch.)
Comment #250
David_Rothstein CreditAttribution: David_Rothstein commentedComment #251
timmillwoodIs this a bug on both the D7 and D8 versions of the code?
Comment #252
David_Rothstein CreditAttribution: David_Rothstein commentedI didn't test, but I don't think so because D8 isn't doing anything with
$_SERVER['SCRIPT_FILENAME']
in this file.Comment #254
ruloweb CreditAttribution: ruloweb commentedFor panels issues check #2437773: Attached CSS and JS files are not loaded
Comment #255
ruloweb CreditAttribution: ruloweb commented