peem83 [peem83]

We review a project per applicant; please choose the one you want reviewed, and let us know it.

1. Minor but lots of tabs and no "two space" indenting
2. Functions solide_node_block_update_block(), solide_node_block_save_block() isn and get_nodes_for_block() t in your modules namespace.
3. $sql = db_query("SELECT nid, title FROM {node} WHERE type = '%s'", $node->type); You should wrap queries for nodes and taxonomy in db_rewrite_sql() to avoid access by-pass security issues.
4. Function node_block_display_block() returns HTML. Theme functions should render HTML. In fact a number of none theme functions return HTML.
5. $block['subject'] = $node_blocks[$delta]['title']; This looks like a potential XSS security issue. Assure me it isn't :)

1. The namespace issue has not been completely resolved.
2. In the definition of the database schema, it is perfectly useless to use empty descriptions, or empty indexes; if they are not used, then they should not declared.

3.     $items['admin/settings/node_block'] = array(
           'title' => t('Node block settings'),
           'page callback' => 'node_block_display_settings',
           'access callback' => 'user_access',
           'access arguments' => array('administer node block'),
       );
   

   Menu callback titles, and descriptions should not be passed to t() because that is already done by Drupal core code.

4. See the Drupal coding standards to understand how a module code should be written. In particular, see how the code should be formatted.

1.         'info' => $block_settings['title'].' '.t('for content type').' '.$block_settings['content_type'],
   

   Rather than concatenating the string, you should use t() placeholders.

2. Check how the code is formatted.
   • There should be a space after foreach, and before the parenthesis.
   • else if should be written elseif.
   • else, and elseif should be on a new line.
   • There should be a space before and after an operator; $var.='test' should be $var .= 'test'.

   •     $form['node_blocks'][$key]['style_'.$key] = array(
           '#type' => 'item',
           '#value' => $value['style']
         );
     

     In such cases, always add the comma also after the last array item.

   • Constants are always written in upper case; this is valid also for TRUE, FALSE, NULL.

3. function node_block_display_settings(){
     $output = '<div class="node_block">';
     $output .= drupal_get_form('node_block_display_block_form');
     $output .= '</div>';
     return $output;
   }
   
   function node_block_edit_block(){
     $output = '<div class="node_block">';
     $output .= drupal_get_form('node_block_edit_block_form');
     $output .= '</div>';
     return $output;
   }
   
   function node_block_set_block(){
     $output = '<div class="node_block">';
     $output .= drupal_get_form('node_block_set_block_form');
     $output .= '</div>';
     return $output;
   }
   

   Those functions can be replaced with a single function.

1.             $output .= theme_image($img_path, $content['filename'], $title = '', NULL, FALSE);
   

   A theme function is not called directly, but it is called with theme(<id>, <parameter>…); that is done because a theme function could be overwritten by a module, or by the currently set theme.

   From the first code line of theme() is evident that a call as theme('image') could not invoke theme_image(), but invoke any functions that replace it.

   function theme() {
     $args = func_get_args();
     $hook = array_shift($args);
   
     static $hooks = NULL;
     if (!isset($hooks)) {
       init_theme();
       $hooks = theme_get_registry();
     }
   
     if (is_array($hook)) {
       foreach ($hook as $candidate) {
         if (isset($hooks[$candidate])) {
           break;
         }
       }
       $hook = $candidate;
     }
   
     if (!isset($hooks[$hook])) {
       return;
     }
     // …
   }
   

   Optional arguments for which is used their default values should not be passed to the function.

2.       default:
         break;
   

   If the default action doesn't do anything, then it can be removed.

3. function node_block_settings_page() {
     $output = '<div class="node_block">';
     switch (arg(3)) {
       case 'blocks_settings':
         $output .= drupal_get_form('node_block_display_block_form');
         break;
       case 'set_block':
         $output .= drupal_get_form('node_block_set_block_form');
         break;
       case 'edit':
         $output .= drupal_get_form('node_block_edit_block_form');
         break;
       default:
         $output .= drupal_get_form('node_block_display_block_form');
         break;
     }
     $output .= '</div>';
     return $output;
   }
   

   If there are more settings pages, then the code should implement more than one menu callback, rather than using a single menu callback that shows different pages basing on the menu arguments.
   Also, if the code need to theme a settings page, it is enough it uses $form['#prefix'] = '<div class="node_block">', and $form['#prefix'] = '</div>'.

4. //-------------------------------------------------------------------- end node_block_display_block_form
   
   //—————————————————————————————————— node_block_edit_block_form
   

   Those comments are longer than 80 characters, and they are probably not seen. it would be better to use the normal comment style, as used from Drupal core code.
   Then, normally people read from left to right; the comments should be probably not noted even if they would be not longer than 80 characters.

5.   $nid = arg(1);
     $node = node_load($nid);
     $nodes = array();
     if ($node->type !== $node_block['content_type']) {
       foreach ($node_block['reference_fields'] as $field_name) {
         if (isset($node->$field_name)) {
           foreach ($node->$field_name as $f) {
             if (!empty($f['nid']))
               $nodes[] = node_load($f['nid']);
           }
         }
       }
     }
   

   I am not sure how the code can be sure arg(1) is a node ID.
   The code should better filter the node obtained from node_load(); showing the data of a node that has been unpublished, or to which the currently logged in user doesn't have access is considered a security issue. The code doesn't seem to check if the user has the permission to see the node, or any data associated with the node.

6. function node_block_update_block($region) {
     $theme = variable_get('theme_default','none');
     db_query("UPDATE {blocks}
               SET region = '%s'
               WHERE module = '%s'
               AND delta = %d
               AND theme = '%s'", $region, 'node_block', arg(4), $theme);
   }
   

   I am not sure that is the correct way to save the settings of a block. This is the first time I see such code being executed from a module that exposes blocks.

7.         $form['blocks_settings'][$i] = array(
             '#type' => 'fieldset',
             '#title' => t("Block $block_nr"),
             '#collapsible' => TRUE,
           );
   

   Use a placeholder. The first argument of t() must be a literal string; what you wrote is equivalent to t('Block ' . $block_nr).

8.         '#title' => t('The CSS clas for block content'),
   

   The CSS class for the block content.

9.     $form['previous'] = array(
         '#type' => 'submit',
         '#value' => '<< Previous'
       );
   

   Strings used in the user interface needs to be translated; this includes also the strings used as options for the form fields.

10.   if ($form_state['clicked_button']['#id'] == 'edit-previous') {
        $form_state['storage']['step']--;
      }
      elseif ($form_state['clicked_button']['#id'] == 'edit-next') {
        $form_state['storage']['step']++;
      }
      $form_state['rebuild'] = TRUE;
    

    When you set any values in $form_state['storage'], there is no need to set $form_state['rebuild'] because it is implicit.

11.   ($setValue === NULL)
    

    It is enough to use isset().

There have not been replies from the OP in the past 7 days. I am marking this report as won't fix.