duozersk [duozersk]

  $form['types'] = array(
    '#type'        => 'fieldset',
    '#title'       => t('Enabled content types'),
    '#description' => t('Check the content types you want the feedback form to appear on.'),
    '#collapsible' => TRUE,
    '#collapsed'   => TRUE,
  );

That options would be better placed in the content type edit form, inside the fieldset Workflow.

function nodefeedback_settings_submit($form, &$form_state) {
  variable_set('nodefeedback:enabled_types', $form_state['values']['enabled'], array());
  variable_set('nodefeedback:form_title', $form_state['values']['form_title'], t('Provide feedback on this information'));
  variable_set('nodefeedback:confirmation_message', $form_state['values']['confirmation_message'], t('Thank you! Your feedback is used to help us improve our content.'));
  variable_set('nodefeedback:button_text', $form_state['values']['button_text'], t('Send'));
}

Check the argument accepted by variable_set().

function nodefeedback_nodeapi(&$node, $op, $teaser, $page) {
  switch ($op) {
    case 'view':
      $exclude_modes = array(
        NODE_BUILD_PREVIEW,
        NODE_BUILD_SEARCH_INDEX,
        NODE_BUILD_SEARCH_RESULT,
        NODE_BUILD_RSS,
      );
      $enabled_types = variable_get('nodefeedback:enabled_types', array());
      if (!$teaser && !in_array($node->build_mode, $exclude_modes) && $enabled_types[$node->type] && user_access('provide node feedback')) {
        $node->content['nodefeedback'] = array(
          '#value'  => drupal_get_form('nodefeedback_form', $node->nid),
          '#weight' => 50,
        );
      }
      break;
  }
}

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

      $results[substr($value['function'], -1)] = $value['value'];

The code should use the Drupal Unicode functions.

  // removes all variables that start with "nodefeedback:"
  $result = db_query("SELECT name FROM {variable} WHERE name LIKE 'nodefeedback:%%'");
  while ($row = db_fetch_object($result)) {
    variable_del($row->name);
  }
}

As the code is using variable_de(), then it would be better to explicitly delete the Drupal variables used.

  $schema['nodefeedback'] = array(
    'fields' => array(
      'nodefeedback_id' => array('type' => 'serial', 'unsigned' => TRUE, 'not null' => TRUE),
      'content_type' => array('type' => 'varchar', 'length' => 64, 'not null' => TRUE, 'default' => 'node'),
      'content_id' => array('type' => 'int', 'unsigned' => TRUE, 'not null' => TRUE, 'default' => 0),
      'value' => array('type' => 'text', 'size' => 'big', 'not null' => TRUE, 'default' => ''),
      'tag' => array('type' => 'varchar', 'length' => 64, 'not null' => TRUE, 'default' => 'vote'),
      'uid' => array('type' => 'int', 'unsigned' => TRUE, 'not null' => TRUE, 'default' => 0),
      'timestamp' => array('type' => 'int', 'unsigned' => TRUE, 'not null' => TRUE, 'default' => 0),
      'vote_source' => array('type' => 'varchar', 'length' => 255),
    ),
    'primary key' => array('nodefeedback_id'),
    'indexes' => array(
      'content_uid' => array('content_type', 'content_id', 'uid'),
      'content_uid_2' => array('content_type', 'uid'),
      'content_source' => array('content_type', 'content_id', 'vote_source'),
    ),
  );

It would be better if the code would be formatted differently, and it would add a description for each field.

Plus this Workflow settings are really about the Node Edit form

That is not exact.
The fieldset Workflow settings is used when a module doesn't have enough options to justify a new fieldset. In the content type form there are all the settings that depends from the content type; it is where you find the settings for the content type comments, which include the setting for how the comments are displayed in the node view, which doesn't interest the node edit form.

Probably yes, however, I didn't really get the reasoning for this...
I used this approach to clean up all the variables that follow the module variables naming convention so if I add new variable in a module - I don't need to modify the uninstall function. I found this approach in the LuceneAPI module uninstall function.
Can you please advise on the reasoning and if it is critical to get it fixed to contribute this module?

The code should be deleting its own Drupal variables, not the ones used by other modules. As you checked what another module did, and you did the same, somebody could create a module that extends the module you created, and use variables with names starting with nodefeedback: simply because you used the same schema.

Probably, but this formatting change makes the schema description really long. Formatted it and added fields descriptions.
I got this formatting from the VotingAPI module and thought that it is fine to keep with it.

