[D7] Garmin Connector

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

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.

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.

This is not fixed? See https://drupal.org/node/532400

http://pareview.sh/pareview/httpgitdrupalorgsandboxcandotri1816816git still shows thousands of code formatting errors. None of those are individually blockers, but there's so many that it's going to be difficult for anyone to read the code to review.

Much of that is likely due to the 3rd party libraries you have included directly in your module. Those need to be changed to a library include to prevent errors with other modules using the same libraries. See: https://drupal.org/node/422996

More points to improve:
- use 7.x-1.x branch instead of master
- remove all dpm() (found them in .install file)

You need to set the status to "needs review" if you are ready and want to get a review, see https://drupal.org/node/532400

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

README.txt

Please take a moment to make your README.txt follow the guidelines for in-project documentation.

• You can remove the commented out lines from garmin_connector.info, they're always in the git repo if you need them back.
• In garmin_connector_install(), I'm not sure what's going on with those empty conditional blocks, can you remove or finish them?
• In garmin_connector_schema() you have a user column set as varchar 40, do you want to use uid instead?
• You should remove garmin_connector_update_7002() - assume your users have not installed this module before.
• Remove the commented out dpm() calls if you can.
• I'm not sure what's in the Makefile - can you delete that file?
• What is garmin_connector.test testing? I'd remove it until you're ready to flesh out the tests.
• In garmin_connector.module, don't use bare php, and definitely don't change any php ini values. Assume the user has their PHP configured the way they want it.
• Is it possible to remove the new global $_garmin_connector_cf? A function with a static variable is an easier way to reuse a single connection.
• If your project requires the flot module, add it to the list of dependencies in the .info file.
• Don't use hook_init() if you can, just load your javascript/css when it's actually needed.
• In garmin_connector_block_view() - all your code blocks ends the same way with
  

    // ...
    $renderer['content'] = array('#markup' => $returner);
    return $renderer;
  

  You could abstract that out from all 3 code blocks and just put it at the bottom of the function.

• In garmin_connector_permission() you don't seem to be using the $descriptions array at all, deleteable?

That's as far as I've gotten, this is not a complete review. The module seems very ambitious and potentially useful, but right now there are a lot of unfinished parts and debug code.

My suggestion is to limit the scope down to what's absolutely necessary (or what you actually have time to build), and delete everything else for now. You can always add it back in later :) Otherwise you'll never get out of "needs work" if there are unfinished bits hanging around.

Let's work on getting this approved, and then you can add new features in the next version.

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

Are you ready for another review?

Sorry, I didn't see that list of questions!

Regarding deleting dpms that are commented out: Is this an asthetic thing?

Yes, just aesthetic. If you feel strongly about it it's fine to leave them in.

garmin_connector_update_7002

It's ok to leave that in, just not usual for a new module.

What is bare php?

It's PHP code that's not contained in any function, hence "bare". When Drupal loads your file as part of it's normal process, any bare PHP is always executed - so it's on every page request, regardless of whether it's useful or not. Can be bad for performance, and just a general bad practice.

$_garmin_connector_cf

We try and avoid globals wherever possible. Generally you can get the same functionality with variable_get/set (for permanent storage) or drupal_static (for storage throughout a single request).

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

• What is disabled.js? Doesn't jquery already have a get() function?
• I think you can get rid of hook_init() now that you've got a dependency on flot?
• garmin_connector_purge_workout() does not seem like it's working?
• In garmin_connector_spill_locals() you should use file_directory_temp() to get the temp path.
• All variables used in your module should be removed in hook_uninstall.
• variable_get can accept default values. So instead of $days = variable_get('garmin_connector_reward_number_of_days') ? variable_get('garmin_connector_reward_number_of_days') : 7; use $days = variable_get('garmin_connector_reward_number_of_days', 7)
• "// Make the pareview warning go away." - I appreciate trying to make the automated script happy, but sometimes it does give false positives. In this case I bet it was telling you the $key variable is not used in your function. Instead of creating a dummy use (if (is_array($key)) {}), change your foreach to foreach ($workouts as $workout) {. No $key variable needed :)
• All user-facing text should be run through t(), like $message. Try and keep HTML tags out of the translation string if possible. So instead of <h2>Did you complete the workouts you hate?</h2> use '<h2>' . t('Did you complete the workouts you hate?') . '</h2>'

