[D7] Local Places

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

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.

Adding Drupal version to issue title, to make reviewing easier, hopefully faster.

Hi,

I found some Issues:

1) Check ventral
link: http://ventral.org/pareview/httpgitdrupalorgsandboxtraceyspencer2002766git
For more details check the coding standard documentation.

2) $_local_places_mapurl
Use drupal_static() for that.

3) Do not use double quotes
Use single quotes instead. To see the difference see php manual.

4) Unnecessary $form declaration

<?php
function local_places_form($form, &$form_state) {
  // The following line is obsolete. $form is declared already.
  $form = array();
  $form['local_places_title'] = array(
  [...]
?>

5) Move local_places_form() to .admin.inc
Add a new file local_places.admin.inc and add the form there.
Edit local_places_menu(): Add file attribute. See hook_menu()

6) Use '-' instead of '_' for menu paths
'admin/config/content/local-places' instead of 'admin/config/content/local_places' in local_places_menu().

7) _local_places_page()
Try to use:

<?php
$local_places_contents = local_places_contents();
if (empty($local_places_contents)) {
  [...]
}
else {
  [...]
  $output .= '<div style="width:30%; float:left; padding-right:10px;">' . $local_places_contents . '</div>';
  [...]
}
?>

8) local_places_contents()

<?php
  // Unused variables:
  $lat = "";
  $lon = "";

  // Because of:
  $lat = $child->geometry->location->lat;
  $lon = $child->geometry->location->lng;
?>

9) Avoid inline CSS
Use classes and separate local-places.css file for this.

10) Use theme functions to provide html
To avoid html in callback functions you should use theme functions.
You can implement them by using hook_theme().
In this way other developers are able to override a theme function to build their own markup.
A theme function is called via theme().

11) Use '===' instead of '=='
It is faster.

12) Use PHP_EOL instead of '\n'
In this way you are independent from platforms. (Windows uses '\n\t')

13) Use '&&' or '||' to chain 'if' statements

<?php
if ($i <=5 && $granchild->text !== '') {
  [...]
}
?>

14) Implementing hook_uninstall()
You have to create a new file, called local_places.install.
In this file you have to implement hook_uninstall().
During hook_uninstall delete all variables which are stored by local_places like 'local_places_title'.

That's all for now.
Happy coding

Hi traceyspencer:

I have installed me your module and it works correctly.

You have a small problem of vulnerability with the title and description, if you write:
"><script>alert('Title');</script> and <script>alert('Description');</script> check it.

In the README.txt file URLs are not correct. I think that it is local-place instead of local_place.

Sorry, reference you forgot me https://drupal.org/node/28984

Hi there,

After installing, adding my api key and going to /local-places/ I was shown these warnings:

Notice: Undefined index: icon in include() (line 10 of ...\sites\all\modules\local_places\maplist.tpl.php).
Notice: Undefined index: type in include() (line 10 of ...\sites\all\modules\local_places\maplist.tpl.php).
Notice: Undefined index: type in include() (line 10 of ...\sites\all\modules\local_places\maplist.tpl.php).
Notice: Undefined index: detailsurl in include() (line 11 of ...\sites\all\modules\local_places\maplist.tpl.php).
Notice: Undefined index: name in include() (line 11 of ...\sites\all\modules\local_places\maplist.tpl.php).
Notice: Undefined index: letter in include() (line 11 of ...\sites\all\modules\local_places\maplist.tpl.php).
Notice: Undefined index: address in include() (line 12 of ...\sites\all\modules\local_places\maplist.tpl.php).

After manually reviewing, I couldn't see any major issues, but I think you could make the code more readable by using double-quote string instread of concatenating single-quote strings.

$map_url = $currenturl . 'details?name=' . $name . '&reference=' . $ref . '&type=' . $type . '&lat=' . $lat . '&lon=' . $lon;
becomes
$map_url = $currenturl . "details?name=$name&reference=$ref&type=$type&lat=$lat&lon=$lon";

Also, you still have some minor issues outstanding with the code sniffer - http://ventral.org/pareview/httpgitdrupalorgsandboxtraceyspencer2002766git.

Finally, it's clear you have put a lot of work into this so it would be a shame if people found it too limiting by having the functionality limited to a single page. Have you considered implementing this as a block?

Hi folks,

I was the one, who told to use single quotes. This may be okay for long url queries. But there where nearly all srtrings implemented with double quotes and this was unnessesary.

