[D7] OSM Route

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, a good idea for a module - first glance:

1.) The link to the project contained an empty a tag - cause it not to work.

2.) You only need the git where a reviewer can get the source not your own.

3.) Looks like there is some missing files line 9 in osm_route.module, you require "osm_route.admin.inc", I cant find this file anywhere ?, and insted of require this you should just add it in the .info file.

4.) You should make an install file with check for that curl is installed. take a look at: hook_requirements()

Other than that it looks fine but I cant install it.

Hi artsakenos!
Here are some suggestions:
1) There is no hook_admin_settings(), please look here https://drupal.org/node/286528. I would recommend you to look here too https://drupal.org/node/1354#forms.
2) Please pay attention to validate and submit functions docs https://drupal.org/node/1354#forms.
3) here is the last suggestion. What if user need couple content types to be able to use OSM. Maybe you can add this ability.

Thank you for nice code!

Removing review bonus tag, you have not listed any reviews in the issue summary? Make sure to read the docs again: http://drupal.org/node/1975228

• You can't have execution rights for your files.
• Remove .gitignore file.
• Use small letters for br in hook_help for consistency.
• I wouldn't put all those spaces in t function in those long form descriptions.
• Package in .info file has a weird name.
• You use randomly double and single quotes. I would like to see consistency.

Some of these are my personal suggestion that you don't have to take seriously. The first two are those that must be fixed.

Hi artsakenos, I can confirm that all of the issues above are fixed.

There are just two smaller issues:

files[] without classes or interfaces

files[] = osm_route.admin.inc
files[] = osm_route.install
files[] = osm_route.module

Those lines should be removed. It's only necessary to declare files[] if they declare a class or interface.

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.

API Request

Your module uses Curl for requests against the API in osm_route_admin.inc. Better practice could be to use drupal_http_request instead. This Drupal HTTP client implementation takes care if a site is behind a proxy.

pareview.sh came up clean, but I have been having problems with it the last day or two.

The project page could use some more info about how to use the module.

You need to elaborate the README.txt a little bit to better explain how to use this. At first glance I was expecting
POI and itinerary content types to already be there.

The docblock on osm_route_help is wrong.

(*) Your usage of osm_route.admin.inc is wrong. Typically, you just use this for your admin pages, and not
general hooks. You also set this via the hook_menu, and don't require it at the top level.

(*) You need a hook_uninstall in osm_route.install to get rid of the variables you are creating.

Avoid HTML in t().

Your settings form can probably use element level validation, eg element_validate_number() and friends.

(*) The hook_install() should go in osm_route.install

(*) I am pretty sure #multiple_toggle is coming from a non-core module. It isn't documented in the Form API (https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...). I would
convert these to selects or checkboxes.

I don't think osm_route_get_webpage() is needed; use drupal_http_request() instead.

osm_route_node_presave() should use the Field API instead of directly accessing the field values. Or, you
can investigate entity_metadata_wrapper, which is part of the Entity module.

(*) The URL builder in osm_route_get_geofield() is bad. Use drupal_http_request() instead. Don't forget to error
check the response code.

Your define()s aren't namespaced to your module. These may interfere with other modules.

Normally, variables use underscores instead of camel case.

(*) You should check if the referenced nodes in osm_route_node_presave() is published or not. This
would allow users to have tentative changes to routes, and unpublished nodes are normally inaccessible to
anonymous users. See also next, which is similar.

(*) You may also have a weird situation where an itinerary may contain POIs that aren't accessible to all users (for
example, if node access records are in use). This is a tricky situation; your route may expose information about
the node the current user doesn't have access to. I am not sure what the best option is for
this. Perhaps you need to do something like

$anonymous = user_load(0);
$can_view = node_access('view', $node_point, $anonymous);

This doesn't cover the situation of hiding nodes in a route from certain users.