This is another partial review.

candotri, I think you have the same problem of having too much debug/test code to get this project approved. I think what's happening (as you mentioned earlier) is that you're still in the debugging process of your module, and it's not polished yet. It's understandable that you still want debug code, and it's fine in a sandbox as well, but for a Drupal module, users expect to download a working, finished module. That's not to say it has to be bug-free, but the basics have to be covered.

I think the best solution is for you to create a local branch to work from (git checkout -b 7.x-1.x-debug or something). Don't push it to origin, just keep it local (if you do accidentally push it, no big deal). You can keep your debug code in there for checking stuff out, and the main 7.x-1.x branch to keep the finished code. Then keep the 7.x-1.x branch clean and tidy. You can use git merge to easily move changes from one branch to another. This is a good simple guide to git if you're not used to it: http://rogerdudler.github.io/git-guide/.

Or you could make the debug stuff "official". What I mean by that is, create a hook_menu entry for it, put permissions around it, render the output like you would any other Drupal function, and move the code out of your .module file. Some modules provide test pages like this, like a page where you can test that your garmin connection is working, or that it's parsing a dummy data file correctly.

From #13:

limit the scope down to what's absolutely necessary (or what you actually have time to build), and delete everything else for now. You can always add it back in later :)

Instead of deleting, you can keep that code in a different branch.

Also, take a look at Tips for ensuring a smooth review - that covers a lot of common things we look for when reviewing a module.

Hope that helps!

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

Candotri, this queue is for use to review finished, working modules to grant you the 'git vetted user' status. Unfortunately, it's not intended to help you learn coding best practices. Additional workload for reviewers results in longer wait times for everyone in the queue. I'm setting this issue to postponed, please feel free to reopen it when you think it's ready for review again.

Take a look at Tips for ensuring a smooth review for a list of common issues if you haven't already.

