Pingdom is a provider of website monitoring services.

In April 2013, they launched a new service called Real User Monitoring

On signing up for this, you are presented with an inline Javascript snippet to paste into the HEAD section of the website you wish to monitor.

This module inserts that code for you.

It also presents a form for the site admin to enter the 24-character string that uniquely identifies the website to Pingdom. This is validated to ensure that only hex characters are entered (otherwise there would be a script injection vulnerability), and that string is then included at the correct point in the Javascript. Finally, it allow a site administrator to control the pages of the site that will be monitored, and to include or exclude specific user roles from having the page monitored. This last feature allows one to exclude the admin section of a site, for example, or only to monitor pageloads by the anonymous user.

The project homepage can be found at http://drupal.org/sandbox/JamesOakley/1967596
The code can be checked out with git clone --branch 7.x-1.x http://git.drupal.org/sandbox/JamesOakley/1967596.git pingdom_rum

I wrote this module to integrate that Javascript on my own websites, and want to contribute it back to the community. Inserting Script code like this is much better done with a module, because: (i) it gets done correctly, which is secure, rather than having each Drupal site administrator code their own implementation, and (ii) it provides an easy way to keep this code maintained as Pingdom continue to develop their offering. The project would be updated to keep pace with any upstream changes in the required script; (iii) it provides an easy way to include this code only on certain sections of the site.

I've checked everything against Coder Review and PAReview.

Other projects I have reviewed:

#1915476: [D6] UC Heureka
#1968868: [D7] Comment Sources
#1949352: Drupal Twitter Block
#1954036: [D7] Neutral Paths
#1946602: Feeds GCal
#1971070: [D7] Asynchronous JavaScript
#1970152: [D7] forWhereiAm Widget Integration

Comments

PA robot’s picture

PA robot CreditAttribution: PA robot commented

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented
Status: Needs review » Needs work

I've glanced through the code for the project, and found one bug (#1968108: Don't inject JS until user enters a value for RUM), and a few code style errors:

  • Make sure there's a blank newline at the end of every file.
  • One empty newline after the opening tag (<?php) at the top of the file.
  • Consider using hook_page_alter() and drupal_add_html_head() instead of hook_init() and drupal_add_js(), since it's a more flexible way of adding the JS, and will ensure the JS is added inside the <head> tags.

Other than that, things look fine, and this module does provide some value, since it will let a lot of people easily enable Pingdom's RUM on their Drupal sites and identify slow pages.

tars16’s picture

tars16 CreditAttribution: tars16 commented

I know this is small but on line 3 of the README.txt file there is a missing period.

Also, I would prefer that the menu item and URL live under 'config'(/admin/config/pingdom) rather than 'search'(/admin/search/pingdom). The Google Analytics module uses config and I think this fits better there.

alexweber’s picture

alexweber CreditAttribution: alexweber commented

@tars16 to really nitpick, the config page would probably be better off living under "admin/config/services" because it's a web service after all :)

tars16’s picture

tars16 CreditAttribution: tars16 commented

@alexweber, this is true! Services does make more sense.

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented
Status: Needs work » Needs review

Thanks everyone for your feedback.

  • JS is now only inserted when a Pingdom ID has been entered
  • A warning is also displayed to the user upon installation that the module needs this configuration step.
  • To ensure correct placement of the SCRIPT section, hook_page_alter() is now used instead of hook_init()
  • @tars16 - /admin/config would not have been right, because everything goes in a subsection of config. I put it in search, because there's something metadata-y about this, but I agree that's not quite right. GA goes in config/system, which is a bit of a catch-all when we can't think where to put it (this isn't really a system module). I agree with @alexweber that config/services is the correct place. That is where the admin page now resides

I see that the project is now 122 lines of code. Was it 120 or 125 that it needs to have in order for me to be vetted? ;-) (I still won't artificially bloat the code to try and get a few extra lines in - that's not the point!)

Marked back to "Needs Review"

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented
Issue summary: View changes

Added reference to ScriptureFilter in the hope it's a helpful cross-reference

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented
Issue tags: +PAreview: review bonus
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: single application approval

Review of the 7.x-1.x branch:

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    
FILE: /home/klausi/pareview_temp/pingdom_rum.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 72 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

Time: 0 seconds, Memory: 5.25Mb

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

  1. pingdom_rum_page_alter(): why do you use drupal_add_html_head() and not drupal_add_js()? Please add a comment.
  2. pingdom_rum_page_alter(): also, you should put your JS into a dedicated JS file and pass down variables with Drupal.settings from PHP to javascript. See http://drupal.org/node/756722
  3. This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