Starred issues (*) are blockers, but I am open to discussion about how to handle the last one.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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/osm_route.module
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   381 | WARNING | Unused variable $new_routes.
  --------------------------------------------------------------------------------
  

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. "$anonymous = user_load(0);": use drupal_anonymous_user() instead.
2. "drupal_alter('osm_route_poi_nodes', $poi_nodes_id);": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php
3. "$cont_title . ' ' . t('for segment') . ' ' . $i": do not concatenate variables to translatable strings, use placeholders with t() instead. Same for "t("Travel Time") . ":$cont_traveltime " . t("minutes")", this should be one string in t() using placeholders. Also elsewhere.
4. osm_route_render(): this should be a proper theme function, so that themers can easily override the markup. Please use hook_theme() and register your theme function.
5. osm_route_get_node_directions(): this is vulnerable to XSS exploits. You are directly printing $i_description into HTML, which stems from an untrusted third party source and must therefore be considered untrusted user input. Am I right that you are directly getting this information from the web service and storing it as is (which is correct)? You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984 again.

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

@artsakenos, Getting another review bonus will help speed up the process and make sure it gets on the review admins radar.

Hello,

I just test your module, it looks great. It's my first review of a project application.

Pareview.sh : OK

I look the code and I did not see something to say. Maybe phpdoc for the constant you define ?

Documentation :
I think there is one or two more steps to detail and it will be ok.

What about the cardinality of field_path ?

I tried to display some contents on a map modifying the feature in example in the openlayers module and I did not managed to do it.

Should I use the field_path on the openlayer view or the geofield in the POI content type ?

It is a little bit confusing, field_poi or field_waypoints ? : In the README.txt
"A content type (e.g., POI) containing a field of type geofield (e.g., field_poi)" ......"Create/configure the POI content type/entity (e.g., POI): Create a field of type geofield (e.g., field_waypoints)"

I also should say that I tried quickly. I will retry to test tomorrow.

Thanks for this promising module.

Ok,

I took the time to well configure the content types and it worked perfectly. I attached a screenshot.

I think what need to be precised is that field_path must be multivalue (or it is unnecessary ?) and that in the openlayers views, for the geofields, uncheck "Display all values in the same row" in the "Multiple field settings" fieldset.

I think to be able to show the waypoints and the itinerary on the same map, it should be the same geofield. But that is clearly a detail.

Thanks for this really cool module.

@artsakenos, just a quick review of your module.

1. osm_route_admin_settings(): You are using multiple t() function within same field value, this should be one string in t() using placeholders. ex: #description in osm_route_near_threshold field element. Same applies for osm_route_help.
2. In osm_route_menu(), The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations. A separate permission would be good, too.
3. At line number 112 and 389, you are using count within loop, best approach is hold count value in separate variable and use that variable in loop. This is just a general practice not related to Drupal :-)
4. Also I tried to run your module on my local machine, getting following notices:
   

    Notice: Undefined property: stdClass::$field_test in osm_route_node_presave() (line 84 of C:\xampp\htdocs\drupal731\sites\all\modules\2122323\osm_route.module).
       Warning: Invalid argument supplied for foreach() in osm_route_osm_route_poi_nodes_alter() (line 56 of C:\xampp\htdocs\drupal731\sites\all\modules\2122323\osm_route.module).
       Notice: Undefined property: stdClass::$field_test in osm_route_get_node_directions() (line 371 of C:\xampp\htdocs\drupal731\sites\all\modules\2122323\osm_route.module).
       Warning: Invalid argument supplied for foreach() in osm_route_osm_route_poi_nodes_alter() (line 56 of C:\xampp\htdocs\drupal731\sites\all\modules\2122323\osm_route.module).

I will do detailed review in next few days, it would better to get review bonus back soon :)

Thank you for your hard work and patience.

The name of this project is OSM Routes, but it shows up between modules as OpenStreetMap Route. I got a bit confused when I searched for OSM, and did not find the module.

Unfortunately I got "The API page http://www.yournavigation.org/api/1.0/gosmore.php could not be reached." error, and due to this the module is not working. I will try it later, and do a manual review for now.

The codig standards say "Functions and variables should be named using lowercase, and words should be separated with an underscore." I saw you missed this a few times, for example: osm_route_triesnumber, $allfields, OSM_ROUTE_YOURNAVIGATION_URL, OSM_ROUTE_YOURNAVIGATION_QUERY_TEMPLATE,