@candotri, What are you trying to do with flot that you can't use the flot module? I've used it fairly extensively and never had problems with its loading using libraries.

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
Review of the 7.x-1.x branch:

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

  
  FILE: /home/klausi/pareview_temp/garmin_connector.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 4 WARNING(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
    795 | WARNING | Variable $message is undefined.
   1382 | WARNING | Unused variable $by_date.
   1670 | WARNING | Unused variable $type.
   2305 | WARNING | Variable $_garmin_connector_debug is undefined.
  --------------------------------------------------------------------------------
  

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 agree with kscheirer that a full project should be production-ready and not contain site-breaking debug code. If you must do that do it in a debugging git branch or add a conditional debug mode.

manual review:

1. garmin_connector_show_workout_set(): this is vulnerable to XSS exploits. You need to sanitize user provided text before printing, you are taking arbitrary stuff from $_GET and printing it directly to HTML, allowing for a reflected XSS attack. Make sure to read https://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
2. The project page should mention that this module should not be used on a production site right now, because it will throw fatal errors if the devel module is not present. Users should be aware that is not ready to be used yet at all.
3. garmin_connector.info: why do you need the JS and CSS on every single page? Why can't you add it when your module produces output? Please add a comment.

manual review:

1. garmin_connector_permission(): do not line-break translatable strings.
2. garmin_connector_activity_manager(): that hook_menu() callabck is completely unprotected and can be accessed by anonymous users, right? Why is there no permission guarding that page and the others? I fear that this could be vulerable to DoS attacks if garmin_connector_ajax_run_cron_form is an expensive operation.
3. garmin_connector_activity_manager(): don't call drupal_render() here, just return a complete return array, drupal will render it later for you. Same in garmin_connector_admin() and garmin_connector_get_contextual_links(), just return a render array.
4. Your module file has nearly 4000 lines of code which is loaded on every single page request on a Drupal site, and of course this is hard to maintain. The module file should only include hook implementations for such huge modules and the rest should be loaded from include files when needed.
5. "'#options' => module_invoke_all('strength_workout_types'),": all hooks defines by your module need to be prefixed with your module's name + docs in garmin_connector.api.php.
6. garmin_connector_admin_validate(): do not use check_plain() 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
7. You are not using any theme functions or templates as far as I can see? So nobody can override the markup on a site?
8. garmin_connector_process_purge(): "// Pareview is satisfied": what does that mean? Why do you need an empty if-statement here? What is Pareview? pareview.sh? Why would that want an empty if statement?
9. garmin_connector_purge_single(): why do you check permissions here? If the user does not have access to that functionality then the ajax form to trigger that should have never been displayed in the first place? /garmin-connector/purge.html should not be accessible by non-admin users, correct? Then there should be a permission in hook_menu()?

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

The usual Drupal philosophy is to not display any links to pages where the user does not have access to in the first place. Also that pages should not leak any information what is behind them, so anyone that does not have access should just get a generic 403.

manual review:

1. garmin_connector_permission(): permissions usually don't use dashes, just separate the words with spaces.
2. garmin_connector_cron(): if corn runs often, then this will run often. Are you sure that you need to do this stuff every 5 minutes?
3. garmin_connector_post_activity_upload(): why do you need cURL and cannot use drupal_http_request()? Please add a comment.
4. garmin_connector_post_activity_upload(): that function is not used anywhere, or am I overlooking something? And the doc block says this implements a hook, so it should be garmin_connector_garmin_connector_post_activity_upload(), right?
5. garmin_connector_post_activity_upload(): this looks vulnerable to XSS exploits. $result2 comes from an untrusted third party source, so it has to be sanitized before printing into HTML. Make sure to read https://drupal.org/node/28984 again.
6. garmin_connector_purge_activities_form: that page callback is not defined in the module file and the hook_menu() entry does not specify a file either, so does that even work?
7. garmin_connector_admin(): don't call drupal_render() here, just add another item to your render array. Same in garmin_connector_activity_manager(), just return nested render arrays.
8. '#markup' => "<h3>Workout set with name $set</h3>",: all user facing text should run through t() for translation.
9. garmin_connector_get_compare_results(): Are you sure that $this_activity->activityName is sanitized already? Is that user provided text?
10. There are still lots of functions that return un-themable raw markup like garmin_connector_activity_manager().

So the potential XSS issue is a blocker right now, otherwise this looks quite ok.

manual review:

1. garmin_connector_admin(): instead of using drupal_add_css() you should use #attached in the render array. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
2. garmin_connector_admin(): it is ok to return a form render array, but you should not call drupal_render() on any nested elements in it. The whole array tree of elements will be rendered later for you and it is easier for other modules to alter the form when not rendered yet.
3. no, there is no Drupal function to sanitize JSON that I'm aware of. I think it would not make sense to sanitize JSON, only if you print stuff from that third party supplied JSON then you should make use of the usual check_plain() and friends.
4. garmin_connector_strength_done(): it does not make sense here to call check_plain(), since you are not printing anything to the user here? And $title is sanitized correctly in the hook implementation with the "@" placeholder, so that would be double escaping, which is bad.
5. t("Workout set with name") . " $set</h3>": do not concatenate variables into translatable strings, use placeholders with t() instead.
6. garmin_connector_get_compare_results(): why do you call check_plain() on the $selected array keys? I don't exactly see where they are printed to the user, but the sanitization should happen as late as possible (right before printing or adding to a render array). That way the internal data structures stay intact as long as possible and check_plain() only happens if needed.
7. garmin_connector_compare_activities_form(): $ignore_regex is never printed to the user, why do you run check_plain() on it?
8. "return "Id $id was not found in the garmin connector catalogue";": all user facing text must run through t() for translation, please check all your strings.
9. garmin_connector_add_to_workout_set(): "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 . Don't run check_plain() on variables that are inserted into the database.

The use of check_plain() is a blocker right now, you need to know when to use it and when not. Otherwise this would get a green light from me, so don't give up just yet, I think you are almost there!

manual review:

1. "Small improvements" aas git commit message all the time is not very helpful, see https://drupal.org/node/52287
2. "<p>" . t("Purging") . $location . ":" . "$status</p>";: do not concatenate variables into translatable strings, use placeholders with t() instead.
3. garmin_connector_show_workout_set(): The check_plain() for $set is not needed, because the "@" placeholder is used in the t() message, and I cannot see where else it would be printed?

Bu otherwise I think this should be now sufficient, so RTBC from me.

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

What is the latex directory for? Is it something that the user needs to install? I don't see anything about it in the README or hook_requirements().

Cool, back to RTBC.

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

Thanks for your contribution, candotri!

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.