osmassoglia [osmassoglia]

The previous applications were #594588: osmassoglia [osmassoglia], and #579356: osmassoglia [osmassoglia].

1.   $statusCat = array('none'  => 'All', '0' => 'Unpublished', '1' => 'Published');
     $form['filter']['status'] = array(
       '#type'           => 'select',
       '#default_value'  => $_postp['status'],
       '#title'          => t('Status'),
       '#options'        => $statusCat,
       '#prefix'         => '<div style="float:left;padding:0px 10px 0px 0px;">',
       '#suffix'         => '</div>',
     );
   

   All the strings used in the user interface must be translatable; this includes the strings used for the options of a form field.

2.   if($_GET['page']){
       $_SESSION['SESSION_PAGE'] = $_GET['page'];
     }
     else{
       if(!$_GET['page'] && $_POST){
         $_GET['page'] = $_SESSION['SESSION_PAGE'];
       }
     }
     if($_POST){
       $_postp                 = $_POST;
       $_SESSION['POST_ADMIN'] = $_POST;
   
     }
     else{
       $_postp = $_SESSION['POST_ADMIN'];
   
     }
   

   Form functions don't take the default values from the variable $_POST.

3.     '#collapsed'  => FALSE,
   

   By default, the fieldsets are not collapsed; you can avoid to set that value, as it is the default value.

4.     '#title'        => t('Filter Info'),
   

   Strings used for the title, or the description should have the first word in capital case, while the others are in lower case (except when they are acronyms, or proper nouns).

5.     '#title'          => '<b>To ' . $statics['totalitems'] . ' articles </b>',
   

   The string is not translatable; if it would be, then it would be better to use placeholders.

6.   $keywordValue       = $_postp['keycol'] == "uid" ? $resultUser->uid : "'%".db_escape_string(strtoupper($_postp['keyword']))."%'" ;
   

   The code is supposed to use Drupal Unicode functions.

7.   $sqlcount =
         "
         SELECT count(*) as total
         FROM term_node AS t
         LEFT JOIN node AS n
         ON n.nid=t.nid
         WHERE t.tid $categoryOperator '$categoryValue'
         AND n.status $statusOperator '$statusValue'
         AND $keywordKey $keywordOperator $keywordValue
         ";
   

   Why isn't the code not using db_rewrite_sql()?

8.     $tiempo_inicio    =  nodemanager_microtime_float();
       $total            =  db_fetch_object(db_query($sqlcount));
   

   The name of the variables are normally in English.

9.     $statics['total']       = '<b style="color:#ff0000;">' . $total . '</b> in ' . $tiempo . ' Seconds';
   

   The string is not translatable. Normally, inline CSS styles are not used.