Otherwise I see no problem. I wonder if yournavigation.org is down only temporarily.

Automated Review

Yes. http://pareview.sh/pareview/httpgitdrupalorgsandboxartsakenos2122323git

Manual Review

Individual user account

Yes. You can read https://www.drupal.org/node/272587

No duplication

Why do you think that is better create a new project than improve routeplanner?

Master Branch

Yes, you can check: https://www.drupal.org/node/1127732

Licensing

Yes, you can check: https://www.drupal.org/licensing/faq

3rd party code

It's ok, too: https://www.drupal.org/node/422996

README.txt/README.md

You can improve it with https://www.drupal.org/node/2181737

Code long/complex enough for review

Yes, you can read: https://groups.drupal.org/node/195848

Secure Code

Why are you using get_t instead of t?
you can read: https://www.drupal.org/writing-secure-code.

Style of Code

You are using
define('OSM_ROUTE_YOURNAVIGATION_URL', 'osm_route_yournavigation_url');
define('OSM_ROUTE_YOURNAVIGATION_QUERY_TEMPLATE', 'osm_route_yournavigation_quer
y_template');
You can create variables in the admin settings, better.

This code is dirty:
$base_url = variable_get(OSM_ROUTE_YOURNAVIGATION_URL, 'http://www.yournavigat
ion.org/api/1.0/gosmore.php');
$query_base = variable_get(OSM_ROUTE_YOURNAVIGATION_QUERY_TEMPLATE, '?format=g
eojson&flat=%s&flon=%s&tlat=%s&tlon=%s&v=%s&fast=1&layer=mapnik&instructions=1&l
ang=en_GB');

admin/config/services/osm_route: The select fields: Georeferenced
content field, Coordinates field, Name of the field where to save the
calculated path appears empty.

@artsakenos, this application doesn't currently have a review bonus. If you can get one, the process will go quicker.

IMHO its ready to fly.

Review of the 7.x-1.x branch (commit 6a05da0):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/osm_route.module
  --------------------------------------------------------------------------------
  FOUND 7 ERRORS AFFECTING 7 LINES
  --------------------------------------------------------------------------------
    83 | ERROR | [x] Whitespace found at end of line
   137 | ERROR | [x] Whitespace found at end of line
   150 | ERROR | [x] Whitespace found at end of line
   219 | ERROR | [x] Whitespace found at end of line
   406 | ERROR | [x] Whitespace found at end of line
   413 | ERROR | [x] Whitespace found at end of line
   465 | ERROR | [x] Expected 1 newline at end of file; 2 found
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 7 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. project page: difference to the existing module https://www.drupal.org/project/routeplanner is missing.
2. osm_route.module: why do you globally require the admin.inc file? It does not include any hook implementations so it does not have to be loaded on every single page request? osm_route_menu() is missing the "file" option to specify the admin.inc file?
3. "// /-{ I build an array containing the lat/long coordinates.": why does the comment start with "/-{"?
4. osm_route_osm_route_poi_nodes_alter(): instead of having multiple DB queries with multiple node_load() calls you should collect all IDs and perform a node_load_multiple() for performance reasons. Same in osm_route_node_presave().
5. theme_render_directions(): all theme names should be prefixed with your module name to avoid name colissions with other modules.
6. "$i_description = check_markup($values['i_description'], 'full_html');": so "i_description" is not sanitized at all, because on most sites the full_html text format will allow any Javascript tags for example? 'full_html' might not exist on every site, so either you have a configurable text format that the admin specifies or you use generic functions such as filter_xss() or filter_xss_admin() to sanitize the description. Looks like a security blocker to me.

manual review:

• osm_route_get_node_directions(): still use node_load() in a loop, could use node_load_multiple(). Same for the hook_osm_route_poi_nodes_alter() docs.

But otherwise looks good to me now.

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

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

Review of the 7.x-1.x branch (commit ccd67e1):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

Most of the points are already covered and nothing major jumped out at me when I read `git diff ccd67e1..` so...

Thanks for your contribution, Addis!

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.