So, if you follow #11 it will be okay for me.

Here's what our community coding standards say about quotes: https://drupal.org/coding-standards#quotes Summary: not much. While many of us (myself included) have strong personal preferences between single or double quotes in code, those preferences are not community standards and don't warrant mentioning in a review like this. The double quotes were fine before. The single quotes are fine now. Let's stop worrying about quotes.

After installing the module I got the impression that the installation failed, because it said "Sorry, no results found." (which could be misleading as it resembles a 404 page)

For a better user-experience I would change the following:

1. Show the map, no matter whether there are results.
2. Change the default radius to 5000 so that the user understands visually how the module works.

Future improvement:
Why not use an interactive Google Map? It would be nice if the user could explore the area, while the local places on the right are updated instantly.

All in all, a working module with great potential for the future! :)

Module works as intended. The only problem I encountered was also having the radius set too low so I got the No Results page. It might be good to include something in the Readme or on the No Results page that suggests you increase the radius if you get no results, or else default to the highest level and note that you can reduce the radius if too many results are returned.

A great start on a very useful module. Suggestions for future improvements/added features:

1. It would be great if the Location could be set based on a geofield in a content type, so that the list of nearby places is displayed on the node created instead of limited to one Location.
2. Display Google Places as a layer on an OpenLayers map

In local_places.admin.inc you have:

'#description' => t(''),

There is no need to have this, its an added function that serves no purpose, just remove the line.

In local_places.module change the function _local_places_page() to this:

/**
 * Custom page callback function, for Home/Results pages.
 */
function _local_places_page() {
  if ($local_places_contents = local_places_contents()) {
    // No places found.
    $page_array['local_places_arguments'] = array(
      // Title serves as page subtitle.
      '#title' => t('Local Places'),
      '#markup' => t('Sorry, no results found'),
    );
  }
  else {
    $page_array['local_places_arguments'] = array(
      '#title' => t('Local Places'),
      '#markup' => theme('local_places_maplist', $local_places_contents),
    );
  }
  return $page_array;
}

No point having two return lines.
Change the function _local_places_details() to this:

/**
 * Custom page callback function, See local_places_menu(). FOR DETAILS PAGE.
 */
function _local_places_details() {
  if ($local_places_content_details = local_places_content_details()) {
    // No place found.
    $page_array['local_places_arguments'] = array(
      // Title serves as page subtitle.
      '#title' => t('Local Place Details'),
      '#markup' => t('Sorry, invalid place. Please go back and try again.'),
    );
  }
  else {
    drupal_set_title(check_plain($_GET['name'] . ' | ' . variable_get('local_places_title', 'Local Places')));
    $page_array['local_places_arguments'] = array(
      '#title' => t('Local Place Details'),
      '#markup' => theme('local_places_details', $local_places_content_details),
    );
  }

  return $page_array;
}

No point having two return lines again.

And change local_places_contents() function to this:

/**
 * Get Contents for the home/results page to load.
 */
function local_places_contents() {
  $vars = array();
  $vars['description'] = check_plain(variable_get('local_places_desc', ''));
  // Loads the results xml file.
  $xml = simplexml_load_file(local_places_xmlfile('list'));
  if ($xml->status == 'OK') {
    $location = variable_get('local_places_location', '51.5073,0.1276');
    // Get current URL without the query string, for next page results.
    $currenturl = explode('?', request_uri());
    $currenturl = $currenturl[0];
    // If the last character of the url is not '/' add a '/' to it.
    if (substr($currenturl, -1) !== '/') {
      $currenturl = $currenturl . '/';
    }
    $mapbaseurl = variable_get('local_places_mapurl', 'http://maps.googleapis.com/maps/api/staticmap?type=roadmap&sensor=false') . '&size=' . variable_get('local_places_lmapsize', '600x600') . '&zoom=' . variable_get('local_places_lmapzoom', 15) . '&center=' . $location;
    $markers = '&markers=color:red%7C' . $location;
    $i = 'A';
    $vars['results'] = array();
    $vars['results'][$i] = array();
    foreach ($xml->children() as $child) {
      // Loops through every child that has a name field (the results nodes).
      if (isset($child->name)) {
        $vars['results'][$i]['letter'] = $i;
        $name = $child->name;
        $vars['results'][$i]['name'] = $name;
        $ref = $child->reference;
        // Get place type from icon name.
        $icon = $child->icon;
        $vars['results'][$i]['icon'] = $icon;
        $rstring = 'http://maps.gstatic.com/mapfiles/place_api/icons/';
        $type = str_replace($rstring, '', $icon);
        $type = str_replace('-71.png', '', $type);
        $type = ucfirst($type);
        $vars['results'][$i]['type'] = $type;
        $lat = $child->geometry->location->lat;
        $lon = $child->geometry->location->lng;
        $vars['results'][$i]['detailsurl'] = $currenturl . 'details?name=' . $name . '&reference=' . $ref . '&type=' . $type . '&lat=' . $lat . '&lon=' . $lon;
        $vars['results'][$i]['address'] = $child->vicinity;
        if (isset($child->rating)) {
          $vars['results'][$i]['rating'] = $child->rating;
        }
        $markers = $markers . '&markers=color:blue%7Clabel:' . $i . '%7C' . $lat . ',' . $lon;
        $i++;
      }
      else {
        // If it is not a result, if it is the next token, get next link.
        if ($child->getName() === 'next_page_token') {
          $vars['nexturl'] = $currenturl . '?pagetoken=' . $child;
        }
      }
    }
    $vars['mapimg'] = $mapbaseurl . $markers;
    return $vars;
  }

  return FALSE;
}

