MultiAnalytics

Hello and welcome,

Project review

- Git link is wrong and should be: git clone http://git.drupal.org/sandbox/ptoly/1648142.git multianalytics
- Please indicate for which version of Drupal this module is on the project page and the README.txt
- You should remove the LICENSE.txt file as it will be added by drupal.org packaging automatically.
- Please see http://drupal.org/empty-git-master and move your code from the master branch to the appropriate one depending on the version of Drupal the module is for (ex: 6.x-1.x)

Automatic review

Several errors have been detected, please read the detailed log on http://ventral.org/pareview/httpgitdrupalorgsandboxptoly1648142git-master.

Best regards.

Corrected wording

Maual Review:
1. Please remove t() from multi_analytics_menu() function it is not neccessary to use t() in hook_menu().
2. In multi_analytics_install() function instead of t() use get_t() to ensure translations don't break at install time.

PAReview - lots of issues.
you can see it here :http://ventral.org/pareview/httpgitdrupalorgsandboxptoly1648142git

Overall, I'd hope that whatever additional feature you needed to add could have been folded in as a patch or even an add-on-modifier module to the main, popular Google Analytics module. Making an entirely new module that replicates lots of that one feels like it introduces confusion.
I'd ask you to make a very strong case explaining clearly why this is neccessary and how it works.

it adds a lot more, by adding an ability to easily create both multiple GA snippets, assign them to pages within the site, and custom (non-Google) conversion tracking codes.

I think (to me) this needs a clearer example of how and why this is done.

It also adds the ability to set the order of tracking codes embedded in a page and choose if they should be embedded in the header or footer of the page (again, a common request of my clients).

I do understand, but I'm surprised that this couldn't be added to the usual module as an optional patch.

Right now, It feels like it's below the threshold for uniqueness - in comparison to the existing module.

Visual review

I'm not sure that this one module feature really needs to add 3 whole permissions just to get access to the admin UI. It seems like overkill.

Pretty good use of Drupal APIs in most places.
Apart from multiple references to $_GET['q'] - which should be eliminated.
Internal comments are OK, with some room for improvement.
Seems to make a good attempt to validate and filter input for danger.
Some of the database inserts could be converted to the DB API versions, but that's not a blocker.

I'm not sure about the overall architecture - things like global $_multi_analytics_footer_snippets; and the reasons this is used bug me a little. But I'd have to dig right in and trace the full code before I could say if this could be done better. :-\ Right now, all I've got is "code smell" that's making me uncomfortable.

I'm not happy about the addition of the additional jquery libraries that are just being pulled in in tpl.php like

   $path = drupal_get_path('module', 'multi_analytics');
   drupal_add_js($path . '/js/jquery.hoverIntent.minified.js');
   drupal_add_js($path . '/js/jquery.cluetip.min.js');
   drupal_add_js('misc/tableheader.js');
   drupal_add_css($path . '/css/jquery.cluetip.css');

This opens up a large potential for version conflicts in hard-to-trace ways. There are ways to register, isolate, publish, and use js libraries if needed. It looks a little more work, but it protects everyone against hours of debugging when things eventually do clash - like when another module makes the same mistake as this, but with a different version of the same library.

And that tpl.php file looks like it's doing quite a bit in old-fashioned PHP-inline-code way rather than using utils like theme_table(). I guess that's why you found it neccessary to then create a form of tabledrag.js
It probably solved your immediate task in a way that made sense to you, but it's not the best Drupal way.

Currently, I think it needs a bit more work to make it feel more robust. And even then - it needs to distinguish itself from 'google analytics' pretty strongly and clearly to be a useful alternative to users and not a confusing choice.

There was certainly a little bit of "hurried" in the review, as I did it as part of the Drupalcon project queue sprint and we were trying to touch as many of the stalled or blocked projects as possible in a short time - to try and help push them along or keep activity up.
This one I was unable to approve immediately, due to concerns I listed above - partly about the duplication issue, and partly about a few points where it seemed to not be cleanly following (or aware of) some general Drupal conventions.

