[D7] Chicago Alternative Fuel Station Map

Git clone failed for http://git.drupal.org/project/2614192.git while invoking http://pareview.sh/pareview/httpgitdrupalorgproject2614192git

Git clone failed. Aborting.

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

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

• ESLint has found some issues with your code (please check the JavaScript coding standards).

manual review:

• See http://eslint.org/docs/rules/strict.html for (strict).
• See http://eslint.org/docs/rules/no-undef.html for (no-undef).
• See http://eslint.org/docs/rules/no-loop-func for (no-loop-func).

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.
Source: http://pareview.sh/ - PAReview.sh online service

Those minor coding standard errors are surely not application blockers, anything else that you found or should this be RTBC instead?

Nothing else, RTBC would be nice.

Pareview is clean.

Project is straightforward and simple.
The code is clean and readable. I'd like to see an example of the JSON that the city of Chicago returns so future developers can easily see if the format changes.

Automated Review

No errors were found using pareview.sh:

http://pareview.sh/pareview/httpgitdrupalorgproject2614192git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

I think this module is ready for the production use so I'll change the status to Reviewed by the community

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

I think this module uses Drupal API correct and has no Coding style problems.

I think this module is ready for the production use so I'm changing the status to 'Review & tested by the community'.

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

• ESLint has found some issues with your code (please check the JavaScript coding standards).
  

  /home/klausi/pareview_temp/js/alternative_fuel_map.js: line 8, col 1, Error - Use the function form of "use strict". (strict)
  /home/klausi/pareview_temp/js/alternative_fuel_map.js: line 8, col 10, Error - "stationMap" is defined but never used (no-unused-vars)
  /home/klausi/pareview_temp/js/alternative_fuel_map.js: line 34, col 55, Error - Don't make functions within a loop (no-loop-func)
  
  3 problems
  

• 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 is a bit short, see https://www.drupal.org/node/997024 . How does the module display the map? On a page? In a block? Is another module required to render the Google map? What are alternative fuel stations?
2. alternative_fuel_map_schema(): could you add descriptions to columns like "lid", what is it used for? See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
3. '#markup' => '<a href="https://goo.gl/ruS96H">Follow these directions to obtain a Google Maps Key.</a>',: all user facing text must run through t() for translation.
4. alternative_fuel_map_fetch_fuel_station_locations(): do not pass dynamic variables to the second parameter of watchdog(), always use a static string with placeholders for variables. See https://www.drupal.org/node/28984 and the watchdog() example.
5. alternative_fuel_map_cron(): If I have cron configured to run every 10 minutes then the DB will be updated very often, which is not necessary? Can you schedule a configurable interval instead?
6. alternative_fuel_map_update_station_locations(): how are locations deleted? As I understand it the list will only grow but a location is never removed from the database?
7. alternative_fuel_map_block_view(): do not call drupal_add_css() here, use #attached instead. See https://api.drupal.org/comment/44413#comment-44413 . Same for the JS.
8. alternative_fuel_map_menu(): the admin page does not show up in the menu, the 'type' of the menu item is wrong. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
9. I wanted to test the map output for XSS issues, but the google maps javascript URL is embedded wrong. The HTML output contains "https://maps.googleapis.com/maps/api/js?key=12345679secret&callback=stat..." which escapes the ampersand, so the Google API tells me key is invalid. Could you fix the code so that actually a valid URL is created and the module works? From what I can see drupal_add_html_head() might be a solution, see https://www.drupal.org/node/1356138

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. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Individual user account
Yes: Follows
No duplication
No module duplication / fragmentation
Master Branch
Yes: Follows
Licensing
Yes: Follows
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.

Nice little module, marking as RTBC!!

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

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

  /home/klausi/pareview_temp/js/alternative_fuel_map.js: line 8, col 1, Error - Use the function form of 'use strict'. (strict)
  /home/klausi/pareview_temp/js/alternative_fuel_map.js: line 34, col 55, Error - Don't make functions within a loop (no-loop-func)
  

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

  
  FILE: /home/klausi/pareview_temp/alternative_fuel_map.module
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  ----------------------------------------------------------------------
   116 | WARNING | Unused variable $update_threshold.
  ----------------------------------------------------------------------
  

