[D7] Webform tracking

Upfront. This is great work ;)

Here's my review:

Licensing

The module doesn't bundle any non-GPLed files. (I took a look at all 4 for files included in the module).

Similar modules

Analytics Tracking seems to cover some minor parts of this modules features, namely: gathering of referers. I don't think that the overlap is big enough to block this project though.

Drupal API

Drupal API: The module makes heavy use of the DB-API and other Drupal APIs when applicable. I haven't found any parts that reimplement Drupal functionality.

Security:

• The module stores additional tracking data for weform submissions. It might be sensible to introduce a new to allow permission for viewing that information. Currently the module presumes that any person authorized to see webform results is allowed to see the tracking data - which is probably fine for nearly all installations.
• The module stores parameters (received via $_GET) in the database. It uses check_plain() to sanitize those parameters.

Coding style

A pareview doesn't show any recommendations.

  // Do not track if Do-Not-Track (DNT) is set, but ignore IE 10 as it sends
  // DNT by default and thus undermindes the concept.
  $ua = isset($_SERVER['HTTP_USER_AGENT']) ? $_SERVER['HTTP_USER_AGENT'] : '';
  if (empty($_SERVER['HTTP_DNT'])
      || preg_match('/MSIE\s*10/', $ua)
    || !variable_get('webform_tracking_respect_dnt', TRUE)) {

Perhaps the condition could be made a bit clearer by using temporary variables with meaningful names. Something along the lines of:

$ua = isset($_SERVER['HTTP_USER_AGENT']) ? $_SERVER['HTTP_USER_AGENT'] : '';
$respect_dnt = !preg_match('/MSIE\s*10/', $ua) && variable_get('webform_tracking_respect_dnt', TRUE);
if (empty($_SERVER['HTTP_DNT']) || !$respect_dnt) {

Code documentation

While some function have concise comments others only have the "Implements hook…()" line. Adding one or two sentences about what the function actually does - especially for the longer functions - would be perfect.

Hi,

I download and installed the module and other required module. It does not shows any extra information said in the above description. When I started to apply patch provide with the module to get the extra details, its gives an error "includes/webform.report.inc: No such file or directory".

• Instead of using $_GET. It is recommended to use drupal_get_query_parameters() in module file.

Hi Phaer,

Its not madatory to use drupal_get_query_parameters() . As there is a drupal function, It's better to use it instead of core PHP functions. That's fine for me. But I have seen many $_GET -variables without check_plain etc like filters in webform_tracking.module. Fillter is mandatory for $_GET-variables.

Also I noticed that you used $_GET['q'] in line number 87 of webform_tracking.module. Instead of $_GET['q'] use current_path. As $_GET['q'] got depreciated in drupal 8. It will be difficult if any one wants to convert your module from drupal 7 to drupal 8.Refer: https://drupal.org/node/1659562

Thanks!

phaer, sorry for the delay, but the project review administrators have been busy. I cannot approve your application,
but I can give it a thorough review. In the future, though, let others change the status to RBTC on your issues.

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/webform_tracking.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
169 | ERROR | Inline comments must start with a capital letter
--------------------------------------------------------------------------------

Minor.

Manual Review

Add a link to automagically run pareview to the description above.

README files normally have .txt extensions in Drupal, but I have seen enough .md ones to say that this
isn't a problem.

Project page should mention that is only works with webform-3.x.

Project page should mention the soft dependency on geoip

.info file should reflect the proper dependency for webform. See https://drupal.org/node/542202#dependencies for how to
specify this.

Your hook_update_N don't follow the conventions in the API docs. Do these work? As this is a brand new module, you may also just
want to start out with the current schema.

Your module has a setting variable, but no hook_uninstall() to delete it.

You should move the link out of the #title in webform_tracking_form_webform_admin_settings_alter() and into a #description.

I thought session_cache_set() and session_cache_get aleady() handled the serialization? Did this change?

The logic in webform_tracking_boot() may not work with sites running out of multiple domains that don't use canonical
URLs, or potentially ones using Domain Access. See what happens here.

I would combine all of the session_cache_sets into a single array/object.

Do you need to handle hook_node_delete() to get rid of data when nodes are deleted? I don't recall whether
hook_webform_submission_delete() gets invoked from webform_node_delete().

I *strongly* suggest a separate permission for viewing this data, or potentially two: one for viewing full data, and the
other for viewing slightly anonymized data (eg, X out the last octet of the IP).

webform_tracking_load() can likely use ->fetchAllAssoc() instead of a loop.

Data filtering is normally done on output, but as the data source in this case may not be clear to others as actually being
user input, I would say that doing it before writing to the database is OK. Be prepared if the project admins disagree
with this assessment, as this does differ from Drupal practices.

The only way to opt out of this is with the DNT header?

Not seeing any third party code, security issues, project duplication issues, or major API issues, so leaving this as RBTC.

manual review:

1. why do you need hook_boot() and cannot use some hook_form_FORM_ID_alter() when the actual webform is displayed? Please add a comment.
2. check_plain() should only happen when data is printed as output to HTML, not when it is saved to the database. Since your module is also responsible for display it should then sanitize the user provided data before printing to HTML. See https://drupal.org/node/28984 . webform_tracking_boot() for example should contain no check_plain() because it does not print anything. webform_tracking_webform_results_extra_data() might be the place, or does the webform module apply sanitization anyway after the hook?
3. I agree with all points made by mpdonadio.

No critical application blockers, so ...

Thanks for your contribution, phaer!

I updated your account so you can 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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.

Then you should delete your sandbox to avoid any confusion which project should be used. Thanks!