Otherwise I think this is pretty ready, but the drupal_add_html_head() thing should be clarified first. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

JordanMagnuson’s picture

JordanMagnuson CreditAttribution: JordanMagnuson commented

There should be options (at least custom PHP) for when to add the code, and when not to. Most people probably don't want the code added to in dev environments, or for site admins. See Google Analytics and VWO modules.

Also, if you're just adding js, and not actually altering the page as it stands so far, you should use hook_page_build() rather than hook_page_alter().

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented
Status: Needs work » Needs review
Issue tags: -PAreview: single application approval

Thank you Klausi, and JordanMagnuson. I've taken all your comments into account, and will now explain what I've changed

1. Why do I use drupal_add_html_head? I've added a comment to the function in question for future documentation. For the purposes of this issue, the reason is to guarantee that the javascript goes into the HEAD section of the document. It must be written in the HEAD section, near the top, without any aggregation or without being placed in any of the regions within the BODY section of the page. drupal_add_js adds CDATA wrappers around the script. And, unless I'm mistaken, the code could theoretically be inserted in the wrong place. This was a change made at the suggestion of geerlingguy (see #2) above.

2. I'm aware I could pass the account ID to the page using Drupal.settings. However, Pingdom specify explicitly that the code snippet must be included inline. Amongst other things, this is a tool to track the load time of secondary page elements (CSS, JS, etc.) which is why this particular snippet must not be in an external file. Also, if the resulting HTML contains the exact JS snippet that Pingdom ask the webmaster to include, Pingdom support staff will be better able to help people troubleshoot the setup. From experience making sure that my page views are coming through to my Pingdom account, the whole setup is quite particular as to how the inserted code is formatted. I've not experimented to find out exactly how fussy it is, and where I can safely change things, as it seems safer simply to ask Drupal to insert the code that Pingdom give out.

3. I've removed the "Single Project Promote" tag. The code is now 282 lines of PHP in length, and 8 functions.

I was very grateful for JordanMagnuson's suggestion that there should be a setting to determine when the code is pasted. I've looked at StatCounter, Google Analytics and Piwik - all of which do something very similar. Having worked out exactly how those functions work, and decided what this module does / doesn't need, I've produced a version of this functionality for this module.

Lastly, why don't I use hook_page_build? I'm not clear why I should. The documentation doesn't specify the exact difference between _build and _alter, but it looks like I may have problems inserting this code into the HEAD section using _build. If that really is a blocker to a full release of this module, please can someone point me to the documentation page that says precisely why that would be a necessary change?

Thanks again for your help - back to "Needs Review"

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented
Issue summary: View changes

Added other reviewed projects

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented
Issue tags: +PAreview: review bonus

Added details of 4 more projects I have reviewed.

hkirsman’s picture

hkirsman CreditAttribution: hkirsman commented

Didn't know about this cool service before. Will try it out though not on Drupal site :)

Here are my discoveries:

1.
Git clone command in the Issue Summary of this thread could have folder name parameter also included. So instead of
clone --branch 7.x-1.x http://git.drupal.org/sandbox/JamesOakley/1967596.git
one would write
clone --branch 7.x-1.x http://git.drupal.org/sandbox/JamesOakley/1967596.git pingdom_rum

2.
After installing the module, you get the text message

"The Pingdom RUM module has been installed, however it will
    do anything until you enter your Pingdom project ID...."

I'm no english expert, but I think you're missing 'not' after 'however it will"

3.
And the string above has line breaks and too many spaces in it. I think the proper thing to do here is to write the message on one single line (sometimes there are exceptions in the 80 chars per line rule). You can see the funny formating when you install the module with Drush. And I wonder how the translation interface will output this line.

This also applies all the other long strings you have in your code.

Check out any Drupal core modules. I don't think you'll find such breaking of lines ;)

4.
I have this error notice on all of my pages:
Notice: Use of undefined constant PIWIK_PAGES - assumed 'PIWIK_PAGES' in _pingdom_rum_visibility_pages() (line 100 of /home/devel/hannes/sites/all/modules/contrib/pingdom_rum/pingdom_rum.module).

Do I really have to have Piwik module installed?

