jcmc [jcmc]

The previous application was #531764: jcmc [jcmc].

We just review a module/theme per applicant; choose the one you want to be reviewed.

1.   $fieldname[$type] = $type == 'gallery_assist' ? 'Gallery assist' : 'Gallery assist '. t('on') .' '. t($type_name);
   

   The string is not completely translated; the first argument of t() needs to be a literal string, or the script to extract the strings to translate will not extract anything.

2.   $form = array();
     if (empty($type)) {
       return system_settings_form($form);
     }
   

   If the variable is empty, then the user will just see an empty form with two buttons (Save, and Restore to defaults), which is pretty useless.

3.     '#title' => t('Margin betwen items'),
   

   Betwen?

4.       '#title' => t($v->name),
         '#default_value' => $default_value,
         '#description' => t($v->description) .'<div class="status">'. t('Check to assign the gallery functionality on this content type.') .'</div>',
   

   See my previous note.

5.   $presets = imagecache_presets();
     foreach ($presets as $presetid => $value) {
       $options[$presetid] = $value['presetname'];
     }
   

   Is the module installed?

6.     '#description' => 'active:'. $settings['thm_presetname'],
   

   Apart that the description doesn't seem much useful, do you see anything wrong with that code?

7. function gallery_assist_copyright() {
     $output  = '<div style="text-align: right;">'. t('Module developed by') .' Juan Morejón  - ';
     $output .= l('jcmc', 'http://simple.puntolatinoclub.de', array('attributes' => array('target' => '_blank'))) .' from the '. l('Quiptime-Group', 'http://www.quiptime.com', array('attributes' => array('target' => '_blank'))) .'</div>';
   
     return $output;
   }
   

   What is wrong with that code?

