Reach Us

welcome,

Please read about the Project Application Workflow.

You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

Also remove the license file, when you create a release it'll be added automatically and all code hosted on d.o is under GPL anyway.

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow tips for a great project page. Make sure your README.txt follows the guidelines for in-project documentation.

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxquardz1513112git

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

Can you describe a little what the difference is between your module and the module you mentioned (http://drupal.org/project/on_the_web), please?...currently I can't see any difference.

His module provides widgets for instant messengers (skype, msn.. ) - on the web provides widgets for social communities (g+, facebook)

I think that's the difference

Ok then...I've made a little digging in the code, looks ok, just few things got my eyes picked on:
-reach_us.module : line 130 , 171 and 217...

otherwise looks ok...hooks used normally, functions are ok, nothing critical!
just a TIP, but it's fine anyway: u can store more things in one variable, you don't need one for each

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

This doesn't seem to have changed since it was moved to "needs work" status.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Getting 404 on demo link.

Initial Code review:

Info file has typo for dependency

name = Reach Us
description = Block provides easy way to reach you via multiple mobile and desktop messaging and communication platform.
core = 7.x

dependancies[] = block

Module file:

No need to pass $delta to block function. In-fact whole function can be removed by calling reach_us_output function directly inside hook_block_view.

/**
 * Implements hook_block_view().
 */
function reach_us_block_view($delta = 0) {
  $block = array(
    'subject' => t('Reach Us On'),
    'content' => reach_us_display_block($delta),
  );
  return $block;
}

/**
 * Block Display.
 *
 * @return string
 *    Content for block.
 */
function reach_us_display_block($delta) {
  // Get the appropriate name to use in links.
  $linktype = variable_get('reach_us_display', 'icon');
  return reach_us_output($linktype);
}

Also, can you please check for XSS attacks?

I tried adding script tag to all the fields, though it saved but it also appeared on block also. All HTML tags should be handled properly and striped at time of saving only.

Also, did you tried using system_settings_form instead of saving all variables manually.
You can have a look at the example at:

https://www.drupal.org/node/1111260

One other small feedback:

Inside reach_us_display_block function, you are retrieving $linktype. This variable will always have some value as you are taking default value also.

But again while passing this value to reach_us_output($linktype = 'icon') function, you are assuming some default value which is kind of useless.

/**
 * Block Display.
 *
 * @return string
 *    Content for block.
 */
function reach_us_display_block($delta) {
  // Get the appropriate name to use in links.
  $linktype = variable_get('reach_us_display', 'icon');
  return reach_us_output($linktype);
}
/**
 * Outputs the rendered HTML of list of contact services.
 *
 * @linktype
 * 	  get either clickable to be"icon" or "link".
 *
 * @return string as HTML
 *    Content for block.
 */
function reach_us_output($linktype = 'icon') {
  // Attach the CSS file.
  drupal_add_css(drupal_get_path('module', 'reach_us') . '/reachus.css');
  $visibility = 'reach-visibility-' . variable_get('reach_us_visibility', 'horizontal');
  $output = '<div class = "reachus-wrap reachus-' . $linktype . ' ' . $visibility . '">';
  $defaultvalue = variable_get('reach_us_services');
  *
  *
  *
  *
}

Hi quardz,
This is a great module ,please have a look on the below mentioned points
1. After saving the configuration form there is no message for form saving(you can see system form setting message) .
2. Please add validation. there should be some validation in the form.
3. There should be configuration for changing the icons so that user can upload images.
4. There should be configuration for adding more services as well.
5. Block should not appear as a blank content on the page if there is no service selected.
6. I will recommend you to use drupal system form settings.

Hi @quardz,
Thanks for the contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder.
http://pareview.sh/pareview/httpgitdrupalorgsandboxquardz1513112git

There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732

Review of the 7.x-1.x branch (commit bb2e5dc):

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Does not cause module duplication and fragmentation.

Master Branch

No: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: Update the Read me file of the module.Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes/No. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*)reach_us.module : reach_us_menu is not registereing the menu. Declare $items as array before registering a menu.Add this $items = array();after this linefunction reach_us_menu() { will fix this.
2. Remove the extra lines excluding line endings from reach_us.info
3. reach_us.inc : Move the function reach_us_services() into reach_us.module file because it is including in the file and used in the module file and there is no extra function in the .inc file so remove that file.We keep it seperate so that the extra code will not compiled on the page load and we manage the code better.But in this module we are keeping only single function in the inc file and calling it in every file(.install and .module)
4. Make a seperate file reach_us.admin.inc and move the admin page callback function reach_us_form and its submit function reach_us_form_submit defination into it.Because these functions are only needed at the admin callback page.After that add the file parameter in the hook_menu 'file' => 'reach_us.admin.inc'
5. function reach_us_output($linktype) : Use the template to print the html by passing the variables so that it can be overrided in theme.

This review uses the Project Application Review Template.

As I am not a git administrator, so I would recommend you, should fix above issues first and then review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Project page https://www.drupal.org/sandbox/quardz/1513112
Update the project page as read me and move the link 'more documentation here' into the documentation textbox by editing the project page so that it will display in the right sidebar and also then similarly move the 'Demo here' link in the 'Demo' text field under Resources section.Also update the installation,configuration text as readme file.

Thanks Again!

Automated Review

No issues found .

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Does not cause module duplication and fragmentation.

Master Branch

No: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: Read me file needs changes as per the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Review of the 7.x-1.x branch:

Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

sites/all/modules/reach_us/reach_us.module
reach_us.module

    severity: normalreview: style_string_spacingLine 160: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]

        $varid = 'reach_us_serv_' . $key  . '_id';

