[D7] Mapply

This should initially be set as Needs review

Please run PAreview.sh once, reporting multiple issues in your module.

URL: http://pareview.sh/

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxtysonkroeker2291965git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

It is still reporting some warnings, which you may want to check.

You should add cURL as a requirement to the module README, as not all servers have cURL installed. You may also consider a cURL check in the module itself, something like:

if (function_exists('curl_version')){
  ...
}
else {
  drupal_set_message('cURL needs to be installed on the server', 'error');
}

Hello @tysonkroeker and thank you for your work!

Pareview / Coder:

There are some issues with the code.
http://git.drupal.org/sandbox/tysonkroeker/2291965.git

Coder: No Problems Found

Manual review:

1. When I access /admin/mapply I see

Strict warning: Only variables should be passed by reference in include() (line 40 \sites\all\modules\contrib\mapply\settings.tpl.php).

To fix that error assign value of drupal_get_form('mapply_settings_form') to any variable, and then call in settings.tpl.php, str 40

print drupal_render($my_variable);

1. Why does the module create its own configuration page in the root level of Administration UI?

IMHO it's better to move it under existing Configuration section. It can be for instance admin/config/user-interface/mapply.

2. Page callbacks mapply_configuration() and mapply_settings() return string (Drupal 6 style) instead of renderable array (Drupal 7).

Drupal 7 uses the concept of "Renderable Arrays" - https://www.drupal.org/node/930760 . In this case the page remains alterable via hook_page_aletr(&$page).

Instead of

function mapply_configuration() {
  drupal_add_css(drupal_get_path('module', 'mapply') . '/css/mapply.css');
  return theme('mapply_configuration_template');
}

You can use

function mapply_configuration() {
  $page = array(
    'configuration' => array(
      '#theme' => 'mapply_configuration_template',
      '#attached' => array(
        'css' => array(
          drupal_get_path('module', 'mapply') . '/css/mapply.css',
        ),
      ),
    ),
  );

  return $page;
}	

You should also add 'variables' or 'render element' property in the hook_theme to prevent error "Notice: Undefined index: render element in theme()"

'mapply_settings_template' => array(
  'template' => 'settings',
  'variables' => array(),
),

The same relates to the declaration of mapply_settings() function.

3. I see that your module sets variables

  variable_set('mapply_api_key', $form_state['values']['mapply_api_key']);
  variable_set('mapply_google_api_key', $form_state['values']['mapply_google_api_key']);
  variable_set('mapply_display_powered_by', $form_state['values']['mapply_display_powered_by']);
  variable_set('mapply_powered_by', $data->powered_by);

What about cleaning them up in hook_uninstall of mapply.install file?

Code Style:

1. Check array formatting in mapply_menu() and in mapply_settings_form(). It should be one space separating each element, not multiple spaces.

Refer to https://www.drupal.org/coding-standards#array

To improve the number of responses and speed up the review process, I would recommend adding a link to the project page, obtaining a Review Bonus and adding a link to the automated PAReview.

@tysonkroeker, a few thoughts - one, you should really try to get the PAReview bonus. The Git admins don't even seem to be looking at PA's without one, at least not in priority.

Secondly, as the purpose of this is to demonstrate your understanding of Drupal coding standards, I would suggest applying appropriate linebreaks. However this won't be a blocker and I won't change the status.

Third - you mentioend old employer indent habits. Don't worry, this is just the Drupal community's way of indenting. It's not right or wrong, it's more about everyone doing the same thing. That's part of what is being checked for in these applications.

@tysonkroker,

1. In your .info file you have "configure = admin/mapply" and admin/mapply is not a page. You should change it to "admin/config/user-interface/mapply"
2. The configure button in the module page doesn't take you to the configure page it takes you to /admin. You should also change this so it can take you the configuration page.
3. When I enable to module i got this message on my terminal "Currently [error] using Mapply requires cURL does_not_exist" my understanding was that it need it Unix Curl not PHP cUrl.

   To make it more clear to the user you should change it to "Mapply requires Php cURL" instead of "Mapply requires cURL".

4. You should consider implementing hook_help even if the same information from README file.

And example of the Hook_help been read from READ ME file.

/**
 * Implements hook_help().
 */
function mapply_help($path, $arg) {
  switch ($path) {
    case 'admin/help#mapply':
      $output = file_get_contents(drupal_get_path('module', 'mapply') . '/README.txt');
      return module_exists('markdown') ? filter_xss_admin(module_invoke('markdown', 'filter', 'process', 0, -1, $output)) : '<pre>' . check_plain($output) . '</pre>';
  }
}

When I use to be a site builder hook_help was one of the place I used to look for help and also this a best practice for writing a module.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.