8.   $schema['gallery_assist'] = array(
       'description' => t('Table for manage the fields of gallery assist node type and gallery assist assigments.'),
       'fields' => array(
         'gid' => array(
           'description' => t('The primary identifier for a GAllery.'),
   

   Schema descriptions should not be passed to t() anymore.

9.         'description' => t('The revition id of the node.'),
   

   Revition?

10.         'description' => t('The GAllery id.'),
    

    The gallery ID.

11.       'lang' => array(
            'description' => t('The DB-Name of the GAllery.'),
            'type' => 'varchar',
            'length' => 8,
            'not null' => TRUE,
            'default' => ''),
          'ptitle' => array(
            'description' => t('The DB-Name of the GAllery.'),
            'type' => 'varchar',
            'length' => 100,
            'not null' => TRUE,
            'default' => ''),
          'palt' => array(
            'description' => t('The DB-Name of the GAllery.'),
            'type' => 'varchar',
            'length' => 100,
            'not null' => TRUE,
            'default' => ''),
          'pdescription' => array(
            'description' => t('The DB-Name of the GAllery.'),
            'type' => 'varchar',
            'length' => 255,
            'not null' => TRUE,
            'default' => ''),
    

    The description is the same for all those fields.

12. function gallery_assist_install() {
      drupal_install_schema('gallery_assist');
      db_query('ALTER TABLE {gallery_assist_translated} CHANGE pdescription pdescription LONGTEXT');
    
    

    Why didn't you changed the schema for that field in gallery_assist_schema()? That SQL query is then making an assumption about the database engine being used.

13.   if (!file_check_directory(variable_get('gallery_assist_directory', file_directory_path() .'/gallery_assist'), 1)) {
        drupal_set_message('It was not posible to create the gallery_assist directory. Pleasy check the write permisions of your files directory. If this error persist contact the administrator.');
      }
    

    See posible, pleasy, permisions, error persist. There is also something else wrong (see the call to drupal_set_message().

14.   // Init gallery_assist.
      variable_set('gallery_assist_default', 1);
      variable_set('gallery_assist_default_data', $ga_variables);
      variable_set('gallery_assist_gallery_assist', 1);
      variable_set('gallery_assist_gallery_assist_data', $ga_variables);
    

    There is no need to initialize Drupal variables on the installation function.

15.   // Delete all gallery assist settings variables.
      $string = 'gallery_assist_%';
      db_query("DELETE FROM {variable} WHERE name LIKE '%s'", $string);
    

    It is sufficient to use db_query("DELETE FROM {variable} WHERE name LIKE 'gallery_assist\_%'", $string).

16.     drupal_set_message('This node muss be saved before you can add items to the gallery. Read more about in the '. l('help', $base_url .'/gallery_assist'. $hepl_path_tail .'/help#ga_why') .'.');
    

    What is wrong with that code?

17.   // Look for submited data from tab extras of gallery assist.
      if ($form_id == 'gallery_assist_settings_extras') {
        $form['#submit'][] = 'gallery_assist_nodetypes_submit';
      }
    
      // Look for submited settings data of gallery assist or gallery assist assigments.
      if ($form_id == 'gallery_assist_settings') {
        $form['#submit'][] = 'gallery_assist_settings_submit';
      }
    

    The code is altering a form it defines through hook_form_alter(). Why didn't the code set the submission function directly in the function that defines its form?

18.       '#description' => t('By enable display gallery_assist the item title as image capture.'),
    

19.       '#description' => t('Choose a weight value to decide witch position will be having your gallery container.'),
    

    Witch position? Are there any witches around?

20.       '#prefix' => '<div class="gallery_assist_list">',
          '#suffix' => '</div>',
          '#weight' => -3.1,
          '#description' => t('Click Update to upload a new file or to save your changes in this section.'),
    

    Weight is an integer, not a floating point value.

21.       if ($node->gallconf[$node->type]['use_imagecache'] == 1) {
            //$ic_thm = imagecache_preset(variable_get('gallery_assist_'. $node->type .'_ic_thumbnail', FALSE));
            $item->image = theme('imagecache', $node->gallconf[$node->type]['thm_presetname'], $item->opath, $item->palt, $item->ptitle, array('style' => 'height:80px; width:80px;'));
          }
    

    The code doesn't check if the module is enabled.

22. function theme_gallery_assist_item_submitted(&$node) {
      return 'Submitted by'.' '. theme('username', $node) .' '.'on '. format_date($node->file_created, 'custom', $node->dateformate);
    }
    

    What is wrong with that code?

Remember to change the status, when you upload new code.

1.   if (!empty($preview)) $link[] = $preview;
     if (!empty($next)) $link[] = $next;
   

   Can you say what is wrong in that code?

2.   t('@gallery_assist on @typename', array('@gallery_assist' => t('Gallery Assist')
   

   There is no need to use a placeholder with a string that is not dynamic.

3.     '#options' => array('0' => 'Numeric', '1' => 'Icons'),
   

   Every string that appears in the user interface must be translated.

4.   $presets = imagecache_presets();
     foreach ($presets as $presetid => $value) {
       $options[$presetid] = $value['presetname'];
     }
   

   Rather than keeping to check if the module is enable (which is not done from the above code), it would be better to add it to the dependencies of this module.

5.     '#description' => t('Current used setting').': '. $settings['prev_presetname'],
   

   It would be better to use a placeholder.

6. function gallery_assist_copyright() {
     return t('Module developed by !ga_creator_link from the !qtg_link', array(
       '!ga_creator_link' => l('Juan Morejón', 'http://simple.puntolatinoclub.de', array('attributes' => array('target' => '_blank'))), 
       '!qtg_link' => l('Quiptime Group', 'http://www.quiptime.com', array('attributes' => array('target' => '_blank'))),
     ));
   }
   

   l() is not used together t(); that is reported in the documentation page for t(), which reports the following text:

   Here is an example of incorrect usage of t():

   $output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));
   

   Here is an example of t() used correctly:

   $output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';
   

7.   version = "6.x-1.5"
   

   That line needs to be removed.

8.   variable_set('gallery_assist_default', 1);
     variable_set('gallery_assist_default_data', $ga_variables);
     variable_set('gallery_assist_gallery_assist', 1);
     variable_set('gallery_assist_gallery_assist_data', $ga_variables);
   

   There is no need to set the default value for Drupal variables. What is the second parameter of variable_get()?

9.           if ($node->show_title == 1) {
               $toggle = 'Hide titles';
               $toggle_value = 'off';
             }
   
             if ($node->show_title == 0) {
               $toggle = 'Show titles';
               $toggle_value = 'on';
             }
   

   $toggle contains a string used in the user interface, but it is not translated.

10.   $output = 'Import finished. To see your changes '. l('refresh here', 'node/'. $node->nid .'/edit') .' or '. l('go to the node view', 'node/'. $node->nid .'/edit') .'.';
    

    I let you guess what is wrong with the code.

1.   $presets = imagecache_presets();
     foreach ($presets as $presetid => $value) {
       $options[$presetid] = $value['presetname'];
     }
   

   The code is executed only when imagecache.module is enabled (the check is done before the function containing the above mentioned code is called). What I previously reported is then not exact.

2. Some strings that are used in the user interface are not translatable; check for any of such strings (see the array indexes #title, #options, and #description).

3.   $dscan1 = file_scan_directory($import_path, '.', array(), 0, FALSE);
   

     $pager_path_theme = drupal_get_path('theme', variable_get('theme_default', FALSE)) .'/ga_pager';
     $scan['theme'] = file_scan_directory($pager_path_theme, '.', $nomask = array('.', '..', 'README.txt'), 0, FALSE);
     $pager_options = array('default' => 'default');
   

   Probably, the second argument of file_scan_directory should be '.*', if you are not looking for files with a name of just one character. The third argument should at least contain the default values (array('.', '..', 'CVS')).