sites/all/modules/reach_us/reach_us.admin.inc
reach_us.admin.inc

    severity: criticalreview: security_form_set_errorLine 164: Potential problem: form_set_error() and form_error() only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_form_set_error]

            form_set_error($varid, $idvalid);

    severity: criticalreview: security_form_set_errorLine 169: Potential problem: form_set_error() and form_error() only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_form_set_error]

              form_set_error($varlinktext, $errormsg);

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No .

Master Branch

Yes

Licensing

Yes/dd>

3rd party code

No.

README.txt/README.md

Yes.

Code long/complex enough for review

Yes.

Secure code

Yes.

Coding style & Drupal API usage

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

quarz is not approved yet, so this is not fixed? See the workflow: https://www.drupal.org/node/532400

I think this module works correctly.

Manual Review

3rd party assets/code

* No: Does not follow the guidelines for 3rd party assets/code. The images that are included are copyrighted.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Automated review

FILE: ...sites/all/modules/reach_us/reach_us_content.tpl.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
--------------------------------------------------------------------------------

FILE: ...sites/all/modules/reach_us/reach_us.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
152 | ERROR | Return comment indentation must be 2 additional spaces
194 | ERROR | Return comment must be on the next line
--------------------------------------------------------------------------------

Manual Review

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

A couple things I missed on my first pass, or that arose after your updates.

1. admin.inc:
   

   else {
           if (!trim($form_state['values'][$varlinktext])) {
             $errormsg = filter_xss('Fill the Link text for ' . $services[$key]['label']);
             form_set_error($varlinktext, $errormsg);
           }
         }

   This should use t() on the "Fill..." and use argument replacement instead of filter_xss.

2. admin.inc: It isn't important, but it is typical to end the page callback like this:
   

     $form['#theme'] = 'reach_us_form';
     return system_settings_form($form);
   

3. .install: I think the module_load_include can be added to the hook_uninstall.
4. .module: Please don't use markup in calls to t(). If you want to emphasize something, use %variable. If you need markup, then it should be inserted as argument replacements or concatenated to the string. If you don't, it makes l10n/i18n harder.
   $output .= t('By enable the <strong>Reach us</strong> block in admin page.');

However, none of these are release blockers. So I'm marking as RTBC. Assigning to mpdonadio to give this a final review, if he has time.

Automated Review

Review of the 7.x-1.x branch (commit b6e94a4):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/matt/PAR/pareview_temp/reach_us.module
  --------------------------------------------------------------------------------
  FOUND 4 ERRORS AFFECTING 4 LINES
  --------------------------------------------------------------------------------
   142 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
   145 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
   148 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
   192 | ERROR | [x] Spaces must be used to indent lines; tabs are not allowed
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  
  Time: 678ms; Memory: 8.25Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) In _reach_us_content(), variable_get($varlinktext); is coming from user input and needs to be sanitized. It also looks the same for $display, $visibility, $iconsize.

Coding style & Drupal API usage

ALl of your variable_get() should have default values.

In reach_us_form(), why the filter_xss() on the description? This is hardcoded in the reach_us_services. Ah, because it is hookable.

(*) Since the services are hookable, they may be coming from user input, which means that you need to sanitize the label in reach_us_form() with check_plain. It is not directly exploitable, otherwise this would be a security issue.

(+) reach_us_form_validate is filtering values on input. This is not the Drupal way; you filter on output. See https://www.drupal.org/node/28984 and https://www.drupal.org/node/263002 It looks like you nust need to use t() with placeholders.

(+) module_load_include() should not be used the top level in the install file. See https://api.drupal.org/api/drupal/includes!module.inc/function/module_lo...

The print example at the bottom of the .api.php file is weird. Normally, you would just render array w/ a theme function.

Drupal has valid_email_address; no need for the PHP versions.

In reach_us_block_view, build up a render array instead of calling theme() directly.

#attached on the render array is preferred over drupal_add_css() for adding CSS.

reach_us_output() should really return a render array.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

There are a few examples where the render array was converted to #markup render array. It could be done better; the render arrays should be formatted:

$render = array(
    '#theme' => 'reach_us_theme',
    '#example_variable' => $example_variable,
    '#prefix' => $prefix,
    '#suffix' => $suffix,
  );

That's not a release blocker...

Thanks for your contribution, quardz!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.