The Mapply module adds a shortcode to Drupal that allows a Mapply locator map to be added to any page. The admin screen also includes a form to manage a Google Map API key as well as your Mapply API key. This module makes the already simple Mapply integration even easier by not requiring any knowledge of Javascript to add to a website.

This module depends on the Shortcode module.

Mapply (http://mapply.net) is a service that allows users to setup store locations and embed a map of those locations on their website. Simple JavaScript code is provided to install onto any website. Mapply has free registration and a 30 day free trial.

Git clone command: git clone --branch 7.x-1.x-dev http://git.drupal.org/sandbox/tysonkroeker/2291965.git mapply

Project Link: https://www.drupal.org/sandbox/tysonkroeker/2291965

Comments

gwprod’s picture

Status:Active» Needs review

This should initially be set as Needs review

er.pushpinderrana’s picture

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

URL: http://pareview.sh/

PA robot’s picture

Status:Needs review» Needs work

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.

tysonkroeker’s picture

Thank you for the prompt responses. After some work the online tool is no longer reporting any errors.

tysonkroeker’s picture

Status:Needs work» Needs review
Daniel.Moberly’s picture

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');
}
Rostislav Sergeenkov’s picture

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

<?php
 
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

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

You can use

<?php
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()"

<?php
'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

<?php
  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

MattWithoos’s picture

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.

MattWithoos’s picture

Status:Needs review» Needs work
tysonkroeker’s picture

Issue summary:View changes

Added link to the project

tysonkroeker’s picture

Status:Needs work» Needs review

First off, thank you @rostislav-sergeenkov and everyone else, for your detailed comments. They were just what I needed to understand a better way of doing what I was trying to do.

My code spacing habits (for a previous employer) have come back to bite me here. I've got those spacing issues corrected now.

pareview results
http://pareview.sh/pareview/httpgitdrupalorgsandboxtysonkroeker2291965gi...
0 Errors and a few warnings on line length in the README.md file.

MattWithoos’s picture

@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.

tysonkroeker’s picture

darol100’s picture

Status:Needs review» Needs work

@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.

<?php
/**
 * 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.

PA robot’s picture

Status:Needs work» Closed (won't fix)

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.