jcmc [jcmc]

The function gallery_assist_item_box() uses a variable that is not initialized, it's not a parameter of the function, and it's not a global variable. It then executes the following code:

  function gallery_assist_item_box(&$item, $conf) {
    $my = (object)$my;
    // ...
  }

The same error is present in most of the module functions.

The module then contains two functions that have their name not prefixed by the module name: ga_makedirs(), and object2array().

A quick look revealed an access by-pass:-

    $res = db_query("SELECT g.gid, n.title FROM {gallery_assist} g INNER JOIN {node} n ON g.nid=n.nid WHERE g.uid=%d", $user->uid);

When selecting from or joining to the node table:-

1. You should check the node status in the WHERE portion with n.status = 1
2. You should run the SQL through db_rewrite_sql() function

)Please dont forget to change the status to "needs review" if you want us to look at it again.

Please don't play with the issue stuff :)

1. function gallery_assist_node_info() {
     return array(
       'gallery_assist' => array(
         'name' => t('GAllery'),  // <-- It should be Gallery
   

2. (in hook_nodeapi())

         if (user_access('view gallery_assist content')) {
           if ($node->type != 'gallery_assist') {
             return;
           }
   

   If the module is handling only its content type, then that code should be in another hook. Modules that implement a content type has hook to show the edit form, to show the node content, etc...; hook_nodeapi() is used by a module that changes in any way a content type that it doesn't define.

3. substr(), and strlen() are still used instead of the equivalent Drupal functions, which should be preferred because they are able to handle Unicode characters even when the multibyte string functions are not enabled.

4.   $my['simbol'] = variable_get('gallery_image_size', 0);
   

   There is no reason to create an array to just contain a item; in such cases, it's better to use a simple variable.

5. Functions, and Drupal variables must use the namespace reserved to the module (see image_get_more_info()).

6. name = Gallery Assist ; <-- it would be better to use Gallery assist
   description = "Drupal node type with gallery funtionality." ; <-- functionality
   

7. A function is declared an implementation of hook_display_item(), which is not a Drupal core hook; the module doesn't declare any dependency from other modules. Is the function not a hook, or does the module forget to declare the dependency from a module?
8. The same is true for hook_item_pager().
9. Actually, there is another module that implements a gallery (image_gallery.module), which is part of Image; I don't see a reason of having another module to do the same thing.

10. /*------------------------------ THEMES SECCION ------------------------------*/
    

    /*-------------------------- DATA PROCECING SECCION --------------------------*/
    

11. function gallery_assist_makedirs(&$node) {
      $my = array();
      $my['files_folder'] = file_directory_path();
      $my['user_upload_folder'] = $my['files_folder'] ."/". $node->uid;
      $my['gallery_upload_folder'] = $my['user_upload_folder'] ."/ga". $node->nid;
      $my['gallery_thumbnails_folder'] = $my['gallery_upload_folder'] ."/thm";
      $my['gallery_previews_folder'] = $my['gallery_upload_folder'] ."/prev";
      if (is_dir($my['user_upload_folder']) && !is_dir($my['gallery_upload_folder'])) {
        if (file_check_directory($my['gallery_upload_folder'], 1));
      }
      if (is_dir($my['gallery_upload_folder']) && !is_dir($my['gallery_thumbnails_folder'])) {
        if (file_check_directory($my['gallery_thumbnails_folder'], 1));
      }
      if (is_dir($my['gallery_upload_folder']) && !is_dir($my['gallery_previews_folder'])) {
        if (file_check_directory($my['gallery_previews_folder'], 1));
      }
    }
    

    The code uses an array when it could use a set of variables.

As there is already a module for image galleries, which integrates with Views and CCK, I don't see any reason to have another module that has the same purpose.
This module gets a -1.

This time is my fault (we both were adding a comment at the same issue).

1.   if (variable_get('gallery_image_size', 0) == '') {
       variable_set('gallery_image_size', 100);
     }
   

   It would be better to use 100 as the default value for that Drupal variable. It's not clear why the code checks if the value of the Drupal variable is an empty string, when it is supposed to be an integer; if it is done because the user can use an empty string for input, then it would be better to put that code in the submission function.

2.   t('1. Pager position'),
   

   Remove the numbers from the titles; they are not required.

3.   $my->ph  = '<div id="config-img" class="clear-block">';
     $my->ph .= theme('image', drupal_get_path('module', 'gallery_assist') .'/img/pager-settings.png', '', "Pager position and quantity of links numbers");
     $my->ph .= '</div>';
   

   The variable is not initialized; even if it would be, it enough to use a normal variable (not an array, not an object).

4. gallery_assist.admin.inc defines only a function. As it is, the code can be moved in the module file.

5.   // create user gallery_assist directory if permitions
   

     // Create the user gallery_assist directory if the user has the right permission.
   

   Comments must start with a capital letter, and end with a period.

6.     case 'admin/help#gallery_assist':
         $output = '<p>'. t('The gallery_assist module allows registered users or users with the gallery_assist administration permitions to create and maintain picture galleries.') .'</p>';
         $output .= '<p>'. t('For more information, see the online handbook entry for <a href="@gallery_assist">Gallery Assist</a>.', array('@gallery_assist' => 'http://jcmc.quiptime.com/gallery_assist/node/48')) .'</p>';
         return $output;
   	  breack;
   

   It is "break".

7. function gallery_assist_view($node, $teaser = FALSE, $page = FALSE) {
     if (user_access('view gallery_assist content')) {
       if ($node->type != 'gallery_assist') {
         return;
       }
   

   The hook is called only for the content type it defines: why is the code checking the content type of the node, then?

8.     if ($page) {
         dsm($node);
   

   dsm() is a debug function.

9.   $my['grafic_next_path'] = file_exists(drupal_get_path('theme', $theme) .'/ga_pager/'. $my['pager_layout'] .'/next.png') ? drupal_get_path('theme', $theme) .'/ga_pager/'. $my['pager_layout'] .'/next.png' : drupal_get_path('module', 'gallery_assist') .'/ga_pager/default/next.png';
     $my['grafic_next'] = theme('image', $my['grafic_next_path'], '', "go to the next");
     $my['next'] = variable_get('gallery_pager_simbol', 0) == 0 ? t('next ›') : $my['grafic_next'];
     $my['grafic_previous_path'] = file_exists(drupal_get_path('theme', $theme) .'/ga_pager/'. $my['pager_layout'] .'/previous.png') ? drupal_get_path('theme', $theme) .'/ga_pager/'. $my['pager_layout'] .'/previous.png' : drupal_get_path('module', 'gallery_assist') .'/ga_pager/default/previous.png';
     $my['grafic_previous'] = theme('image', $my['grafic_previous_path'], '', "go to the previous");
     $my['previous'] = variable_get('gallery_pager_simbol', 0) == 0 ? t('‹ previous') : $my['grafic_previous'];
   

   There is no need to use an array, in this case. Use plain PHP variables ($graphic_next, graphic_previous).

   You should use English words for variable names, array indexes, etc.

10. function gallery_assist_item_box(&$item, $conf) {
      $my = array();
      $my['title_height'] = $item->show_title == 1 && variable_get('gallery_image_size', 100) >= 100 ? 30 : 0;
      $my['item_width'] = variable_get('gallery_image_size', 100) + (variable_get('gallery_item_padding', 0) * 2) + (variable_get('gallery_item_border', 0) * 2);
      $my['item_height'] = variable_get('gallery_image_size', 100) + (variable_get('gallery_item_padding', 0) * 2) + (variable_get('gallery_item_border', 0) * 2) + $my['title_height'];
    

    An array variable is again used when there is no use for it; it would be better to use plain PHP variables.

11.   if (count($node->gallitems) <= 1) return;
    

    Follow the coding standards.

As I already said, there is already a module that has the same purpose; if it is found to be difficult to be used, that is not a reason to create a duplicate of an already existing module. The code should at least be better of the other module, to be accepted.

You need to re-apply for the CVS account, as the application has already been declined; changing the status of this report doesn't change the status of the application.