[D7] Sailthru API Module

Hi,

first of all there are quite a few issues to sort out such as indentation, whitespace.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxnickgundry1909942git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

Hi nickgundry ,

Your are right, we can not alter 3rd party code/ libraries.

If Sailthru Client is external Library, Then do not put its code in your module. Library folder provided by drupal (eg: sites/all/libraries) is used to hold 3rd party libraries. Provide the option to user to download Sailthru library in sites/all/libraries.

Check the Sailthru library existence using Library API module which is used to handle 3rd party library and also check it in hook_requirements()

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

you can find coding style issue generated by automated review program. please fix them and as @klausi recommend to get a review bonus so we can come back to your application sooner.

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you won't show up on my high priority list.

use the Tags fieldset in the comment form.

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

• 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.
  

  
  FILE: /home/klausi/pareview_temp/sailthru.applications.inc.php
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   247 | WARNING | All variables defined by your module must be prefixed with
       |         | your module's name to avoid name collisions with others.
       |         | Expected sailthru_user_subscribe_list but found
       |         | user_subscribe_list
   257 | WARNING | All variables defined by your module must be prefixed with
       |         | your module's name to avoid name collisions with others.
       |         | Expected sailthru_user_subscribe_name_fields but found
       |         | user_subscribe_name_fields
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/sailthru.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   306 | WARNING | All variables defined by your module must be prefixed with
       |         | your module's name to avoid name collisions with others.
       |         | Expected sailthru_user_subscribe_name_fields but found
       |         | user_subscribe_name_fields
   331 | WARNING | All variables defined by your module must be prefixed with
       |         | your module's name to avoid name collisions with others.
       |         | Expected sailthru_user_subscribe_list but found
       |         | user_subscribe_list
  --------------------------------------------------------------------------------
  
  Time: 1 second, Memory: 9.50Mb
  

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. "'#markup' => 'The Sailthru module requires jQuery 1.5.1 or greater.You have jQuery Update installed, so everything should be fine.',": all user facing text must run through t() for translation. Please check all your strings.
2. sailthru_get_url_data(): that function is never used? Also: why can't you use drupal_http_request()?
3. sailthru_user_profile_ui(): it looks to me that this is vulnerable to XSS exploits. The $user variable stems from an untrusted third party source and therefore has to be sanitized before printing. Or can you point me to the place where this might already be done by the PHP library? Make sure to read http://drupal.org/node/28984 again.
4. "'#markup' => t('<p>Scout is a powerful recommendation engine that, when applie": HTML tags should be outside of t() where possible.
5. "watchdog('Sailthru - List Details', 'Could not get connect to Sailthru ' . $e);": do not concatenate dynamic variables into the watchdog message, use placeholders instead.

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

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

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

My uess is nickgundry should have set this back to needs review in comment 14?

if ($has_list != 0 || $has_list != '') {

more standard:

if (!empty($has_list)) {

I'm not reviewing, I just need something with sailthru... in drupal 6.. and was evaluating if backporting this would be useful (nope, doesn't do what I need)

Whew! I know there's a lot of things in the list below, but this look like a great start to make sailthru integration easy, thanks! Most of the below is about using the Drupal API instead of creating your own functions or manual process. You're not required to fix everything below, but not using the Drupal API is a blocking issue. Also, thanks for re-opening this hefox.

1. You have a few code style issues reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxnickgundry1909942git.
2. In step 1 of your README, I think you meant the final path should be /sites/all/libraries/sailthru/Sailthru_Client.php
3. I don't think you need if (function_exists('libraries_get_path')) { or function_exists('taxonomy_get_vocabularies') since Libraries and Taxonomy are already dependencies - you can count on them being enabled by the time your code executes.
4. sailthru.make.example should just be called sailthru.make
5. What is $st in sailthru_get_client()? Doesn't look like it's used anywhere.
6. In sailthru_validate_install() you can swap the if/else clauses so you don't have to set $_sailthru_loaded = FALSE; in 4 places. Or how about
   

     $_sailthru_loaded = TRUE;
     $files = array('Sailthru_Client.php', 'Sailthru_Util.php', 'Sailthru_Client_Exception.php');
     foreach ($files as $file) {
       if (file_exists("$library_path/$file") {
         require_once "$library_path/$file";
       }
       else {
         $_sailthru_loaded = FALSE;
       }
     }
   

   Similarly with the if/else blocks at the end, you can use

     $is_valid = (!empty($api_key) && !empty($api_secret));
     if (!$installation) {
       $is_valid = ($is_valid && !empty($tpl));
     }
    

   No need to set true/false manually when you've already evaluated them.

7. variable_gets have a default return value of NULL, no need to set it to empty string really.
8. What is user_subscribe_list in sailthru_block_info()? Did you mean sailthru_user_subscribe_list?
9. Instead of $has_list != 0 || $has_list != '' you can use empty(), which will return false for both of those cases.
10. In hook_uninstall you're missing variable deletes for
    • sailthru_user_subscribe_name_fields
    • sailthru_user_subscribe_list
    • sailthru_jquery_version
    • sailthru_concierge_enabled
    • sailthru_scout_enabled
    • sailthru_concierge_paths
    • sailthru_concierge_css_path
    • sailthru_concierge_from
    • sailthru_concierge_threshold
    • sailthru_concierge_tags
    • sailthru_concierge_delay
    • sailthru_scout_render
    • sailthru_scout_num_visible
    • sailthru_scout_include_consumed
    • sailthru_taxonomy
    • sailthru_validate_taxonomy
11. Instead of sailthru_validate_list(), you can just set the '#required' => TRUE property in your form element - that will make sure the user has entered a non-empty value.
12. sailthru_js_alter() is not very friendly, adding jquery_update to the list of dependencies is the way to go! That guarantees you at least 1.5.1. Directly modifying Drupal's version of jquery is a bad idea, that can easily break other contrib modules.
13. In sailthru_page_alter() instead of checking for admin paths with arg(0), just use path_is_admin()
14. Also in sailthru_page_alter(), you have double if ($is_admin) statements that should be combined.
15. I'm not sure about switch ($concierge_enabled) - it's a boolean, so there's only 2 possibilities. Use an if/else instead?
16. For setting the css_path, you can use url().
17. Your js calls are a little screwy, I'd encourage you to read the docs for drupal_add_js() again. You can send values from PHP to JS in the Drupal.settings.yourmodule object instead of printing them directly into JS.
18. I don't think you need loadHorizon() at all - lower down you're already including your external js library, and if you use the settings, all your values will be available already. Also modifying the onload object is not very nice.
19. All your code in sailthru_preprocess_html() should instead be in a hook_node_view(), since you only want to take action when a node is being viewed. Additionally, the node object should already have its taxonomy terms attached there, so you don't need to get them manually with sailthru_get_node_tags().
20. All user-facing content should be run through t(), like "Recommended For You".
21. In sailthru_subscribe_form_submit(), use $form_state['values'] to retrieve the entered data.
22. All your hook functions should have a docblock like "Implements hook_something().", see https://drupal.org/coding-standards/docs#drupal.
23. In sailthru_user_profile_ui() you can use theme('table') to create your tables instead of building them manually.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Added new reviews for the review bonus program

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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