• 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. alternative_fuel_map_install(): no need to set variables with variable_set() since you can make use of default values with variable_get() anyway.
2. alternative_fuel_map_update_station_locations(): time() will never be less than the alternative_fuel_map_last_update variable, so this will be executed on every single cron run.
3. alternative_fuel_map_add_station_pinpoints(): instead of calling drupal_add_js() here you should pass in the block render array and use #attached instead.
4. PDOException when enabling the module: 'PDOException' with message 'SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'NOT NULL DEFAULT 'CURRENT_TIMESTAMP' COMMENT 'TODO: please describe this field!''
5. That error prevented me from XSS testing the unsanitized HTML string that looks suspicious in alternative_fuel_map_add_station_pinpoints(). Can you make the module work so that I can test again?

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

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

  /home/klausi/pareview_temp/js/alternative_fuel_map.js: line 8, col 1, Error - Use the function form of 'use strict'. (strict)
  /home/klausi/pareview_temp/js/alternative_fuel_map.js: line 8, col 10, Error - 'stationMap' is defined but never used (no-unused-vars)
  /home/klausi/pareview_temp/js/alternative_fuel_map.js: line 34, col 55, Error - Don't make functions within a loop (no-loop-func)
  
  3 problems
  

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

  
  FILE: /home/klausi/pareview_temp/alternative_fuel_map.module
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  ----------------------------------------------------------------------
   159 | WARNING | Variable $locations is undefined.
  ----------------------------------------------------------------------
  
  Time: 73ms; Memory: 6.75Mb
  

• 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. no need to save your JS to a file. JS settings can also be used in #attached. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
2. Can you also fix the issues reported by pareview.sh?
3. What API key do I have to enable in the Google Developer console? README does not give me instructions?
4. Your regex replacer for the API key is wrong. It strips out dashes "-", but they are part of my API key.
5. Invalid argument supplied for foreach() alternative_fuel_map.module:159
6. I still can't import any locations to manipulate them for XSS testing. Can you make sure the module works and imports data into an empty database on a cron run?

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

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

  
  FILE: /home/klausi/pareview_temp/README.txt
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  ----------------------------------------------------------------------
   1 | WARNING | File encoding is invalid, expected UTF-8
  ----------------------------------------------------------------------
  

• 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. station-map.tpl.php: never use database queries like variable_get() in template files. Always pass all variables needed to the theme function when invoking theme(). See https://api.drupal.org/api/drupal/includes!theme.inc/function/theme/7
2. alternative_fuel_map_add_station_pinpoints(): saving the JS file will not work on most Drupal installs because the webserver user is not allowed to write files to a module folder. See https://www.drupal.org/node/244924 . Only writing to a dedicated files folder should be allowed for the webserver user. As I said you should use #attached with the JavaScript settings instead.

Although the module will not work on a securely set up Drupal install I don't see any other security blockers, so setting this to RTBC.

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

1. A lot of times folks put the admin configuration in a module_name.admin.inc file. And reference that file in the hook menu. Saves on cluttering up the memory since it will only get loaded when that route is accessed.
2. There's an API for creating links: '#markup' => '<a href="https://goo.gl/ruS96H">' . t('Follow these directions to obtain a Google Maps Key.') . '</a>',
3. Typically this is a !=, instead of if (200 <> $request->code)
4. REQUEST_TIME is a constant for time(). if (time() < $update_threshold[$update_frequency]) {
5. The duration should be configurable or an explanation via comment provided for 63 is the right amount of time.
   

   db_delete('alternate_fuel_map_locations')
       ->condition('last_updated', strtotime('-63 days'), '<')
       ->execute();

6. Using a hard-coded html ID is bad form. It means the block can only be placed once on a page and you better hope no one else has already used that same ID themself. Maybe use css classes instead. <div id="station-map"></div> (from tpl) and new google.maps.Map(document.getElementById('station-map') from JS

None of these things are really blockers though. So congratulations and thanks for your contribution, hook_awesome!

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.