mistergroove [mistergroove]

Please change only the status, when you upload new code.

The Apply for contributions CVS access reports that

Module 'Foo' is just too complex so I have written a much simpler version. This is just straight duplication of functionality and applications like this will almost certainly be declined. It is worth remembering that many modules start out simple also and, over time, grow into more complex systems. If you have the know how to write a "simple version" from scratch rather than work out how a complex module works then maybe your effort would be better spent making the existing complex module easier to use (improve it's UI, write documentation, etc).

If the module only creates blocks, then the modules have two different purposes.

1. function simplemap_settings() {
   
     // play nice with the gmap module
     if (module_exists('gmap')) {
   
       $form['simplemap_googleapikey'] = array(
         '#type' => 'markup',
         '#title' => t('Google Maps API Key'),
         '#value' => t('Since the GMAP module is installed Simplemap will use the Google Maps API key provided by it.  You can configure this setting under <a href="@gmapsettings">admin->settings->gmap</a>.  If the GMAP module is removed you will be able to add your api key here.  You can get a key by visiting <a href="@googlemappath">Google maps API sign up page</a>', array('@gmapsettings'=>url('admin/settings/gmap'),'@googlemappath'=>url('http://code.google.com/apis/maps/signup.html'))),
       );
   
     } else {
   
       $form['simplemap_googleapikey'] = array(
         '#type' => 'textfield',
         '#title' => t('Google Maps API Key'),
         '#description' => t('Please provide a valid google api key.  You can get a key by visiting <a href="@googlemappath">Google maps API sign up page</a>', array('@googlemappath'=>url('http://code.google.com/apis/maps/signup.html'))),
         '#default_value' => variable_get('simplemap_googleapikey', ''),
       );
       $form['#submit'][] = 'simplemap_settings_submit';
       $form['buttons']['submit'] = array('#type' => 'submit', '#value' => t('Save configuration') );
   
     }
   
     return $form;
   }  
   
   function simplemap_settings_submit($form, &$form_state) {
     variable_set('simplemap_googleapikey',$form_state['values']['simplemap_googleapikey']);
   }
   

   If the code would use system_settings_form(), there would not be the need to use a submission function; the form would automatically save the values in Drupal variables.

2. function simplemap_add_block_form(&$form_state) {
     $key = simplemap_get_api_key();
     if (empty($key)) {
       drupal_set_message('You must set a Google Maps API key in the Simplemap settings before you can add map blocks.','error');
       drupal_goto('admin/build/block');
     }
     include_once './' . drupal_get_path('module', 'block') . '/block.admin.inc';
     return block_admin_configure($form_state, 'simplemap', NULL);
   
   }
   

   The code could use module_load_include().

3. /**
    * Implements hook_enable().
    */
   function simplemap_enable() {
     drupal_set_message(t('To use simplemap blocks, find the "Add map block" tab (or button) on the <a href="@url">administer blocks page</a>.', array('@url' => url('admin/build/block'))));
   }
   

   The message would be shown all times the module is enabled. It would be better to add the information in the help text of the module.

4.   define('SMNL',"\n");
   

   All the constants name should start with the name of the module written all in upper case letters.

5. /**
    * Alters the block admin form to add delete links next to menu blocks.
    */
   function simplemap_form_block_admin_display_form_alter(&$form, $form_state) {
     module_load_include('inc', 'menu_block', 'simplemap.admin');
     _simplemap_form_block_admin_display_form_alter($form, $form_state);
   }
   

   It would be better to merge the code of the two functions, rather than unconditionally include a file just to use a function.

6. /**
    * Implements hook_block().
    */
   function simplemap_block($op = 'list', $delta = NULL, $edit = NULL) {
   
     $function = 'simplemap_block_' . $op;
     
     if (function_exists($function)) {
       return $function($delta, $edit);
     } else {
       module_load_include('inc', 'simplemap', 'simplemap.admin');
       if (function_exists($function)) {
         return $function($delta, $edit);
       }
     }
   }
   

   The code would be simpler, if the functions would all be in the same file. For what I can see, only simplemap_block_view() is defined in the module file.

7. function simplemap_get_theme_marker_image($key) {
     static $imagearray = false;
     
     if (!is_array($imagearray)) {
       $imagearray = array();
       $activetheme = variable_get('theme_default', 'garland');
       $themepath = drupal_get_path('theme', $activetheme);
       $markerconfigpath = $_SERVER['DOCUMENT_ROOT'].'/'.$themepath.'/simplemap.markers';
       if (file_exists($markerconfigpath)) {
         $imagebasepath = '/'.$themepath.'/';
         $options = parse_ini_file($markerconfigpath,true);
         foreach($options as $class=>$settings) {
           $icon = new StdClass();
           foreach($settings as $key=>$setting) {
             $tree = explode('-',$key);
             $valuekey=array_pop($tree);
             $current = $icon;
             if (!empty($tree)) {
               foreach($tree as $treeitem) {
                 if (!isset($current->{$treeitem})) $current->{$treeitem} = new StdClass();
                 $current = $current->{$treeitem};
               } 
             } 
             if ($valuekey=='path') $setting = $imagebasepath.$setting;
             $current->{$valuekey} = $setting;
           }
           if ($icon->image->path && (!$icon->image->width || !$icon->image->height)) {
             if ($size = simplemap_get_marker_dimensions($icon->image->path)) {
               if (!$icon->image->width) $icon->image->width = $size['width'];
               if (!$icon->image->height) $icon->image->height = $size['height'];
             }
           }
           if ($icon->shadow->path && (!$icon->shadow->width || !$icon->shadow->height)) {
             if ($size = simplemap_get_marker_dimensions($icon->shadow->path)) {
               if (!$icon->shadow->width) $icon->shadow->width = $size['width'];
               if (!$icon->shadow->height) $icon->shadow->height = $size['height'];
             }
           }
           $imagearray[$class] = $icon;
         }
       }
     }
     if (isset($imagearray[$key])) return $imagearray[$key];
     return false;
   

   See the Drupal coding standards to understand how a module code should be written.

8. function _simplemap_help($path, $arg) {
     $output = '';
     switch ($path) {
       case 'admin/build/block':
         $output = '<p>' . t('Use the <a href="@url">add map block page</a> to create a customizable map block. You will then be able to configure your map block before adding it.', array('@url' => url('admin/build/block/add-map-block'))) . '</p>';
         if (module_exists('help')) {
           $output .= theme('more_help_link', url('admin/help/simplemap'));
         }
       break;
   

   If the module help.module would not be enabled, the implementation of hook_help() would not be invoked, and so would do _simplemap_help(). Thefore, there is no need to verify if that module is enabled.
   It's not clear to me why to put the code in a private function that is only called by the implementation of hook_help().

I think it's helpful to get instructions when a modules is installed. I've seen this in a few existing modules and I kind of liked it. But if this is not standard I'm happy to remove it.

If you need to give instructions when a module is installed, then the message can be given in the installation function. The implementation of hook_enable() is not thought to be executed when a module is installed, but when the module is enabled.

This approach means that drupal doesn't include a lot of unnecessary code that is only used rare events - such as configuring. This is about keeping the drupal lean and fast.

I agree with that. Still, I don't see why to leave a single function on the module file, and the other ones in the file that is included; it would rather be better to move also simplemap_block_view() in the other file.

function simplemap_add_js_external($url) {
  static $staticurls = array();
  if (!in_array($url,$staticurls)) {
    drupal_add_js('//--><!]]>'.SIMPLEMAP_NL.'</script>'."\n".'<script type="text/javascript" src="'.$url.'" ></script>'.SIMPLEMAP_NL.'<script type="text/javascript">'.SIMPLEMAP_NL.'<!--//--><![CDATA[//><!--', 'inline');
    $staticurls[] = $url;
  }
}

The code could use drupal_set_html_head().

The file simplemap.pages.inc is used for just a function. In that case, the code can be moved in the module file.
An external file is created for more that one function; differently, there would not be any vantage on using it (the overload to load the file would kill the vantage obtained to move the code outside the module file).

When I have doubts about how to implement something, I look at what Drupal core code does.
Drupal doesn't use a file for just a function; I guess there would be a reason.

I understood what you meant, and I agree with that. My point is that you should not create a file for just a function; it is perfectly fine if you move the function into simplemap.admin.inc, which is used also for other functions.

I would also move there simplemap_block_view(); that would simplify the code that would not need to first verify if the function exists, then load the file and check again if the function exists.
Really, the code could be simplified as:

/**
* Implements hook_block().
*/
function simplemap_block($op = 'list', $delta = NULL, $edit = NULL) {
  module_load_include('inc', 'simplemap', 'simplemap.admin');
  
  switch ($op) {
    case 'list':
      return simplemap_block_list($delta, $edit);
    
    case 'view':
      // ...

    case // ...
   }
}

Remember to change status, when you upload new code.

1. function simplemap_get_marker_images(&$location) {
     $default = simplemap_get_theme_marker_image('simplemap'.$location['id'].'-default');
     if (!$default) $default = simplemap_get_theme_marker_image('default');
     foreach($location['markers'] as $key=>$marker) {
       if ($markericon = simplemap_get_theme_marker_image('simplemap'.$location['id'].'-marker'.($key+1))) {
         $location['markers'][$key]->icon = $markericon;
       } elseif ($default) {
         $location['markers'][$key]->icon = $default;
       }
     }
   }
   

   The IF-statements are not formatted as they would.

2. function simplemap_get_location($delta) {
     if ($locationarray = variable_get('simplemap_block_'.$delta,false)) { 
       $locationarray['id'] = $delta;
       $locationarray['key'] = $delta;
       simplemap_get_marker_images($locationarray);
       return $locationarray;
     }
     return false;
   }
   

   Constants must be written all in upper case characters.

3.     $block['subject'] = t($mapsettings['title']);
   

   The first argument of t() must be a literal string, not a variable; differently the script to extract the strings to translate will not extract the string to translate.

4.     $mappageid = 'simplemap'.$settings['id'];
   

   There should be a space before, and after the concatenation string.

5.     $locationtitle = (isset($location['title']))?$location['title']:$key;
   

   The same is true for this code line.

Change the status to needs review, when you upload new code.

As for point 3 I need to be able to pass the user edited title into the translate interface so that people can add translations of the titles they add in.

As I reported, that doesn't happen, if the argument of t() is not a literal string (in other words, it must be a constant value). Using a call like t($variable) has the same effect of using $variable.
The error reported by the script that extract the strings to translate is the following: The first parameter to t() should be a literal string. There should be no variables, concatenation, constants or other non-literal strings there..

I'm not using constants in point 2 (simplemap_get_location) so I couldn't understand that one.

FALSE is a constant, and it should be written all in upper case characters; you wrote false.

Do you have any ideas on how to present user input text to the translate interface?

If you know the string that will be passed to t(), and they are limited, you can create a file (possibily with a name prefixed by the name of the module) that will never be used from the module, and that contains something like

  t('First string');
  t('Second string');

Clearly, this can be done if the strings are in a closed set. If the strings are the input of users, then it is not possible to do what I said.

To wait until somebody approves it, or reports something that needs to still be changed. :-)

Jokes apart, I will review the code later. As you can see, there are a lot of CVS applications that need to be reviewed.

            if (!$icon->shadow->width) $icon->shadow->width = $size['width'];

There are some IF-statements that need to be fixed.