5. Suggestion. Add the module to package = Statistics (That's what piwik and google_analytics have done)

Good luck!

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented

Thanks hkirsman

1. Fixed. That will help other reviewers - thank you
2. "Not". Oops. Fixed ;-)
3. The 80-char rule. I take the point about Drush. The problem is that the automated review process is taken by many to be absolute. I think it may still be desirable to wrap such messages in a controlled way, but Drush does preserve too many of the spaces. There's no clean way around this one. Have lines of more than 80 chars, PAReview spits it out. Wrap it the way I've done, it looks funny in Drush. Use string concatenation in PHP prior to running it through t(), Drush is happy, but PAReview then complains that t() should only act on string literals. My view is that the way I've got it is probably the best compromise between those.
4. I've taken that out. Thank you - the variable_get should have an empty string as its default. That was an artefact from me borrowing code from Piwik, GA and Statcounter modules. The error should now vanish.
5. Not sure. It's only half a statistics module. I prefer to leave modules in the "Other" section unless there's a clear indication it should be grouped with others.

Thanks again - and, yes, the Pingdom RUM service is a good one.

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented
Issue summary: View changes

Added 4 more projects I have recently helped by reviewing

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented
Issue summary: View changes

Added folder name 'pingdom_rum' to the "git clone" command in the issue summary.

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented
Issue summary: View changes

Removed note about the module being ordinarily too short for awarding right to "create new project". The module is longer now the page-visibility settings have been added.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

pareview.sh only complains if comment lines and arrays are longer than 80 characters (as in the Drupal codings standards), the rest can be of course longer.

hkirsman’s picture

hkirsman CreditAttribution: hkirsman commented
Status: Needs review » Reviewed & tested by the community

That was fast response :)

All has been fixed and I understand the problem with long lines and some users not understanding it. You can still fix it if you get full access ;) Klausi should comment on that if he haz the timez.

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented

@klausi - thanks for clarifying.

I've modified that one string that was being displayed to the user through Drush with the excessive spaces, and can indeed confirm that pareview.sh did not complain. Inspired by the success of that, I've done the same with the 3 other string literals that were being used as descriptions.

@hkirsman - thanks for the RTBC. Once a git maintainer moves this to fixed, you can use the module in the usual way to try out the Pingdom service - on a Drupal site!

JordanMagnuson’s picture

JordanMagnuson CreditAttribution: JordanMagnuson commented

@JamesOakley:

Your changes look good.

As far as adding js to head via drupal_add_js(), you can do that by setting the 'scope' option to 'header'. You can also prevent aggregation by setting 'preprocess' to 'FALSE'. As far as I know, this is best practice for adding js in Drupal, and can be seen in modules similar to this, such as google_analytics and visual_website_optimizer. I am not aware of a high profile module adding js via drupal_add_html_head() as per the suggestion in #2.

The difference between hook_page_alter() and hook_page_build() is explained and summarized in the docs:

If you want to alter the elements added by other modules or if your module depends on the elements of other modules, use hook_page_alter() instead which runs after this hook.

In other words, use hook_page_build() unless you need to use hook_page_alter().

Why use hook_page_build() to add js rather than hook_init() is touched on briefly here:

hook_init() runs even on AJAX requests, private file requests, etc. hook_page_build() runs when we're only delivering HTML.

Here's how I've been adding the Pingdom RUM code on my own website via a custom module:

function my_module_page_build() {
  global $conf;
  global $user;

  // Don't add on dev server, or for site admins (my own project requirements)
  if (!$conf['environment_indicator_enabled']) {
    if (!in_array('administrator', $user->roles)) {
      // See Google Analitics and VWO modules for examples
      $options = array(
        'type' => 'inline',
        'scope' => 'header',
        'preprocess' => FALSE,
      );
      drupal_add_js('var _prum={id:"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"};var PRUM_EPISODES=PRUM_EPISODES||{};PRUM_EPISODES.q=[];PRUM_EPISODES.mark=function(b,a){PRUM_EPISODES.q.push(["mark",b,a||new Date().getTime()])};PRUM_EPISODES.measure=function(b,a,b){PRUM_EPISODES.q.push(["measure",b,a,b||new Date().getTime()])};PRUM_EPISODES.done=function(a){PRUM_EPISODES.q.push(["done",a])};PRUM_EPISODES.mark("firstbyte");(function(){var b=document.getElementsByTagName("script")[0];var a=document.createElement("script");a.type="text/javascript";a.async=true;a.charset="UTF-8";a.src="//rum-static.pingdom.net/prum.min.js";b.parentNode.insertBefore(a,b)})();', $options);
    }
  }
}

Anyway, nice work on this.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented

@JordanMagnuson - I think I used drupal_add_html_head() in my example simply because that was the first thing that popped in my head :)

But drupal_add_js() is apparently not as favored as using the #attached parameter of the $page array in most circumstances D7+... that's probably the most future-proof way to do this, but any way works equally well, imo.

JordanMagnuson’s picture

