[D7] Personalization

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.

I have found some issues into your module code
1.

function personalization_node_view($node, $view_mode, $langcode) {
  if ($view_mode == 'full') {
    $setting = array(
      'currentNid' => $node->nid,
    );
    drupal_add_js($setting, 'setting');
  }
}

As you are passing some values globaly into js so that you variable name must be something like this due to prevent other modules conflicts.

function personalization_node_view($node, $view_mode, $langcode) {
  if ($view_mode == 'full') {
    $setting = array(
      'currentNid' => $node->nid,
    );
    drupal_add_js(array('personalization' => $setting), 'setting');
  }
}

2.
In personalization.admin.inc, no need for these lines drupal automatically call these functions
$form['#submit'][] = 'personalization_admin_submit';
$form['#validate'][] = 'personalization_admin_validate';

Quite a big module so might take a while to test it all properly.

Some comments from me:

1. "require_once" - admin one can be included in hook_menu() via "file" property. Maybe otehrs could be implemented in a same way?

2. Implement hook_uninstall() to remove the variable? just so if one removes the module it wont leave an variables in database.

Hi,

I have reviewed "Personalization" module code using coder module. and pareview.sh .
I haven't found any error.
I have installed "Personalization" module. Then I have tried to add the new search mapping. But gives me following error. I have tried with Search keyword "test" and taxonomy term "test" . This taxonomy term has not been created in the system.

Error :
Notice: Trying to get property of non-object in personalization_admin_search_submit() (line 548 of D:\xampp\htdocs\wbc\sites\all\modules\personalization\personalization.admin.inc).
Notice: Trying to get property of non-object in personalization_admin_search_submit() (line 549 of D:\xampp\htdocs\wbc\sites\all\modules\personalization\personalization.admin.inc).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'vid' cannot be null: INSERT INTO {personalization_keyword_mapping} (keyword, vid, tid, created) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3); Array ( [:db_insert_placeholder_0] => test [:db_insert_placeholder_1] => [:db_insert_placeholder_2] => [:db_insert_placeholder_3] => 1384850970 ) in personalization_admin_search_submit() (line 551 of D:\xampp\htdocs\wbc\sites\all\modules\personalization\personalization.admin.inc).

Could you please look into it?
Thanks,
Shubhangi.

Review report:

Please modify:
Git: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Dan.Ashdown/2137715.git
as below:
Git: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Dan.Ashdown/2137715.git personalization

Might be useful to know that testdb user 1 credentials are admin:admin :)

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

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

Documentation

The current description is rather esoteric. I believe that a simple use case or two in the product description might help a lot in seeing the value of this module.

Code review

The source code is well structured and easy to read, although I would have liked to see a few more explanatory comments, esp. around the score altering functions. The module uses Drupal's APIs where applicable and the overall impression is solid. A few nit-picking errors were reported by pareview.sh. Some additional remarks:

Includes

• You should try to not to include files directly, as this adds (superfluous) overhead. You should consider copying the external code into the .module file instead. Also, check out the API function module_load_include() for including files.
• Use the “file” key (as you have with admin pages) on your ajax page handlers in hook_menu, to remove the need of including personalization.ajax.inc.
• The personalization.js can be included in the module's .info file instead of hook_init(), since its not a conditional load.

cURL

To remove the need for cURL, consider using Drupal's HTTP client, drupal_http_request.

Suggestions

A permission “bypass personalization” might be useful for roles who have no need for this functionality – or maybe even a “use personalization” permission instead of choosing Anonymous and authenticated users on the config page.

I will just assume that the weighing algorithm works as intended, as I'm not familiar with it. However, there seems to be quite some additional database processing going on per request. It might be an idea to implement some form of “personalization_get_user”-dependent cache on personalization_match_content.

Other

First time I visited the page after importing example db, the module wouldn't stop writing db entries, and I ended up with 14k+ rows in user_locations and user_scores tables. I wasn't able to reproduce this behavior on a db reset, neither did I find any non-terminating conditions in source, so I'm writing it off as Force majeure. If it occurs to someone else, you might have a problem.

Thanks, and if you think I was too harsh, feel free to tear me a new one in my own project application ;)

Hello again,

Nice work! I've checked your code and as far as I'm concerned, you're good to go.

A few picky remarks:

• I see you're using time() in some places and the Drupal constant REQUEST_TIME in others,
  I see no reason why REQUEST_TIME couldn't be used in all cases.
• As user_access() uses the global user per default, you can shorten personalization_trackable() to
  

  function personalization_trackable() {
    return user_access('use personalization');
  }

Updating status to RTBC.

manual review:

