| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | statistics.module |
| Category: | task |
| Priority: | normal |
| Assigned: | timmillwood |
| Status: | needs review |
| Issue tags: | needs backport to D7 |
Issue Summary
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?
Comments
#1
http://drupal.org/node/1018716
#2
I like the idea of making it a pluggable framework; I think that's been proposed for Search module as well?
#3
Seriously? 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.
#4
Malware wanring for the OWA site :(
www.openwebanalytics.com contains malware. Your computer might catch a virus if you visit this site.
#5
@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.
#6
A 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.
#7
A 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.
#8
I 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.
#9
It's good but my issue is that Radioactivity module is just way better and already in contrib...
#10
Radioactivity is overkill. We need to simplify statistics to keep only the basic view counter. The rest will should go to contrib.
#11
+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.
#12
Honestly, 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.
#13
@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.
#14
@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.
#15
I 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.
#16
@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.
#17
@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.
#18
I 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.
#19
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. 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.
#20
#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.
#21
I 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?
#22
Instead of improving the Statistics module, what if we were to improve the Radioactivity module?
#23
Great idea Rob, created another issue to track this idea. http://drupal.org/node/1277710
/me goes to install Radioactivity module.
#24
Maybe 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.
#25
@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.
#26
#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.
#27
One 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.
#28
Semantically, 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.
#29
Actually, 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.
#30
In a way, this could be like the current tripled of
includes/bootstrap.inc#watchdog()vsmodules/dblogvsmodules/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
#31
@tcmug: “…(unwritten) plans for 3.x”. Well, can someone actually write the plan? :-)
#32
Sort 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
#33
Well, 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.
#34
See also #1120928: Move "history" table into separate History module.
#35
@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.
#36
Hi,
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
#37
Step 1) merge http://drupal.org/project/statistics_ajax into the statistics module so that it works with Varnish.
Step 2) reduce database writes.
#38
There 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).
#39
jstats 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.
#40
I 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.
#41
My 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.
#42
Radioactivity 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.
#43
Store 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.
#44
We 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...)
#45
Here'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.
#46
Adding need's review
#47
Please 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().
#48
My 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?
#49
The last submitted patch, statistics-addajax-1209532-45.patch, failed testing.
#50
@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
#51
I have now added the caching of statistics get, the cache is then set to clear every cron run.
#52
#53
The last submitted patch, statistics-addcache-1209532-51.patch, failed testing.
#54
Just noticed I managed to include my statistics and taxonomy patch into one.
Here's the real addcache patch.
#55
Just noticed I managed to include my statistics and taxonomy patch into one.
Here's the real addcache patch.
#56
The last submitted patch, statistics-addcache-1209532-54.patch, failed testing.
#57
urgh, just noticed that the cache is caching the same number of views for all nodes!
*goes back to work*
#58
caching is a little better now.
#59
The last submitted patch, statistics-addcache-1209532-58.patch, failed testing.
#60
I 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.
#61
Node 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.
#62
I'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?
#63
I 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.
#64
I 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).
#65
queues vs cache?
#66
I 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.
#67
Good 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?
#68
The last submitted patch, statistics-addajax-1209532-67.patch, failed testing.
#69
See 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.
#70
About the js, as it is now it'll run multiple time if there are other drupal ajax calls on the page,
(function ($) {var run = false;
Drupal.behaviors.statisticsModule = {
attach: function (context, settings) {
if (!run) {
var basePath = settings.basePath
$.ajax({
type: "POST",
cache: false,
url: basePath+"core/modules/statistics/statistics.php",
data: settings.statistics,
});
run = true;
}
}
};
})(jQuery);
#71
Well, that's a bit stupid. Whys that?
May be we should fix it rather than use the 'run' workaround.
#72
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.
settingsis reallyDrupal.settingsand it should already be available, just make sure you include that in the footer.#73
+ drupal_add_js(array('statistics' => array('nid' => arg(1))), 'setting');Just 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:
+ drupal_add_js(array('statistics' => array('nid' => array(1, 2, 3))), 'setting');#74
Joachim,
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.
#75
Quite 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)
#76
The last submitted patch, statistics-ajax-1209532-75.patch, failed testing.
#77
Forgot to add statistics.php and statistics.js
#78
Sorry, did it again
#79
The last submitted patch, statistics-ajax-1209532-77.patch, failed testing.
#80
Don't forget blank like at end of files. :-)
#81
> 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.
#82
I 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.
#83
I 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.
#84
This 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.
#85
This 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.
#86
The patch did not attach properly
#87
Trying that again.
#88
Please ignore the patches in 84, 85 and 86.
Patch in 87 has passed, would be good to get some reviewers.
#89
The 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.
#90
Aww man! I really need to check / test my patches better.
Thanks for looking.
#91
I 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.
#92
The last submitted patch, statistics-ajax-1209532-91.patch, failed testing.
#93
Fails due to search.test?
WTF?
#94
I hope this is configurable. Usually we want to add only full nodes to the counter.
#95
Yes, 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.
#96
@timmillwood: awesome!
#97
#98
#91: statistics-ajax-1209532-91.patch queued for re-testing.
#99
The last submitted patch, statistics-ajax-1209532-91.patch, failed testing.
#100
Hi, 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
#101
The last submitted patch, statistics-ajax-1209532-99-2.patch, failed testing.
#102
Corrected 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!
#103
Hi 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.
--- /dev/null+++ b/core/modules/statistics/statistics.php
@@ -0,0 +1,34 @@
+<?php
+
+// Change the directory to the Drupal root.
+chdir('../../..');
+if (empty($_POST['nids'])) {
+ exit();
+}
+/**
+ * Root directory of Drupal installation.
+ */
+define('DRUPAL_ROOT', getcwd());
+
+include_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
+drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES);
+if (variable_get('statistics_count_content_views', 0)) {
+ $nids = json_decode($_POST['nids']);
+ watchdog('statistics', $_POST);
+ if (is_array($nids)) {
+ foreach ($nids as $nid) {
+ if (is_numeric($nid)) {
+ db_merge('node_counter')
+ ->key(array('nid' => $nid))
+ ->fields(array(
+ 'daycount' => 1,
+ 'totalcount' => 1,
+ 'timestamp' => REQUEST_TIME,
+ ))
+ ->expression('daycount', 'daycount + 1')
+ ->expression('totalcount', 'totalcount + 1')
+ ->execute();
+ }
+ }
+ }
+}
Maybe these two lines fix that?
+++ b/core/modules/statistics/statistics.php@@ -0,0 +1,34 @@
+include_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
+drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES);
I just wanted to have someone else look at this just in case.
Also, what would happen when nodes are loaded via ajax?
Cheers!
#104
It 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)
#105
I 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.
#106
@timmillwood, @chx great. RTBC then?
#107
I 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.
#108
(for the tests)
#109
Added the tests back in, which are working better, but not perfectly.
#110
The last submitted patch, statistics-ajax-1209532-109.patch, failed testing.
#111
Hope this one works better for you.
#112
fixing path variable in tests.
#113
reroll with minor spacing corrections and patching search.test
#114
There 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'.
#115
rerolled again, this time includen the patch to search.test by @timmillwood and the fixes suggested by @Lars Toomre
#116
#117
Hmmm... 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?
#118
fixing the indentation and spacing issues.
#119
just for the test
#120
marking as RTBC since it's been checked by various users, and the tests are back in.
#121
So nice to finally see this RTBC!
#122
Something funny is happening with testbot... Only passes 11 tests?
Setting back to needs review until all tests pass.
#123
#118: statistics-ajax-1209532-118.patch queued for re-testing.
#124
Some more coding nits...
+++ b/core/modules/statistics/statistics.jsundefined@@ -0,0 +1,12 @@
+ });
+ });
The indentation of the first set is one too much it seems.
+++ b/core/modules/statistics/statistics.moduleundefined@@ -115,6 +98,18 @@ function statistics_permission() {
+ if(!empty($nids_json)) {
There needs to be a space between if and '('.
+++ b/core/modules/statistics/statistics.testundefined@@ -104,6 +104,13 @@ class StatisticsLoggingTestCase extends DrupalWebTestCase {
+ drupal_http_request($stats_path, array('method' => 'POST','data' => $post,'headers' => $headers,'timeout' => 10000));
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.
#125
#118: statistics-ajax-1209532-118.patch queued for re-testing.
#126
When 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.
#127
The last submitted patch, statistics-ajax-1209532-118.patch, failed testing.
#128
Fixed the block test, forgot to call statistics.php, also tidied up some of the code.
#129
#130
Once committed to D8 I would like to re-roll this for D7.
#131
Hey @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:
+++ b/core/modules/statistics/statistics.php@@ -0,0 +1,33 @@
\ No newline at end of file
two extra spaces:
+++ b/core/modules/statistics/statistics.php@@ -0,0 +1,33 @@
+ 'daycount' => 1, ¶
+ 'totalcount' => 1, ¶
also here
+++ b/core/modules/statistics/statistics.js@@ -0,0 +1,12 @@
+})(jQuery);
\ No newline at end of file
Also, according to http://drupal.org/coding-standards#phptags the closing ?> should be removed in statistics.php
+++ b/core/modules/statistics/statistics.php@@ -0,0 +1,33 @@
+?>
what do you think?
#132
@lucascaro: You are right. All the points you highlight should be fixed. Thanks for checking these out.
#133
Fixing @lucascaro's suggestions, but... also making the new checkbox only appear when the old one is checked using states.
#134
Thanks for the code clean up. It looks good except search.test which is missing spaces in the array definition there.
#135
Re-roll for Lars
#136
changing status
#137
#138
+++ b/core/modules/statistics/statistics.jsundefined@@ -0,0 +1,12 @@
+(function ($) {
+ $(document).ready(function() {
+ var nids = Drupal.settings.statistics.nids;
+ var basePath = Drupal.settings.basePath
+ $.ajax({
+ type: "POST",
+ cache: false,
+ url: basePath+"core/modules/statistics/statistics.php",
+ data: "nids="+nids
+ });
+ });
+})(jQuery);
This 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?
+++ b/core/modules/statistics/statistics.admin.incundefined
@@ -269,6 +269,17 @@ function statistics_settings_form() {
+ $form['content']['statistics_count_only_full'] = array(
+ '#type' => 'checkbox',
+ '#title' => t('Count full nodes only'),
+ '#default_value' => variable_get('statistics_count_only_full', 1),
+ '#description' => t('Count views of only whole content and not as part of blocks, lists or teasers.'),
+ '#states' => array(
+ 'visible' => array( // action to take.
+ ':input[name=statistics_count_content_views]' => array('checked' => TRUE),
+ ),
+ ),
+ );
return system_settings_form($form);
}
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.
'visible' => array( // action to take.Comment should be above the line.
+++ b/core/modules/statistics/statistics.jsundefined
@@ -0,0 +1,12 @@
+(function ($) {
+ $(document).ready(function() {
+ var nids = Drupal.settings.statistics.nids;
+ var basePath = Drupal.settings.basePath
+ $.ajax({
+ type: "POST",
+ cache: false,
+ url: basePath+"core/modules/statistics/statistics.php",
+ data: "nids="+nids
+ });
+ });
+})(jQuery);
This 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?
<code>
+ global $nids_json;
+ if (!empty($nids_json)) {
+ $nids = drupal_json_decode($nids_json);
+ }
+ $nids[] = $node->nid;
+ $nids_json = drupal_json_encode($nids);
+ drupal_add_js(drupal_get_path('module', 'statistics') . '/statistics.js', array('scope' => 'footer'));
+ drupal_add_js(array('statistics' => array('nids' => $nids_json)), 'setting');
+ }
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.
#139
Using 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.
#140
I 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.
#141
@timmillwood: when a node is fetched (via AJAX) and display in, e.g., a modal frame, does it count?
#142
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.
#143
I'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.
#144
We 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.
#145
data-attribute, much better than a class or whatever. there is an issue somewhere in the queue to remove as much stuff fromDrupal.settingsas possible and replace with data- attributes on the relevant element.#146
@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.
#147
I have taken Catch's comments into account and rolled another patch.
#148
:-)
#149
+ if (!empty($node->nid)) {+ drupal_add_js(drupal_get_path('module', 'statistics') . '/statistics.js', array('scope' => 'footer'));
+ drupal_add_js(array('statistics' => array('nid' => $node->nid)), 'setting');
+ }
This should use #attached.
Also if it's only logging on full node pages now, shouldn't it check the view mode?
#150
I have made sure the JS only gets added for 'full' nodes and also moved js to #attached.
#151
Looking good!
So there is no admin settings anymore, right?
a few minor details:
+++ b/core/modules/statistics/statistics.php@@ -0,0 +1,33 @@
+ } ¶
There is trailing space in here.
+++ b/core/modules/statistics/statistics.js@@ -0,0 +1,12 @@
+(function ($) {
+ $(document).ready(function() {
+ var nid = Drupal.settings.statistics.nid;
+ var basePath = Drupal.settings.basePath
+ $.ajax({
+ type: "POST",
+ cache: false,
+ url: basePath+"core/modules/statistics/statistics.php",
+ data: "nid="+nid
+ });
+ });
+})(jQuery);
And maybe we need a @file comment here (as per #138 )
cheers
#152
I 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.
#153
looks good
#154
OK this looks good now. committed/pushed to 8.x, thanks!
#155
+++ b/core/modules/statistics/statistics.js@@ -0,0 +1,12 @@
+ url: basePath+"core/modules/statistics/statistics.php",
This makes it impossible to override statistics.module from contrib.
Marking "needs review" at least for that detail.
#156
@tstoeckler: because this patch has now been committed, please open a new issue about this.
#157
I am new moving this issue to D7 to work on back porting this patch.
#158
As 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 ;)
#159
@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.
#160
Attached 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
#161
It seems my editor also stripped a few bonus trailing spaces as well. Hope this is okay still
#162
The last submitted patch, statistics-ajax-1209532-160.patch, failed testing.
#163
#164
While 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
#165
The patch from #160 works fine for me, therefore there might be an issue in the test.
Looking into it.
#166
It 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.
#167
I 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.
#168
New patch that hopefully passes all the tests
#169
The last submitted patch, statistics-ajax-1209532-168.patch, failed testing.
#170
Okay, another try
#171
setting status
#172
Looks good, I will try to give it a test today.
#173
Looks good to me!
#174
#170: statistics-ajax-1209532-170.patch queued for re-testing.
#175
this 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.
#176
Perfect, I would be a bit nervous putting it in with only 2 day to go as well.
#177
We could open up http://drupal.org/project/statisticstng or something similar.
#178
there are already contrib modules that do similar, would be great to have it in core, wouldn't it?
#179
@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. :-)
#180
Ready for a D7 commit? It'd be great to get some eyes on this before the next release.
#181
This 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:
+ url: basePath+"modules/statistics/statistics.php",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.
- if (arg(0) == 'node' && is_numeric(arg(1)) && arg(2) == NULL) {- // A node has been viewed, so update the node's counters.
...
function statistics_node_view($node, $view_mode) {
+ if (!empty($node->nid) && $view_mode == 'full') {
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.)function statistics_node_view($node, $view_mode) {...
+ $node->content['#attached']['js'] = array(
+ drupal_get_path('module', 'statistics') . '/statistics.js' => array(
+ 'scope' => 'footer'
+ ),
+ );
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:
+ $(document).ready(function() {@catch asked this above and I'm not sure I saw a response; why isn't this using Drupal.behaviors?
+ // Manually calling statistics.php, simulating ajax behavior.+ $nid = $node->nid;
+ $post = http_build_query(array('nid' => $nid));
+ $headers = array('Content-Type' => 'application/x-www-form-urlencoded');
+ global $base_url;
+ $stats_path = $base_url . '/' . drupal_get_path('module', 'statistics'). '/statistics.php';
+ drupal_http_request($stats_path, array('method' => 'POST', 'data' => $post, 'headers' => $headers, 'timeout' => 10000));
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.
#182
Several of the issues above affect Drupal 8 also, so I'm moving this back there for now.
#183
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.
#184
- 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)
#185
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.
#186
Here's a bunch of changes based on David_Rothstein's suggestions.
#187
+++ b/core/modules/statistics/statistics.module@@ -102,13 +102,12 @@ function statistics_permission() {
+ if (!empty($node->nid) && $view_mode == 'full' && node_is_page($node) && empty($node->in_preview)) {
I 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.
+++ b/core/modules/statistics/statistics.module@@ -102,13 +102,12 @@ function statistics_permission() {
+ $settings = array('nid' => $node->nid, 'url' => url('core/modules/statistics/statistics.php'));
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.
#188
Makes sense!
#189
Cool, marked #1549064: Statistics module is not overridable by contrib as duplicate.
#190
#191
I 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 :)
+ 'scope' => 'footer'While rerolling, might as well add a comma at the end of this line (per coding standards).
#192
@david_rothstein, thanks for the feedback, rerolled.
#193
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?#194
Drupal.settings.statistics would make it more flexible wouldn't it?
#195
yep, jquery takes care of building the post string from the object too.
#196
Adding in suggestion from nod_
#197
Coming 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.
#198
About #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.
#199
moved the nid to a data in the JS rather than sending all Drupal.settings.statistics.
#200
#201
#199: 1209532-199.patch queued for re-testing.
#202
The last submitted patch, 1209532-199.patch, failed testing.
#203
nod_ has been making awesome JS changes again, but these caused my patch not to work, here's a new one.
#204
haha sorry about that. Patch looks good :)
#205
Marking as RTBC then!
#206
There is just one extra "," after Drupal.settings.statistics.data in statistics.js though, beside that it's RTBC I agree :)
#207
Removed extra comma
#208
Passed tests so marking at RTBC
#209
Gosh, 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.
#210
@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.
#211
Since @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.
#212
Oops. That'll learn me to read things before asking dumb questions. Thanks, David!
#213
What about getting #197 in D8 so we can get it in D7?
#214
For 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.
#215
Test 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.
#216
The last submitted patch, drupal-1209532-statistics_via_ajax-215.patch, failed testing.
#217
Was not reading the tests very well when I wrote them.
#218
The last submitted patch, drupal-1209532-statistics_via_ajax-217.patch, failed testing.
#219
#217: drupal-1209532-statistics_via_ajax-217.patch queued for re-testing.
#220
#217: drupal-1209532-statistics_via_ajax-217.patch queued for re-testing.
#221
@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.
#222
@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.
#223
I 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.
#224
#223 but both (client and server) don't have to manage that connection, it will free more resource.
#225
Well 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.
#226
As far as I can see, there are currently two issues with the patch from #217:
/usr/share/drupal-7
index.php
modules
includes
...
/var/www/web_app_1
index.php -> /usr/share/drupal-7/index.php
modules -> /usr/share/drupal-7/modules
...
sites -> /var/lib/drupal-7/web_app_1
/var/www/web_app_2
index.php -> /usr/share/drupal-7/index.php
...
sites -> /var/lib/drupal-7/web_app_2
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.
#227
getcwd() 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?
#228
$_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'].