JordanMagnuson CreditAttribution: JordanMagnuson commented

But drupal_add_js() is apparently not as favored as using the #attached parameter of the $page array in most circumstances D7+

Interesting. Can you point to some support for that statement? Not trying to be contentious by any means... just interested to learn about this, as I don't see anything about drupal_add_js() being abandoned on its Drupal 8 doc page.

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented

Thanks @geerlingguy for coming back into the discussion.

I suspect these are all ways to get the same code to work. I like the fact that drupal_add_html_head(), by its very definition, puts the code into the HEAD. I also like the fact you don't get CDATA wrappers, that are already one departure from the Pingdom code. Yes, it does work using drupal_add_js(), because that was how the first version of this module did it. But when geerlingguy suggested drupal_add_html_head, I immediately decided I liked it better.

I also was unsure whether I could trust drupal_add_js() with its scope of "header", because "header" is usually a named region in the body section of the page, where blocks can be put. That is not the same thing as the "head" of the page. If drupal_add_js() means the HEAD section when the scope is given as "header", that's unfortunate nomenclature - if the documentation reassured me that it is only confusing naming, I'd have been less keen to use drupal_add_html_head(). I still prefer drupal_add_html_head() though.

As for hook_page_alter(), as opposed to hook_page_build() - those same docs you linked to have this detail:

For your additions to be printed, they have to be placed below a top level array key of the $page array that has the name of a region of the active theme.

By default, valid region keys are 'page_top', 'header', 'sidebar_first', 'content', 'sidebar_second' and 'page_bottom'. To get a list of all regions of the active theme, use system_region_list($theme). Note that $theme is a global variable.

Your code does not do that, so perhaps it isn't as necessary as the documentation makes out. The documentation suggests, however, that this is only to be used to insert code into page regions. The fact that it works more broadly is good, for now, but what if the code was tightened up later to ensure that code only went into the regions specified?

Again, given hook_page_alter() works, I feel happier using it. (I agree that hook_init() isn't right).

Thanks both for your ongoing input.

JordanMagnuson’s picture

JordanMagnuson CreditAttribution: JordanMagnuson commented

I suspect these are all ways to get the same code to work.

Absolutely. By all means, use whatever method you prefer. I've just seen more examples of the one I mention, but of course that doesn't mean that it's necessarily better. If people know of more examples of drupal_add_html_head() being used, I'd love to take a look at them. (@geerlingguy: I assume that you've perhaps used this method in some of your modules?)

I also was unsure whether I could trust drupal_add_js() with its scope of "header", because "header" is usually a named region in the body section of the page, where blocks can be put. That is not the same thing as the "head" of the page. If drupal_add_js() means the HEAD section when the scope is given as "header", that's unfortunate nomenclature

I agree that the documentation could be a lot more clear. The code is in fact added to HEAD, but that certainly isn't obvious from the docs...

For your additions to be printed, they have to be placed below a top level array key of the $page array that has the name of a region of the active theme.

By default, valid region keys are 'page_top', 'header', 'sidebar_first', 'content', 'sidebar_second' and 'page_bottom'. To get a list of all regions of the active theme, use system_region_list($theme). Note that $theme is a global variable.

This is only if you're adding something to the actual $page array, like in the code example in the docs. Again, the documentation's not super clear there.

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented

Thanks for clarifying.

In that case, if we're really in the realms of preference, I'll leave it as it is - for now. This is at RTBC stage, and the version that's been tested is the approach we have. When this reaches full project status, I can always release the code we now have, and then re-open this discussion in the project issue queue. At that point I have the luxury of changing the code in the -dev release only, so that any change can be thoroughly tested before being released.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Reviewed & tested by the community » Fixed

Looks good to me.

Thanks for your contribution, JamesOakley!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented

@Jordan - uh... hmm. I seem to have been misleading myself w/r/t #1029460: Find a better hook than hook_init() for drupal_add_js() and drupal_add_css() - hook_init() and drupal_add_js() is discouraged (for reasons you mentioned earlier), but drupal_add_js() itself isn't discouraged. But, as you affirm, there are so many ways to do the same thing :)

JamesOakley’s picture

JamesOakley
he/him/his
Primary language English
Location Kent, UK
CreditAttribution: JamesOakley commented

Any chance this discussion could now continue over at #1972476: Decide on the best hook, and the best Drupal function to use, for inserting the Javascript?

I'll also take this chance to thank you all for your help. We are now at a 7.x-1.0 release, and I'm sure the module (and the Pingdom service) will be a great help to people trying to discover if their Drupal sites are running slow.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Added comments to issue summary about the page / role visibility settings.