Route Planner

please add README.txt file into your repository

README.txt

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

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.

coder on minor

I noticed some very small code style issues. Please run the Coder module on "minor" setting to help catch these. The coding standards have even more information in this area. I noticed things like ...

Please note: The Coder module currently has an unresolved flaw which will prompt you to add file declarations to your .info file even when it's not necessary to do so. Please do not try to make this warning go away by declaring files which do not contain classes or interfaces.

Severity minor, Drupal Coding Standards
sites/all/modules/route_planner/route_planner.module:
+17: [minor] There should be no trailing spaces
+18: [minor] There should be no trailing spaces
+30: [minor] There should be no trailing spaces
+35: [minor] There should be no trailing spaces
+64: [minor] Use an indent of 2 spaces, with no tabs
+83: [minor] There should be no trailing spaces
+87: [normal] use a space between the closing parenthesis and the open bracket
+97: [minor] There should be no trailing spaces
+110: [minor] There should be no trailing spaces
+112: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
+118: [minor] There should be no trailing spaces
+127: [minor] There should be no trailing spaces
+128: [minor] There should be no trailing spaces
+129: [minor] There should be no trailing spaces
+131: [minor] There should be no trailing spaces
+135: [minor] There should be no trailing spaces
+151: [minor] There should be no trailing spaces
+152: [minor] There should be no trailing spaces
+153: [minor] There should be no trailing spaces
+155: [minor] There should be no trailing spaces
+159: [normal] use a space between the closing parenthesis and the open bracket
+160: [minor] There should be no trailing spaces
+161: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
+162: [minor] There should be no trailing spaces
sites/all/modules/route_planner/route_planner.admin.inc:
+33: [minor] There should be no trailing spaces
+50: [minor] There should be no trailing spaces
+57: [minor] There should be no trailing spaces
+58: [minor] Use an indent of 2 spaces, with no tabs
Status Messages:
Coder found 1 projects, 3 files, 4 normal warnings, 24 minor warnings, 0 warnings were flagged to be ignored

You have to delete the branch 7.x-1.0, it will confuse Drupal when making releases!

Branches are always named 7.x-1.x, which you did already http://drupalcode.org/sandbox/Drupaldise/1315890.git/shortlog/refs/heads...

Some remarks:
1/ Small typo: interesst ==> interest.
2/ Maybe add in the README / Project description that this is a block, I had to look at the code to find out.
3/ Put your javascript inside a separate file, so it can be cached/aggregated.
4/ Mention that this module requires javascript to function.
5/ Mention that both blocks need to be enabled (maybe use 1 block?).

For your javascript use the following to encapsulate your code:

(function ($) {
Drupal.behaviors.routeplanner = {
  attach: function (context, settings) {
    $('body', context).once('routeplanner', function () {
      Drupal.myRoutePlanner = new Drupal.RoutePlanner();
    });
  }
};

Drupal.RoutePlanner = function() {
}

Drupal.RoutePlanner.prototype.route = function(){
}

Drupal.RoutePlanner.prototype.calcRoute = function(){
}

})(jQuery);

Drupal handles to loading of all javascript by using the Drupal.behaviors that way Drupal can control the loading of it, this mostly matters when your page is using AJAX callbacks to Drupal. By using the once method your code will only be added once to the page, even in case of an AJAX callback.

'Best' documentation I've found is http://drupal.org/node/756722

You have to use Drupal.settings to pass parameters to your javascript file, see http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ad...

Some small fixes

diff --git a/route_planner.js b/route_planner.js
index 115c91a..05e45cd 100644
--- a/route_planner.js
+++ b/route_planner.js
@@ -2,18 +2,20 @@
Drupal.behaviors.routeplanner = {
attach: function (context, settings) {
$('body', context).once('routeplanner', function () {
- Drupal.myRoutePlanner = new Drupal.RoutePlanner();
+ if (!Drupal.myRoutePlanner) {
+ Drupal.myRoutePlanner = new Drupal.RoutePlanner();
+ }
});
}
};

Drupal.RoutePlanner = function() {
- // initialize the map
- $(document).ready(function() {route(); });
-
var directionDisplay;
var directionsService = new google.maps.DirectionsService();
var map;
+
+ // initialize the map
+ this.route();
}

Drupal.RoutePlanner.prototype.route = function(){
diff --git a/route_planner.module b/route_planner.module
index 79e0607..62105e9 100644
--- a/route_planner.module
+++ b/route_planner.module

@@ -79,7 +79,7 @@ function route_planner_address_form($form, &$form_state) {
'#type' => 'button',
'#value' => t('Calculate Route'),
);
- $form['button']['#attributes'] = array('onClick' => 'calcRoute();return (false);');
+ $form['button']['#attributes'] = array('onClick' => 'Drupal.myRoutePlanner.calcRoute();return (false);');

return $form;
}

have a look at the included patch, it uses Drupal.settings to pass the parameters

Drupal is getting used to ;p, but good work! Someone else will have a second and will change the status accordingly.

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

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• route.module in route_planner.info: It's only necessary to declare files[] if they declare a class or interface.

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

Manual review:

• route_planner_settings_page(): if you just want a form then remove that function and use "drupal_get_form" as page callback directly in hook_menu().
• "//The name that will appear in the block list.": All comments should be on a separate line. There should be a space after "//".
• "// add google maps api": All comments should start with a capital letter.
• route_planner_install(): empty function, so remove it.
• your menu link does not appear on the admin/config page, you should place it in one of the categories
• route_planner_settings_form(): the map settings are not saved, check the names of the form elements that they match your variable names.
• route_planner_map_display(): you should always sanitize variables before embedding them into markup to avoid XSS attacks.

A small remark, but non-blocking: you can move your settings to the block itself, see http://api.drupal.org/api/drupal/modules--block--block.api.php/function/...

you can optimize '#attributes' => array('readonly' => 'readonly'), see http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....

$output = drupal_get_form('route_planner_address_form'); there are 2 spaces after =

for your settings form, if you use something like the following, you don't need a submit handler and if you change it, rename all variables to route_planner_

  $form['route_planner']['map']['rp_map_width'] = array(
    '#type' => 'textfield',
    '#title' => t('Map width'),
    '#description' => t('A width value in % or px, for example 300px or 100%.'),
    '#size' => 10,
    '#default_value' => variable_get('rp_map_width', '100%'),
  );

I did some more cleaning, see attached patch:

• use sentence case in strings
• moved settings to the blocks, both use the same settings
• cleaned $form structure in route_planner_address_form
• added the drupal_add_js's to route_planner_map_display as well so you can enable only one block

Can you try the patch and see if it's ok?

new patch

route_planner.js has some tabs on lines 32-36

but this looks good to me, changing status. someone else will have another look.

There is a security vulnerability in route_planner_map_display() line 157. First, you must not run filter_xss() on parts of HTML elements, see here http://drupalscout.com/knowledge-base/using-filter-functions-intended-fi...
Second, you should not use filter_xss() here because you don't want to pass through markup anyway. Use check_plain() instead of filter_xss(). See also http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-... for details on what function to use in which case.

I also looked at the unsanitized variables that you pass to the Javascript settings, but they go through drupal_json_encode() in the process and therefore do not pose an XSS security risk.

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

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• ./route_planner.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function route_planner_menu() {
  --
  
  function route_planner_get_address_form() {
  --
  
  function route_planner_map_display() {
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

tagging

do not RTBC your own issues.

Looks good to me

Thanks for your contribution, Drupaldise! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.