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
Comment #1
PA robot CreditAttribution: PA robot commentedWe 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.
Comment #2
geerlingguy CreditAttribution: geerlingguy commentedI'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:
<?php
) at the top of the file.hook_init()
anddrupal_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.
Comment #3
tars16 CreditAttribution: tars16 commentedI 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.
Comment #4
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 :)
Comment #5
tars16 CreditAttribution: tars16 commented@alexweber, this is true! Services does make more sense.
Comment #6
JamesOakleyThanks everyone for your feedback.
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"
Comment #6.0
JamesOakleyAdded reference to ScriptureFilter in the hope it's a helpful cross-reference
Comment #7
JamesOakleyComment #8
klausiReview of the 7.x-1.x branch:
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:
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.
Comment #9
JordanMagnuson CreditAttribution: JordanMagnuson commentedThere 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().
Comment #10
JamesOakleyThank 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"
Comment #10.0
JamesOakleyAdded other reviewed projects
Comment #11
JamesOakleyAdded details of 4 more projects I have reviewed.
Comment #12
hkirsman CreditAttribution: hkirsman commentedDidn'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
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!
Comment #13
JamesOakleyThanks 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.
Comment #13.0
JamesOakleyAdded 4 more projects I have recently helped by reviewing
Comment #13.1
JamesOakleyAdded folder name 'pingdom_rum' to the "git clone" command in the issue summary.
Comment #13.2
JamesOakleyRemoved 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.
Comment #14
klausipareview.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.
Comment #15
hkirsman CreditAttribution: hkirsman commentedThat 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.
Comment #16
JamesOakley@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!
Comment #17
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:
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:
Here's how I've been adding the Pingdom RUM code on my own website via a custom module:
Anyway, nice work on this.
Comment #18
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.Comment #19
JordanMagnuson CreditAttribution: JordanMagnuson commentedInteresting. 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.
Comment #20
JamesOakleyThanks @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:
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.
Comment #21
JordanMagnuson CreditAttribution: JordanMagnuson commentedAbsolutely. 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 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...
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.
Comment #22
JamesOakleyThanks 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.
Comment #23
klausiLooks 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.
Comment #24
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 :)
Comment #25
JamesOakleyAny 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.
Comment #26.0
(not verified) CreditAttribution: commentedAdded comments to issue summary about the page / role visibility settings.