[D7] Tether Stats

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxrzan2329491git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then 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.

Installed in drupal 7.32 showing
Notice: Undefined index: in charts_pre_render_element() (line 207 of /var/www/html/drupal/sites/all/modules/charts/charts.module).
Notice: Undefined index: in charts_pre_render_element() (line 207 of /var/www/html/drupal/sites/all/modules/charts/charts.module).
Notice: Undefined index: in charts_pre_render_element() (line 207 of /var/www/html/drupal/sites/all/modules/charts/charts.module).
Notice: Undefined index: in charts_pre_render_element() (line 207 of /var/www/html/drupal/sites/all/modules/charts/charts.module).

Notice: Undefined index: in charts_pre_render_element() (line 207 of /var/www/html/drupal/sites/all/modules/charts/charts.module).

hi @rzan

I’ve downloaded and installed your code and have conducted a manual review according to the review templete found at https://www.drupal.org/node/1587704 . My review comments are as follows…

1.1 Your application issue DOES contains a link to the project page and a working "git clone" command.

1.2 The realm of collecting site statistics in Drupal seems dominated by Google Analytics. Your sandbox page indicates the basic types of stats collected by Tether Stats includes Page Hits, Link Clicks, and Impressions. The impressions concept seems unique to this code, but I’m not an analytics expert.

1.3 I’ve checked for multiple application and see you only have this one project.

2.1 (+) I’ve cloned from your sandbox version control and that works OK. When I view the project directory however its named 2329491. This make it somewhat difficult, I’d suggest fixing that now to make it easier for other reviewers.

2.2 Git Status tells me that “Your branch is up-to-date with ‘origin/7.x-1.x’ “ which is good, you are using major version branches (e.g., 7.x-1.x, 6.x-1.x).

3.1 (+) I’ve checked for use of the t() function and find that it is only used once, in tether_stats.install, in the form of

// Ensure translations don't break during installation.
$t = get_t();

I’ve also checked your use of the database abstraction layer to avoid SQL injection attacks. I note that ‘db_select’ is used 6 times in tether_stats.module . To me this usage appears correct but I call it out for other reviewers expert in the use of the DB layer in Drupal to review more closely.

4.1 The repository does not contain a license.txt file.

4.2 (+) Tether_Stats, and Tether_Stats_Charts both include .js files which appear to have been authored by you? There is no associated licensing files tagged in or packaged with these, so is it right to assume this is NOT third party code?

If it is third party code, a dependency should be declared on Libraries and the .js files placed accordingly. If not, you may want to consider doing this anyways, but I don’t think its required by Drupal. IMHO, the way you are handing it now works best, if they are indeed your work.

I call this out explicitly because third party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.
The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on drupal.org.

5.1 The Project pages are helpful and provide a clear way to understand what a project does.

5.2 The README.txt files in the package are very good!

5.3 Your code is well commented and clearly understandable by other developers. Very nice job on that!

6.1 Automated review is clean.

7.1 (+) In the Project description you indicate the capability to write and retrieve data to/from a third party db . When reading this data back in, it may be this is from an untrusted source? If so, are the proper checks in place to scrub the data and present it securely?

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Please note that I've changing status back to needs work so you can respond primarily to the licensing question .

Hi Rzan,

I installed module and go to configuration and click on "Tether Statistics" and then found following error.

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drawing.tether_stats_derivative' doesn't exist in menu_local_tasks() (line 1941 of C:\xampp\htdocs\janfence\includes\menu.inc).

Please find the attached screen shot for your reference.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Hi rzan,

I've download and installed your module, and found no issue the module is working fine to me so I am changing the status to fixed.

Best,
Zeeshan

Then this should be RTBC instead.

Great!
get cool things going!!!!!!!!

Cheers!

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

Review of the 7.x-1.x branch (commit a02321e):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: ...usi/pareview_temp/modules/tether_stats_charts/js/tether_stats_charts.js
  --------------------------------------------------------------------------------
  FOUND 2 ERRORS AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   17 | ERROR | Doc comment short description must start with a capital letter
   17 | ERROR | Doc comment short description must end with a full stop
  --------------------------------------------------------------------------------
  
  FILE: /home/klausi/pareview_temp/tether_stats.test
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   7 | ERROR | Missing class doc comment
  --------------------------------------------------------------------------------
  

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. project page: could you add the difference to existing projects such as https://www.drupal.org/project/jstats ?
2. tether_stats_requirements(): why would you want the tables in a different database? Please add a comment. I think you should just implement hook_schema() so that the tables are created automatically. Power users can always create them manually in other DB if they must.
3. tether_stats_admin_derivative_delete_callback(): this is vulnerable to CSRF exploits. You either need a confirm form or a security token in the URL before performing write operations on a request. See also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained . Note that your tether_stats/track callback can be considered an exception, because it has to work for any user, even anonymous users anyway. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
4. tether_stats_page_build(): do not use drupal_add_js() here, use #attacked on the $page render array instead. See https://api.drupal.org/api/drupal/developer--topics--forms_api_reference...
5. tether_stats_get_element(): this function is 300 lines long and very hard to read/understand. I would refactor it in more meaningful parts.
6. tether_stats_describe_element(): this looks vulnerable to XSS exploits. Node titles for example are user provided text and need to be sanitized before printing to HTML. If I have a node with the title <script>alert('XSS');</script> then this will trigger a nasty javascript popup. Make sure to read https://www.drupal.org/node/28984 again.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

Review of the 7.x-1.x branch (commit e2db844):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives. See attachment.

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. tether_stats_schema(): doc block is now outdated, right?
2. tether_stats_drop_database(): where exactly is that function used? AS menu loader? But it is not appearing in hook_menu() in the paths?
3. tether_stats_describe_element(): all user facing text in there must run through t() for translation. Same in theme_tether_stats_admin_derivative_entity_form() for example "array('Derivative Name', 'Description', 'Actions');" and "No derivatives currently exist for this entity type.". And in tether_stats_charts_get_iterable_renderable_chart() for the link labels. Please check all your strings.
4. tether_stats_settings_form_validate(): instead of validating positive integers yourself you can use https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h... and https://api.drupal.org/api/drupal/includes!form.inc/function/element_val...

But otherwise looks RTBC to me.

Assigning to heddn as he might have time to take a final look at this.

1. tether_stats_charts.js: line 55 variable implicitly declared.
2. tether_stats_charts.js: line 25 unused local variable chart_id.
3. tether_stats.js: line 140 variable initializer is redundant.
4. tether_stats_charts.examples.inc: line 32/42 use l(), rather than url(). Also in tether_stats_charts.module line 318/260
5. tether_stats.callbacks.inc line 40: void function 'drupal_json_output' result used
6. tether_stats.module: line 440 used local variable $match
7. tether_stats.module: line 1283 variable 'options' might have not been defined
8. tether_stats.forms.inc: line 680 unused local variable $elid
9. tether_stats_charts.module: tether_stats_charts_get_iterable_renderable_chart() looks like a theme function. But since it isn't setup as one, it makes things difficult to alter for other folks.
10. (+) tether_stats_charts.module: charts_info() is called several times. But no such function exists. It is also mentioned in the example.inc too.
11. Personal peeve: Don't throw Exception. Make a custom Exception class. try/catch of Exception is bad programming style. Example in tether_stats_charts.schema.inc->calcDomainStep()
12. Use of REQUEST_TIME instead of calling time() is prefered. Example tether_stats.mdoule line 612 & 674. Other examples also exist.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

None of these are blockers though so...

Thanks for your contribution, rzan!

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.