10.     while($result = db_fetch_object($res)){
          $title = $_postp['keycol'] == 'title' ? preg_replace("/({$_postp['keyword']})/i", '<b style="color:#FF0000;">\\1</b>', $result->title):$result->title;
          if($result->status == 1){
            $status = '<img src="/' . $pathImage . 'publish.png">';
          }
          else{
            $status = '<img src="/' . $pathImage . 'unpublish.png">';
          }
    

    The code is not formatted as reported in the coding standards.

11.       $editnode = '<a href="/node/' . $result->nid . '/edit?destination=admin/content/nodemanager">edit</a>';
          $title = '<a href="/node/' . $result->nid . '/view?destination=admin/settings/feeddelivery/admincontent">' . $title . '</a>';
    

    The code could use l().

12.       $form['options'][$counter]['check']     = array('#type' => 'checkbox','#value' => 0,);
    

    What is the purpose to show a form field, if then the user cannot change the form field value?

13.   foreach($form as $key => $listItem){
        if(is_numeric($key)){
          $rows[]  = array  (
            drupal_render($listItem['check']).drupal_render($listItem['id']),
            $listItem['title']['#value'],
            $listItem['author']['#value'],
            $listItem['created']['#value'],
            $listItem['status']['#value'],
            $listItem['type']['#value'],
            $listItem['taxName']['#value'],
            $listItem['editnode']['#value'],
          );
        }
      }
    

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

14.         drupal_set_message('Deleted node '.$obj['nid']);
    

    There are two errors in that code line. Which ones are they?

15.     switch($action){
          case 'delete':
            node_delete($obj['nid']);
            drupal_set_message('Deleted node '.$obj['nid']);
            break;
          case 'publish':
            $nodeL          = node_load($obj['nid']);
            $nodeL->status  = 1;
            node_save($nodeL);
            break;
          case 'unpublish':
            $nodeL          = node_load($obj['nid']);
            $nodeL->status  = 0;
            node_save($nodeL);
            break;
    

    The code is duplicating something that is already done by Drupal core code. Is that a reason to do it?

IMO, it seems the module is doing something that is already done by Drupal core code. What would the reason to prefer this module be?

I hope that osmassoglia is going to reply to this report; until now he opened a new CVS application for three times because I closed the previous after 14 days without a reply from his side.

  $statusCat = array( 'none'  => t('All'), '0' => t('Unpublished'), '1' => t('Published') );
  $form['filter']['status'] = array(
    '#type'           => 'select',
    '#default_value'  => $_postp['status'],
    '#title'          => t('Status'),
    '#options'        => $statusCat,
    '#prefix'         => '<div style="float:left;padding:0px 10px 0px 0px;">',
    '#suffix'         => '</div>',
  );
  $form['filter']['categories'] = array(
    '#type'           => 'select',
    '#default_value'  => $_postp['categories'],
    '#title'          => t('Categories'),
    '#options'        => $taxCateg,
    '#prefix'         => '<div style="float:left;padding:0px 10px 0px 0px;">',
    '#suffix'         => '</div>',
  );

The default value for a form field is not taken from the $_POST variable; Drupal code, and modules use a different approach.

when you say, "different aproach" you mean set_variable and get_variable?

Yes, I mean that. variable_get() should be used in that case. variable_set() is not necessary, if you use system_settings_form().

There are many reasons to not use values from the variable $_POST; the fact Drupal uses values from Drupal variables while populating the settings form fields should be a reason to do the same.

  if($_POST){
    variable_set('nodemanager_status', $_POST['status']);
    variable_set('nodemanager_categories', $_POST['categories']);
    variable_set('nodemanager_filter', $_POST['keycol']);
    variable_set('nodemanager_keyword', $_POST['keyword']);
  }
  if(!$_SESSION['LAST_ADMIN_SQL']){
//  	drupal_set_message("UNSET DATA");
  	variable_set('nodemanager_status', 'none');
    variable_set('nodemanager_categories', '');
    variable_set('nodemanager_filter', 'title');
    variable_set('nodemanager_keyword', '');
  }
  
  
  $tax = db_query('SELECT t.name,t.tid FROM term_data AS t ORDER BY t.name ASC');
  $taxCateg[0] = 'All';

1. The code is still using the variable $_POST, rather than using the content of Drupal variables.

   What I meant before is that the code should use the content of Drupal variables by calling variable_get(), not set the content of Drupal variables with values taken from $_POST.

2. See the Drupal coding standards to understand how a module code should be written. In particular see the part about the namespace respect, which is also valid for session variables.
3. The code is not using db_rewrite_sql() in the query for the taxonomy terms.

As a side note: I don't reply to emails messages about CVS applications because I am not the only person who approves CVS accounts; the review of code is then free to all Drupal.org users.

Why cannot set a variables with $_POST values?

Because you are saving in the database volatile values.

It seems that you didn't understand how a module should use the form API.
You will not see any module that in the settings page takes the values directly from $_POST, because that would mean to by-pass all the form API, and to allow the code to accept forged modules.

I still think that this module duplicates functionalities already present in Drupal, which already has code to manage nodes.

Please don't play with the status, fixed is quit different from the status I set.