[D7] Bongo

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

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.

Hi bneil, i looked at your module and only find one thing :)

bongo.admin.inc
--------------------------
line 15: If you store variables in the database (with system_settings_form) you should have a .install file and delete the variables in the hook_uninstall().

Kind regards
kasalla

Here's my feedback as a patch so you have a list of todos. This also removes some tab chars in the bongo_panes.js file. These are mostly calls for refactoring some things to make your life easier down the road. I'm not suggesting you use this module but you may gain insight into some of my feedback by picking through https://drupal.org/project/cis_connector

Hi bneil!

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.

Major coding standards / best practice issues

An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

You can find the results of the automated report at http://pareview.sh/pareview/httpgitdrupalorgsandboxbneil1807738git.
OR
I´ve attached the automated report as text file to this comment.

Other:

1. bongo.module - please, use url() function for build urls to "api.ebongo.org"
2. For all text need use t() function and Drupal.t() (for js).

Kind regards

hook_requirements should be in the module's .install I believe so that should then be resolved.

Hi bneil :)

I agree with btopro, hook_requirements should be in the .install file. According to https://api.drupal.org/api/drupal/modules!system!system.api.php/function/hook_requirements/7,

Note that this hook, like all others dealing with installation and updates, must reside in a module_name.install file...

There are a couple of periods missing...one at the end of bongo.module line 135 ("An array of routes") and one at the end of bongo_predictions.inc line 63 ("Shown by default").

I enabled bongo and bongo_panes on a fresh site install, and everything worked as expected.

Hi bneil,
after take a look to bongo module, just a little thinking: Probably creating "bongo_build_predictions_table($stopid, $rows, $agency)" as a render element through "hook_theme" instead of being a custom function would provide more integration and flexibility to developers.

Hope this is useful.

Regards!

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/bongo.module
  --------------------------------------------------------------------------------
  FOUND 7 ERROR(S) AFFECTING 6 LINE(S)
  --------------------------------------------------------------------------------
   197 | ERROR | Array indentation error, expected 4 spaces but found 2
   198 | ERROR | Array indentation error, expected 4 spaces but found 2
   199 | ERROR | Array indentation error, expected 4 spaces but found 2
   200 | ERROR | Array indentation error, expected 4 spaces but found 2
   208 | ERROR | Array indentation error, expected 4 spaces but found 2
   209 | ERROR | Line indented incorrectly; expected 2 spaces, found 0
   209 | ERROR | Array closing indentation error, expected 2 spaces but found 0
  --------------------------------------------------------------------------------
  
  
  FILE: ...i/pareview_temp/bongo_panes/plugins/content_types/bongo_predictions.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   139 | WARNING | Translatable strings must not begin or end with white spaces,
       |         | use placeholders with t() for variables
  --------------------------------------------------------------------------------
  

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. "array('Time', 'Route', 'Agency');" all user facing text must run through t() for translation. Also elsewhere.
2. bongo_build_predictions_table(): this looks vulnerable to XSS exploits. $title and $agency_name seem to come from an untrusted third party source and therefore must be treated as user provided input. You need so sanitze all user provided text before printing. Please 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.

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

Cool, looks RTBC to me know!

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

bongo.module

  97   else {
  98     drupal_set_message(t('A Bongo API key is required to use the Bongo Developer
  99      API.'), 'error', FALSE);
 100     return array();
 101   }

Please don't make a line break within translatable strings, very ugly to handle. (btw, the 80 characters max. rule only applies for comments - not code. you can remove the newline here completely)

 275 function _bongo_decode($uri) {
 276   $decode_content = drupal_http_request($uri);
 277   $decode_array = drupal_json_decode($decode_content->data);
 278   return $decode_array;
 279 }

What could possibly go wrong? (please add error handling and proper logging)

beside that I couldn't find any blockers..

Thanks for your contribution! :-)

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.

*yay*

(forgot status change)