Change local_places_content_details() to this:

/**
 * Gets Place Details contents for details page.
 */
function local_places_content_details() {
  // Loads the details xml file.
  $xml = simplexml_load_file(local_places_xmlfile('detail'));
  if ($xml->status == 'OK') {
    $mapbaseurl = variable_get('local_places_mapurl', 'http://maps.googleapis.com/maps/api/staticmap?type=roadmap&sensor=false') . '&size=' . variable_get('local_places_smapsize', '300x200') . '&zoom=' . variable_get('local_places_smapzoom', 16);
    $fullmapurl = 'https://maps.google.com/maps?hl=en&&q=';
    $name = $_GET['name'];
    $type = $_GET['type'];
    $lat = $_GET['lat'];
    $lon = $_GET['lon'];
    $fullmapurl = $fullmapurl . $lat . ',' . $lon;
    $fullmapurl = check_plain($fullmapurl);
    $markers = '&markers=color:red%7C' . variable_get('local_places_location', '51.5073,0.1276') . '&markers=color:blue%7C' . $lat . ',' . $lon . '&center=' . $lat . ',' . $lon;
    $mapimage = check_plain($mapbaseurl . $markers);
    $vars = array();
    $vars['name'] = $name;
    $vars['type'] = $type;
    $vars['lat'] = $lat;
    $vars['lon'] = $lon;
    $vars['mapurl'] = $fullmapurl;
    $vars['mapimg'] = $mapimage;
    foreach ($xml->children() as $child) {
      if (isset($child->name)) {
        $vars['icon'] = $child->icon;
        $vars['address'] = $child->formatted_address;
        if (isset($child->formatted_phone_number)) {
          $vars['tel'] = $child->formatted_phone_number;
        }
        if (isset($child->website)) {
          $vars['website'] = $child->website;
        }
        $vars['googleurl'] = $child->url;
        if (isset($child->review)) {
          $vars['reviews'] = array();
          $i = 1;
          foreach ($child->children() as $granchild) {
            if ($granchild->getName() === 'review') {
              if ($i <= 5 && $granchild->text !== '') {
                $vars['reviews'][$i] = array();
                if ($granchild->author_url != '') {
                  $vars['reviews'][$i]['author'] = '<a href="' . $granchild->author_url . '" target="_blank" title="Author\'s Google Plus Page (new window)">' . $granchild->author_name . '</a>';
                }
                else {
                  $vars['reviews'][$i]['author'] = $granchild->author_name;
                }
                $vars['reviews'][$i]['text'] = $granchild->text;
                $i++;
              }
            }
          }
        }
      }
    }
    return $vars;
  }

  return FALSE;
}

I would recommend looking at drupal_http_build_query() for this:

/**
 * Build URL for Google Places XML File.
 */