1. personalization_ajax_get_list(): don't use die() here, but rather drupal_exit().
2. personalization.info: why do you need your javascript on every single page request? Why can't you add it when it is actually needed in a block or page?
3. personalization.install: description for one tid column is wrong.
4. personalization_ajax_init(): "personalization_track_page(check_plain($_POST['nid']));": check_plain() is wrong here, you are not printing anything to the user in that function? Make sure to read https://drupal.org/node/28984 again, you should only sanitize data if you are outputting/printing something to HTML.

Need to run now, will follow-up another time.

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/personalization.geo.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   120 | ERROR | Invalid @param data type, expected int but found number
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/personalization.tracker.inc
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   225 | ERROR | Invalid @param data type, expected int but found number
   227 | ERROR | Invalid @param data type, expected int but found number
  --------------------------------------------------------------------------------
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/theme/pzs-page-list-item.tpl.php
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   8 | WARNING | Do not use check_plain() on the first argument of l(), because
     |         | l() will sanitize it for you by default
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/theme/pzs-block-list-item.tpl.php
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   8 | WARNING | Do not use check_plain() on the first argument of l(), because
     |         | l() will sanitize it for you by default
  --------------------------------------------------------------------------------
  

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.

I don't think that pareview.sh requested check_plain() there. Coder Review has some old checks that are known to throw false positives, and you should never change your code blindly.

manual review continued:

1. "$url = parse_url(check_plain($_POST['path']));": same problem here again: parse_url() does not print stuff, so you should not run the path through check_plain(). Same in personalization_track_search(): no need to run DB query stuff through check_plain(), that does not make sense.
2. personalization_ajax_init(): doc block is wrong, this is not hook_init()?
3. personalization_ajax_init(): looks vulnerable to CSRF exploits, but I guess that is ok for tracking modules, so this should not be a problem.
4. personalization_get_user_location(): is that DB query MySQL-specific or does that work on any other DB system like Postgres? Please add a comment.
5. You are using db_select() a lot for simple static select queries, use db_query() instead. See https://drupal.org/node/310075
6. personalization_add_geo_vocabulary(): do not hard-code "und" as field language identifier, use LANGUAGE_NONE instead.
7. personalization_form_alter(): if you are targeting only one form you should use hook_form_FORM_ID_alter().
8. personalization_admin_search_delete(): this is vulnerable to CSRF exploits. You are performing destructive write operations to the database without using a confirmation form or a security token in the URL. See also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained . And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
9. personalization_match_content(): this is vulnerable to node access bypass exploits. You forgot to add the node_access tag to your select query on the node table. See also https://api.drupal.org/api/drupal/modules!node!node.module/group/node_ac...
10. personalization_admin_search_form_validate(): do not document $form and $form_state here, see https://drupal.org/node/1354#forms
11. personalization_admin_search(): do not call theme() here, just return a nested render array. Drupal will render it later for you.

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

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/personalization.tracker.inc
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   162 | ERROR | Invalid @param data type, expected string but found String
   165 | ERROR | Invalid @return data type, expected string but found String
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/personalization.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   302 | ERROR | Invalid @return data type, expected string but found String
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/personalization.admin.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   368 | ERROR | Invalid @return data type, expected string but found String
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/personalization.ajax.inc
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AND 1 WARNING(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
    75 | ERROR   | Invalid @param data type, expected string but found String
   136 | ERROR   | Invalid @param data type, expected string but found String
  

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.

Sorry, I didn't see the node_access tag further down below in personalization_match_content(), so that is fine.

manual review:

1. personalization_block_view(): do not call theme() here, just return a nested render array. Drupal will render it later for you.
2. personalization_admin_search(): @return doc block is wrong
3. personalization_admin_search(): this is vulnerable to XSS exploits. If I enter a mapping name as <script>alert('XSS');</script> then I get a nasty javascript popup. You need to sanitize user provided text before printing into a table. Make sure to read https://drupal.org/node/28984 again. Same for the term name ==> user provided text, so it needs to be sanitized in the table.

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

I'm constantly improving the automated review tools, that's why the show new stuff from time to time.

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

• Codespell has found some spelling errors in your code.
  

  ./personalization.install:145: seperately  ==> separately
  ./personalization.install:148: seperately  ==> separately
  ./personalization.admin.inc:165: seperate  ==> separate
  ./personalization.admin.inc:317: occured  ==> occurred
  

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. personalization_admin_search_form_validate(): why do you call filter_xss() here, you are not printing the keyword to HTML?
2. personalization_admin_search_form_submit(): filter_xss() is wrong here. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from https://drupal.org/node/28984
3. admin/config/system/personalization/search is still vulnerable to XSS exploits. personalization_admin_search() needs to sanitize $mapping->keyword and $mapping->name.

Looks good to me now!

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

no objections for more than a week, so ...

Thanks for your contribution, Dan.Ashdown!

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.