Google Map Field

welcome,

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.

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow tips for a great project page. Also create a README.txt that follows the guidelines for in-project documentation.

Remove the license file as it will be added automatically later and all code hosted on d.o is under gpl anyway. Also the old $id$ tags are deprecated since we got git on drupal.org.

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxscothubbard1531046git

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

Git can be a real pain

I like this module - all seems ok in coder, and it is simple to run and set up (no problems here).

I would consider looking into use hook_help (http://api.drupal.org/api/drupal/modules%21help%21help.api.php/function/...) and possible hook_permission (http://api.drupal.org/api/drupal/modules%21system%21system.api.php/funct...) to see if you only want to allow certain people to be able to change map data - could see a case for this.

Only UX issue I found was that if you allow the field to have multiple values - you can't set them using the map?
Maybe limit the field to one value?

Very impressed though, well done.

Ahhh, I see now. I get 5 maps if I say have 5 values, not one map to which I could add multiple values :)

It might be interesting to support multiple default map positions.

Could I give maps a label?

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.

  $js = '
    var map_' . $delta . ';
    jQuery(document).ready(function(){
      var latlng = new google.maps.LatLng(' . $lat . ', ' . $lon . ');
      var mapOptions = {
        zoom: ' . $zom . ',
        center: latlng,
        streetViewControl: false,
        mapTypeId: google.maps.MapTypeId.ROADMAP
      };
      map_' . $delta . ' = new google.maps.Map(document.getElementById("google_map_field_' . $delta . '"), mapOptions);
      // add a marker to the map for the dealer
      marker = new google.maps.Marker({
        position: latlng,
        map: map_' . $delta . ',
      });

Javascript should go into .JS files, and parameters/settings should be passes with the settings function ... see http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_add...

<div class="google-map-field"><div class="google-map-field-label">' . $name . '</div><div id="google_map_field_' . $delta . '" style="width: ' . $wth . 'px; height: ' . $hth . 'px;"></div></div>

Markup should best be placed into theme functions or template files ...

return t("To insert a google map field, <a href='/google-map-field/token-builder' target='_new'>click here for the token builder</a> (opens in a new window)");
Links should be created with the l() function, and html should not be passed to t()!

The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could for example start by evaluating my own project:
http://drupal.org/node/1302786

Did you check the settings part @ http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_add... ?

you could create arrays with drupal.settings, even so I'm not an expert on this subject, I suppose it should be possible...

@scot, your reviews are quite short, but I won't judge on them ;)

1. Callback functions should not be "hidden" functions, so remove "_" at the geginning:

  $filters['google_map_field_token'] = array(
    'title' => t('Google Map Field'),
    'description' => t('Use tokens to insert Google Map Fields.'),
    'process callback' => '_google_map_field_filter_process',
    'cache' => FALSE,
    'tips callback' => '_google_map_field_filter_tips',
  );

2. It's bad practice setting height and width fix in code:
'#markup' => '<div id="google_map_field_selector" style="width: 300px; height: 250px;"></div>'
they should be defined in CSS files!
I would even suggest putting as much html-code into theme functions or template files.

--
The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could consider evaluating my own application:
http://drupal.org/node/1302786

I agree with luxaparazzi... it would be good to try and get as much of your markup as possible in to theme functions or tpl's.

I'd also suggest that you add some simple validation to the form values used by the field as it will let you add some values that'll break the output of the field.

Also it looks as though the field does not respect the 'required field' option when adding the field to an entity.

Other than that though I like the look of this module. It looks nice and lightweight.

Thanks for your participation, but I'm afraid the comments you did are not very detailed and IMHO not really "reviews".
A good deep review can take up to an hour, this is not about just pointing out 2 things and set the issues to needs work.

Sorry, removing tag.

Added project review link.

Thanks for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

manual review:

1. google_map_field_help(): all user facing text must run through t() for translation.
2. "* Implements hook_menu().": There should be only one space after the "*". Also elsewhere.
3. google_map_field_menu(): please add a comment why the menu path is unprotected, so that I don't have the feeling that this is a security issue.
4. "t('Invalid value - ') . $element['#title']": do not concatenate translatable strings, use placeholders in t() instead.
5. google_map_field_filter_process(): put the JS in a separate file for easier maintenance and pass the settings to it.
6. google_map_field_filter_process(): do not use jQuery(document).ready() but Drupal.behaviors, see http://drupal.org/node/304258 and http://drupal.org/node/756722 . Same for all your JS files.
7. google_map_field_wysiwyg_include_directory(): that is a hook implementation and should be documented as such, see http://drupal.org/node/1354#hookimpl
8. Javascript: please use the same documentation guidelines as for PHP, i.e. please document every function.
9. google_map_field_format_field(): this is not a hook?
10. Your module is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as map name I get a nasty javascript popup. Please read http://drupal.org/writing-secure-code and all subpages, especially http://drupal.org/node/28984

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

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

manual review:

1. google_map_field_field_is_empty(): doc block is wrong.
2. "t('Invalid value - :title', array(':title' => $element['#title']))": The ":" is not a valid placeholder character for t().
3. google_map_field_edit_form.js: not entirely sure, but I think you should prefix the functions with your module name to avoid name collisions.

But otherwise this looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Your project page could still be enhanced by making it a little more structured you may have a look at the tips for a great project page.

You should make the label translatable:
'label' => 'Google Map Field',

FYI, this has probably no effect.

'instance_settings' => array(
'cardinality' => 1,
),

to make your function naming consistent may should rename google_map_field_default to google_map_field_formatter

'default_widget' => 'google_map_field_widget',
'default_formatter' => 'google_map_field_default',

Maybe not a bad idea to make "http://maps.googleapis.com/maps/api/js?sensor=false" configurable ?

After all I don't think here's anything serious.

Thanks for your contribution and 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.

Thanks to the dedicated reviewer(s) as well.