[D7] Mark-a-Spot Distribution

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

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-2.x branch:

• ./modules/mark_a_spot/modules/geolocation/modules/geolocation_cloudmade_osm/geolocation_cloudmade_osm.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function geolocation_cloudmade_osm_field_formatter_info() {
  function geolocation_cloudmade_osm_field_formatter_settings_form($field, $instance, $view_mode, $form, &$form_state) {
  function geolocation_cloudmade_osm_field_formatter_settings_summary($field, $instance, $view_mode) {
  function geolocation_cloudmade_osm_field_formatter_view($entity_type, $entity, $field, $instance, $langcode, $items, $display) {
  function geolocation_cloudmade_osm_field_widget_info() {
  function geolocation_cloudmade_osm_field_widget_settings_form($field, $instance) {
  function geolocation_cloudmade_osm_field_widget_form(&$form, &$form_state, $field, $instance, $langcode, $items, $delta, $element) {
  function geolocation_cloudmade_osm_field_widget_validate($element, &$form_state, $form) {
  function geolocation_cloudmade_osm_field_widget_error($element, $error, $form, &$form_state) {
  

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives. See attachment.
• Codespell has found some spelling errors in your code.
  

  ./themes/mas/less/README.txt:21: inital  ==> initial
  ./modules/mark_a_spot/modules/markaspot_log/markaspot_log.module:6: adminstration  ==> administration
  

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. mark_a_spot_admin_init(): do not use hook_init() to add CSS. Only add your CSS when it is actually needed, i.e. in the admin page callback ot the form or similar. If it absolutely must be included on every page you should define it in the info file of the module. See https://drupal.org/node/542202#stylesheets
2. Looks like the geolocation submodules don't belong in the distribution, you can include them in your make file with git from other sandboxes?
3. markaspot_log_get_log(): you should always use placeholders with db_query(), do not concatenate variables directly into SQL (prone to SQL injection).
4. markaspot_uuid.install: empty file, so remove it.
5. markaspot_uuid_form_alter(): if you are targeting only one form you should hook_form_FORM_ID_alter() instead. Also elsewhere.
6. "drupal_set_message(check_plain(t('Error while importing Tweets; Code:') . " " . $data->code), 'error');": don't concatenate variables into translatable strings, use placeholders with t() instead. In this case use "@" to sanitize the variable and remove the check_plain().
7. _markaspot_tweet311_twitter_create_user_from_tweet(): the check_plain() is useless here, since you are not printing anything to the user? Make sure to read https://drupal.org/node/28984 again.
8. geolocation_osm_field_formatter_settings_summary(): all user facing text must run through t() for translation. "Map", "Tile Server" etc.
9. geolocation_osm_field_formatter_settings_summary(): this is vulnerable to XSS exploits. You need to sanitize $settings['tile_server'] before printing into HTML, because it is user provided text. Please check all your variable output in your code. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
10. "'alt' => 'Geolocation',": all user facing text must run through t() for translation.
11. geolocation_osm_field_widget_validate(): "t('!name field is incomplete, latitude value is missing.', array('!name' => $element['#title'])));": that also looks suspicious, why don't you use the "@" placeholder to automatically sanitize the field title? Or can you be sure that the title is never user provided text?

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

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives. See attachment.

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. If certain link content is not translatable that is fine, then just ignore the warning. Same for st() usage - if you can be sure that the code will never run on an installed Drupal you can ignore them.
2. markaspot_logic_validate(): doc block is wrong, this is not a hook? See https://drupal.org/node/1354#forms
3. markaspot_logic_css_alter(): why do you remove that CSS? Please add a comment.
4. "'bbox_nw_lat' => variable_get('bbox_nw_lat'),": all variables defined by your module need to be prefixed with your module name to avoid name clashes with others.
5. All variables defined by your modules need to be removed in hook_uninstall().
6. "'access arguments' => array('administer twitter import'),": all permissions used by your module need to be defined in hook_permission(), or is that a permission re-used from another module?
7. markaspot_logic_admin_settings(): doc block is wrong, this is not a hook. See https://drupal.org/node/1354#forms
8. markaspot_logic_contents(): this looks a bit vulnerable to XSS exploits. The term name is user provided text and needs to be sanitized before printing. You are using views_trim_text() which will cut off the string at 25 characters, but that function does not perform any escaping. It might be hard to perform a useful XSS with that setting, but you should sanitize it anyway. Make sure to read https://drupal.org/node/28984 again.
9. _markaspot_tweet311_create_content(): since this runs during cron you should rather use watchdog() instead of drupal_set_message() to log errors, right?
10. _markaspot_tweet311_twitter_create_user_from_tweet(): the check_plain() is useless here, since you are not printing anything to the user? Make sure to read https://drupal.org/node/28984 again.
11. markaspot_log_delete_log(): db_query() with DELETE is forbidden, use db_delete() instead.
12. markaspot_archive_cron(): the query should directly include a filter for the timestamp and the status, then you only load the relevant nodes and don't need the condition in the foreach loop.

So I think there are still some rough edges here and there and handling user provided text securely concerns me a bit, but since I did not find any obviously exploitable place I would say this is RTBC.

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

Thanks for your contribution, Holger!

Some notes from me:

markaspot_archive.module: lines 21 and 29

You are using fixed term IDs here. Are you sure, the term IDs represent the correct terms in all installations? We, for example, configure our MySQL servers to increment numeric IDs by 2 so there is small chance to get even IDs ... (same here on drupal.org).
Additionally you could use entity_metadata_wrapper() and ->set() to set the fields value. That would remove this nasty ['und'] (which you should ditch in favor of LANGUAGE_NONE btw.).

markaspot_logic.module: line 201

There is no hook hook_admin_settings() ;). It's simply a menu callback.

Several .js-files

Try to use Drupal.behaviors whenever possible (for example in markaspot_logic/js/markers_leaflet.js).

Anyway, I couldn't find anything blocking your application so 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.

Cool!