I understand your reasoning for needing your own module, it sounds like the approach taken is significantly different. The question now is mostly about how we could inform new users about why they should choose your code over the GA one - in language that makes sense to the users who are going to be looking at the two project pages.

"Pretty good" use of the Drupal APIs means that most of the common APIs are used appropriately, though there are a few more places (specifically the DB Layer APIs, as mentioned) that would be needed to make it "100% use of the correct Drupal APIs in all the right places". Nobody is expected to know all the APIs and shortcuts all the time, so pretty good is good enough. Good enough, no complaint, but not outstanding either.

The comments are "fine" or "adequate" or "OK" and, as I say, are not a blocker. I wouldn't call them "good" or "excellent", and given that - from reading the code alone - I'm unable to fully understand large amounts of what is going on or why - I can only say the docs "could be improved".
Still, "better than nothing" and not something I'd recommend holding up the review over.

Using $_GET['q'] is strongly discouraged and deprecated. Code that does so is a red warning flag, and on visual review makes us have to question "do they know what they are doing or is this a hack that could break something else?". I'm unable to suggest exactly what you should do instead without setting up a full test system and tracing and rewriting all your logic there. Only that it looks shaky and should be fixable.

Also, the use of a global to store a string that is built and displayed at some other time later implies that your logic is placing things outside of the expected Drupal process flow, and this is the sort of thing that creates unpredicted conflicts when used in combination of other factors, such as AJAX-loaded pages, Page caching, Element-level caching, Admin overlays, Theme switching, stand-alone page bootstrapping, daemonized page-serving, and other really weird situations.

The "Experience" I'm offering as a Drupal developer is that a few of these short-cuts (globals and $_GET) are the things that turn into strange bugs, unpredictable weaknesses, difficulty to integrate with, and hair-pulling conflicts for debugging later. If they can be avoided, they should be.

My comments here are just as much for our other peer-reviewers here as for you. I haven't been able to specifically spend the time needed to reverse-engineer your code and then rewrite it for you to tell you exactly how to fix the things I think should be fixed - so the suggestions for improvement are somewhat non-specific, not actual patches.
But I'm not throwing up huge must-fix blockers either. I expect other reviewers to compare my comments with yours and your code and make second opinions, and either agree or disagree a little.

Depending on the depth of the functional testing we do in a code review, often it's sufficient to evaluate a module and say "this coder knows exactly what they are doing, and I can trust this performs as expected" and "The code looks a little uneven around the edges and I'm not confident that this approach will stand up to full edge-case testing". Based on this code smell I have tried to describe, I certainly would not and would not recommend installing this module on a live site in its current state.
This makes it impossible for me to green-light it as RTBC right now.
That's a straightforward opinion based on my own experience of reviewing thousands of Drupal modules (no exaggeration) without hard proof of a real problem (any more than there is hard proof that there are no problems).

Therefore I won't push this into either RTBC or Needs Work myself.
I'll leave this issue where it stands in case anyone else feels like picking it up one day.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

Bunch of styling issues: http://ventral.org/pareview/httpgitdrupalorgsandboxptoly1648142git

multi_analytics_uninstall() deletes variables with a wildcard. This may cause problems if this module is ever sub-moduled in the same namespace.

You need more logic in multi_analytics_embed() to detect pages you don't want to add tracking to. You detect admin, but this will also run on ctools modals and all sorts of non-HTML hook_menu callbacks. You may also want to consider organizations who actually want to track admin page usage.

You have template_preprocess_multi_analytics_admin_list_form(), but you also have PHP inlined in the .tpl.php. Can't you move it to the preprocess?

You should really provide additional error checking multi_analytics_admin_list_toggle_enabled_ajax() of the parameters since you are pulling directly from $_POST.

You are using multi_analytics_conversion_id directly in _multi_analytics_generate_output(); A user could embed arbitrary JS on the page.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

fixed another typo