G.H.M. [ghm] [#835408]

CVS edit link for G.H.M.

At Groupe Hersant Media, we have been using Drupal for now more than a year, mainly to build websites for french local newspapers (http://www.lunion.presse.fr/, http://www.laprovence.com/, http://www.nicematin.com/, ...). Some of these reveive up to 1 million visits a day, which has some impact on performances.

Doing this, we have had the occasion to work with Drupal, to appreciate it and its community, and to use a large number of existing community-contributed modules. Also, we've gained experience in writing several new modules, to answer our needs and those of our websites.

Some of our needs are not specific to the websites we're working on, and we believe that sharing a couple of modules we developped could be interesting for both the community, and us.

The first module we would like to share as open source is related to the gathering of statistics. More precisely, it allows one to keep track of how many times the detail page of each node has been opened.

Drupal's statistics module only collects data when the HTML ouput for a node is generated. It does not suit our needs, as our web-servers are behind a reverse-proxy that is used as cache for an important portion of our page views -- which means the PHP code is often not executed, and the statistics would not be gathered.

To deal with this situation, we have created a new module, that we've called bg_stats. In a few words :

* It adds an img tag to the HTML code of the nodes pages.
* This img tag is used to load a PHP page on our server, passing it some data to indicate from which node it's called.
* That PHP script (independent from Drupal, in order to be as lightweight as possible) logs accesses to a file (and not a database, for the same reason).
* And, finally, there's a cronjob that aggregates data from this file, and uses them to populate the statistics tables.

It means statistics cannot be seen immediatly... But it also means statistics will work in a setup where the PHP servers are behind a reverse-proxy that acts as a caching layer.

Obtaining a CVS account is the first step in the process of sharing and open-sourcing a module... So, here we are !

Comment	File	Size	Author
#4	bg_stats.zip	5.4 KB	G.H.M.
#1	bg_stats.zip	5.27 KB	G.H.M.

Comments

Comment #1

G.H.M. commented 23 June 2010 at 07:47

Status:

Postponed (maintainer needs more info)

» Needs review

Status	File	Size
new	bg_stats.zip	5.27 KB

Adding zip containing our "bg_stats" module.

Comment #2

avpaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

commented 23 June 2010 at 11:41

Issue tags:

+Module review

Comment #3

avpaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

commented 30 August 2010 at 01:10

Assigned:	Unassigned	» avpaderno
Status:	Needs review	» Needs work

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

The points reported in this review are not in order or importance / relevance.
Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

The version line needs to be removed from the .info file.
```
require dirname(__FILE__) . '/_config.php';
```
There is a Drupal function to use in such cases.
It usually not a good idea to unconditionally include code contained in PHP files. If the files contain form handlers, theme functions, or menu callbacks, Drupal core code allows to third-party modules to define which files must be loaded in the implementations of hook_menu(), and hook_theme(); in these cases, Drupal loads the file only when necessary.

/*
 * Implementation of hook_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL)
 */

Hook implementation comments should be like the following one:

/**
 * Implements hook_menu().
 */

    drupal_add_js('
    </script>
    	' . $result_html . '
    <script>', 'inline', 'footer');

The first argument of drupal_add_js() is JavaScript code, without any <script> tag.

header("Content-Type: image/gif");
header("Cache-Control: no-cache, must-revalidate, post-check=0, pre-check=0, max-age=1");

The function to use is drupal_set_header().

Comment #4

G.H.M. commented 9 September 2010 at 09:52

Status:

Needs work

» Needs review

Status	File	Size
new	bg_stats.zip	5.4 KB

Hello,

Thanks for the review!

I'm attaching a new version of the module, in which I've tried to fix as much as possible :-)
=> I'm switching the status of this issue back to "Needs review"

For a bit more informations:

1. fixed

2. fixed (using module_load_include, and not require)

3. fixed -- and checked if the same mistake was made elsewhere; doesn't seem so.

4. fixed : now using hook_footer() to add the HTML code at the end of the page, instead of some dirty hack based on drupal_add_js()

5. The function drupal_set_header() looks fine to me ; but, in the register_stats.php file, we are "outside" of Drupal : this is a simple non-drupal PHP script, to avoid the overhead of loading Drupal's core and everything.
So, we can't use any drupal_* function in there -- which is why we are using header().
(I've added a comment at the top of that file, to be more explicit about that fact ; and to prevent other developpers looking at our code from thinking that not using Drupal's functions is OK)

I have also talked about your comments to other developpers arround me -- should help us all, in the future ;-)

Comment #5

avpaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

commented 9 September 2010 at 11:45

Status:

Needs review

» Fixed

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.