function local_places_xmlfile($display) {
  $baseurl = variable_get('local_places_url', 'https://maps.googleapis.com/maps/api/place/nearbysearch/xml?sensor=false');
  $api_key = variable_get('local_places_key', 'Enter your API Key');
  $location = variable_get('local_places_location', '51.5073,0.1276');
  $radius = variable_get('local_places_radius', 50000);
  $types = variable_get('local_places_types', 'cafe|restaurant');
  $xmlurl = $baseurl . '&key=' . $api_key . '&location=' . $location . '&radius=' . $radius . '&types=' . $types;
  // Gets the page token from the query string if on results page 2 or 3.
  if (isset($_GET['pagetoken'])) {
    $xmlurl = $xmlurl . '&pagetoken=' . $_GET['pagetoken'];
  }
  if ($display === 'detail') {
    $baseurl = 'https://maps.googleapis.com/maps/api/place/details/xml?sensor=false';
    $xmlurl = $baseurl . '&key=' . $api_key . '&reference=' . $_GET['reference'];
  }
  return $xmlurl;
}

Thats what I can find for now on code improvements. Otherwise it appears to function as intended.

There also appears to be a few errors:
http://pareview.sh/pareview/httpgitdrupalorgsandboxtraceyspencer2002766git
Change your hook_help to:

/**
 * Loads Help Contents.
 */
function local_places_help($path, $arg) {
  switch ($path) {
    case 'admin/help#local_places':
      return '<p>' . t('Displays local places, using the Google Places API.') . '</p><p>' . t('You must get a Google Places API Key before attempting to use this module. It will not work without one. Once you have a Google Places API Key, please enter it in the configuration form, before attempting to view any local places. After completeing the configuration form, please browse to [YOURSITE}/local_places/.') . '</p>';
  }
}

Sorry my bad, needs to be this :)

In local_places.module change the function _local_places_page() to this:

/**
 * Custom page callback function, for Home/Results pages.
 */
function _local_places_page() {
  if ($local_places_contents = local_places_contents()) {
    $page_array['local_places_arguments'] = array(
      '#title' => t('Local Places'),
      '#markup' => theme('local_places_maplist', $local_places_contents),
    );
  }
  else {
    // No places found.
    $page_array['local_places_arguments'] = array(
      // Title serves as page subtitle.
      '#title' => t('Local Places'),
      '#markup' => t('Sorry, no results found'),
    );
  }
  return $page_array;
}

Change the function _local_places_details() to this:

/**
 * Custom page callback function, See local_places_menu(). FOR DETAILS PAGE.
 */
function _local_places_details() {
  if ($local_places_content_details = local_places_content_details()) {
    drupal_set_title(check_plain($_GET['name'] . ' | ' . variable_get('local_places_title', 'Local Places')));
    $page_array['local_places_arguments'] = array(
      '#title' => t('Local Place Details'),
      '#markup' => theme('local_places_details', $local_places_content_details),
    );
  }
  else {
    // No place found.
    $page_array['local_places_arguments'] = array(
      // Title serves as page subtitle.
      '#title' => t('Local Place Details'),
      '#markup' => t('Sorry, invalid place. Please go back and try again.'),
    );
  }

  return $page_array;
}

access administration vs. administer site configuration

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.

• You have a few other errors reported here: http://git.drupal.org/sandbox/traceyspencer/2002766.git.
• Your branch should be named "7.x-1.x" instead of "7.x-1.x-dev". You can enable snapshot releases on the project page once it's approved, which will make a 7.x-1.x-dev package available.
• What is this default value? variable_get('local_places_location', '51.5073,0.1276'); Somewhere near London?
• In local_places_contents() you can use

Setting to "needs work" for the permission issue above, otherwise this looks ready to go.
I posted on https://drupal.org/node/1211304 to point them to your module, you may find more inspiration there.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Hmm, good question! :)

I think I was gonna say you could use drupal_http_build_query() instead of building the string yourself, but in your implementation it may not make things much easier. So you could go either way really.

Thanks for making the rest of those changes.

----
Top Shelf Modules - Crafted, Curated, Contributed.

No further action is required, but the best thing you can do is get a Review Bonus by reviewing other applications. That will get you to the top of the list of projects to get reviewed (and hopefully approved). Only manual reviews count, just using http://pareview.sh is not enough.

----
Top Shelf Modules - Crafted, Curated, Contributed.

It actually hasn't been approved yet, that's the next step. The Review Bonus can speed that up, but it's not required.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Well, it's been a month without any further issues, so...

Thanks for your contribution, traceyspencer!

I updated your account to let you 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 get 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.