ecofinn [ecofinn]

Hello, and thank you for applying for a CVS account.

Remember to attach the archive containing the code in a comment here.

1.

function kashflowapi_settings_form() {

  $form['username'] = array(
    '#title' => t('User name'),
    '#type' => 'textfield',
    '#default_value' => variable_get( 'kashflow_username', '' ),
  );
  $form['password'] = array(
    '#title' => t('Password'),
    '#type' => 'password',
    '#default_value' => '',
  );
  $form['submit'] = array(
    '#value' => t('Submit'),
    '#type' => 'submit',
  );

  return $form;
}

/**
 * Form submission handler for for kashflowapi_settings_form()
 *
 * @see kashflowapi_settings_form()
 * @see kashflowapi_settings_form_validate()
 */
function kashflowapi_settings_form_submit($form, &$form_state) {
  variable_set( 'kashflow_username', $form_state['values']['username'] );
  variable_set( 'kashflow_password', $form_state['values']['password'] );
  drupal_set_message( 'Changes to username and password have been stored.' );
}

can be done easier as this:

/*
 * Form builder for the Kashflow admin page at admin/settings/kashflowapi
 *
 * @ingroup forms
 */
function kashflowapi_settings_form() {

  $form['kashflow_username'] = array(
    '#title' => t('User name'),
    '#type' => 'textfield',
    '#default_value' => variable_get('kashflow_username', ''),
  );
  $form['kashflow_password'] = array(
    '#title' => t('Password'),
    '#type' => 'password',
    '#default_value' => '',
  );

  return system_settings_form($form);
}

/**
 * Form submission handler for for kashflowapi_settings_form()
 *
 * @see kashflowapi_settings_form()
 * @see kashflowapi_settings_form_validate()
 */
function kashflowapi_settings_form_submit($form, &$form_state) {
  drupal_set_message('Changes to username and password have been stored.');
}

you don't really need the submit function when using system_settings_form, but if you would like to set the message then keep it.

2. Skip the spaces after the "(" and before the ")" in funtion calls, if´s and function definitions. Use the coder module to search in your code for these. Here is some bad examples.

  variable_get( 'kashflow_username', '' )

  if ( ! isset( $result ) ) {

  function kashflowapi_latest_error( $error = '' ) {

Else it looks good, in that quick overview I did. Good comments, nice to see that.

Forgot one thing.

  drupal_set_message('Changes to username and password have been stored.');

The message should be sent in t() first, read more here, like this:

  drupal_set_message(t('Changes to username and password have been stored.'));

Drupal variable names should be prefixed with the module name; if the module name is kashflowapi.module, then the Drupal variable names should be prefixed by kashflowapi_.

• The points reported in this review are not in order of importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
2. The package line is used only when it contains a package name already used by other modules.
3. The module doesn't implement hook_uninstall(), or doesn't implement it to remove the Drupal variables it defines.
4. Remove any debugging code, including the commented out one.
5. Some comments present in the code make me think the code is not complete; one of the requirements is that the code is as much complete as possible.
6. Strings used in the user interface should be translated.

1. Should be in a kashflowapi.install file.

/**
 * Implementation of hook_uninstall().
 */
function kashflowapi_uninstall() {
  variable_del('kashflowapi_username');
  variable_del('kashflowapi_password');
}

2. Think the array key ("username" and "password") should be kashflowapi_*** if you using system_settings_form().

  $form['username'] = array(
    '#title' => t('User name'),
    '#type' => 'textfield',
    '#default_value' => variable_get('kashflowapi_username', ''),
  );
  $form['password'] = array(
    '#title' => t('Password'),
    '#type' => 'password',
    '#default_value' => '',
  );

3. Else you remove the validation or you make it. Don't have any todo when applying for CVS.

  /**
   * Do nothing - KashFlow implements it's own constraints on username and
   * password. We don't need to.
   * @todo Should we not still validate that our user has provided sensible
   * imput perhaps even a connection attempt to confirm that the credentials
   * were correct?
   */

subscribing

Subscribing

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

As per previous comment, I am setting this issue to won't fix.

Since new users can now create full projects, applications have a different purpose and they are handled on a different issue queue. See Apply for permission to opt into security advisory coverage for more information.