The database schema as it is now it is not readable; the coding standards report how the code should be formatted to make it more readable. If we would only be worried about the code length, why would the coding standards reports to write if ($test) {? That means to add at least 2 characters more for each if statement. Why aren't we allowed to write if($test){? We would save 2 characters for each if statement, and we would save a lot of characters if we would not indent each code line; if then we write all the code inline, we would have a shorter code.

1. /**
    * Submit handler for the nodefeedback settings form.
    */
   function nodefeedback_settings_submit($form, &$form_state) {
     variable_set('nodefeedback:enabled_types', $form_state['values']['enabled']);
     variable_set('nodefeedback:form_title', $form_state['values']['form_title']);
     variable_set('nodefeedback:confirmation_message', $form_state['values']['confirmation_message']);
     variable_set('nodefeedback:button_text', $form_state['values']['button_text']);
   }
   

   That code should not be required if the module would use system_settings_form(), and would use the proper names for the form fields.

2. The Drupal variable name doesn't follow the schema normally used, which should be <module_name>_<variable_name>.

3. /**
    * Implementation of hook_form_alter().
    */
   function nodefeedback_form_node_type_form_alter(&$form, &$form_state) {
     $enabled_types = variable_get('nodefeedback:enabled_types', array());
     $form['workflow']['nodefeedback'] = array(
       '#type'          => 'checkboxes',
       '#title'         => t('Node Feedback form settings'),
       '#description'   => t('Enable the Node Feedback form for this content type. For other settings check the <a href="@nodefeedback-settings-page">Node Feedback settings page</a>.', array('@nodefeedback-settings-page' => url('admin/settings/nodefeedback'))),
       '#options'       => array($form['#node_type']->type => t('Enable Node Feedback form')),
       '#default_value' => array($form['#node_type']->type => $enabled_types[$form['#node_type']->type]),
       '#access'        => user_access('administer site configuration'),
     );
     $form['#submit'][] = 'nodefeedback_node_type_form_submit';
   }
   

   The code duplicates a setting already present in the module settings page; the code is then not correct because, if you would visit the content type form of two different content types, it would allow to see node feedback form fields only in the last content type.

   The code should be similar to this one, to be correct.

     if (isset($form['#node_type'])) {
   
       // …
   
       $form['nodewords']['nodewords_edit_metatags'] = array(
         '#type' => 'checkbox',
         '#title' => t('Allow editing of meta tags'),
         '#description' => t('If selected, the node edit form will allow the users with the right permissions to edit the meta tags associated with nodes of this content type.'),
         '#default_value' => variable_get('nodewords_edit_metatags_' . $form['#node_type']->type, TRUE),
       );
   
       // …
     }
   

   See the name of the Drupal variable used.

function nodefeedback_node_type($op, $info) {
  if ($op == 'delete') {
    variable_del('nodefeedback_' . $info->type);
  }
}

The code doesn't need to do anything when a content type is renamed, because that is done automatically by Drupal, if you use nodefeedback_form_node_type_form_alter(). The settings page you created would lose the setting for a content type if the content type would be renamed; that is a reason more to use nodefeedback_form_node_type_form_alter() to handle settings that depend from a content type.
If the settings that depend on the content type are already added by nodefeedback_form_node_type_form_alter(), then they should be removed in other settings pages.

If you need the code to delete the Drupal variables used for a content type, then use the following one, which should be placed in nodefeedback_uninstall():

  foreach (array_keys(node_get_types('names')) as $node_type) {
    variable_del('nodefeedback_' . $node_type);
  }

The one big thing pointing out is you're allowing PHP input with the 'Node Feedback questions array config' field, even though you've added warnings. The security would most likely recommend (since we just dealt with this same problem with Views and Node Import) is new permissions, 'administer node feedback' as the base access for the admin/settings/nodefeedback callback, and 'use PHP for node feedback options' to make it *explicitly* clear that PHP could be used to own your site.

Other than that, I don't see anything else holding this up!

Also wondering if I entered in JavaScript it looks like it wouldn't be filtered out. Kinda a moot point since you've added the special PHP permission, but the security team would probably still like to see use of filter_xss_admin or filter_xss.

Something like:

  0 => array(
    'tag'            => 'resolution',
    'title'          => '<script>alert('XSS!');</script>,
    'type'           => 'radios',
    'description'    => '',
    'required'       => FALSE,
    'storage'        => 'votingapi',
    'answer_options' => array(
      0 => "Yes",
      1 => "No",
      2 => "I don't know",
    ),
   ),

The extra stuff would be put on display of the form from the auto-PHP-array. It's this part that's the problem:

function nodefeedback_form(&$form_state, $node_type, $nid) {
  ...
  foreach ($nodefeedback_questions as $key => $question) {
    $form['nodefeedback'][$question['tag']] = array(
      '#title'         => $question['title'], <-- XSS potential here
      '#type'          => $question['type'],
      '#description'   => $question['description'], <-- XSS potential here
      '#required'      => $question['required'],
    );
    if ($question['type'] == 'radios') {
      $form['nodefeedback'][$question['tag']]['#options'] = nodefeedback_answer_options_array($question, $node_type, $nid);  <-- XSS potential here (kinda)
      $form['nodefeedback'][$question['tag']]['#default_value'] = 'NULL'; //we need default value so it doesn't vote to the option "0" when the answer is not chosen
    }
    if ($question['type'] == 'textarea') {
      $form['nodefeedback'][$question['tag']]['#cols'] = 65;
      $form['nodefeedback'][$question['tag']]['#rows'] = 5;
      $form['nodefeedback'][$question['tag']]['#resizable'] = FALSE;
    }
  }

I agree, it is...

Perhaps this is the reason why many developers are unable to contribute to Drupal.org. While I agree that security and coding standards are important, CVS application issue queue is not the best way to deal with it. I can see many CVS applications in the queue waiting because of negligible reasons like 'provide more motivation' or 'there is an extra space in xyz.php file' etc.

Developers applying for CVS account are new at contributing to Drupal.org and may not be well acquainted with the coding practices followed at Drupal.org. There is no reason to expect that every module or theme being submitted to be flawless, nothing can be flawless.

Project tools like issue queue, version control and patches are for improving and maintaining modules and themes. Perhaps 'Project' is the best place to tackle these issues rather than holding up the CVS applications queue.

Coming from Wordpress development to Drupal.org, it feels like CVS application process is more of a hurdle than a helpful tool. I strongly believe the CVS application process must be simplified and barriers to entry must be lowered.

@Ravish: There is a place to discuss of how CVS applications should be make better; that place is not this queue, though.