Hupso Social Share Buttons

Hi @penga

You need to check module duplication issue as there are many other module present in Drupal contirbution list which are giving the same feature.

http://ventral.org/pareview/httpgitdrupalorgsandboxpenga1806244git-7x-1x

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
5 | WARNING | Line exceeds 80 characters; contains 234 characters
13 | WARNING | Line exceeds 80 characters; contains 81 characters
--------------------------------------------------------------------------------

FILE: ...-7-pareview/sites/all/modules/pareview_temp/test_candidate/hupso.module
--------------------------------------------------------------------------------
FOUND 37 ERROR(S) AND 3 WARNING(S) AFFECTING 34 LINE(S)
--------------------------------------------------------------------------------
5 | ERROR | The last line in the file doc comment must be " */"
5 | ERROR | Expected 1 space(s) before asterisk; 0 found
13 | ERROR | Doc comment for var Which does not match actual variable name
| | $path at position 1
13 | ERROR | Parameter comment must be on the next line at position 1
15 | ERROR | Doc comment for var Array does not match actual variable name
| | $arg at position 2
15 | ERROR | Parameter comment must be on the next line at position 2
17 | ERROR | Expected 1 space(s) before asterisk; 0 found
24 | ERROR | Whitespace found at end of line
28 | ERROR | Expected 1 space(s) before asterisk; 0 found
31 | WARNING | Line exceeds 80 characters; contains 93 characters
31 | ERROR | No space before comment text; expected "// The name that will
| | appear in the block list." but found "//The name that will
| | appear in the block list."
31 | ERROR | Comments may not appear after statements.
32 | ERROR | No space before comment text; expected "// Default" but found
| | "//Default"
32 | ERROR | Comments may not appear after statements.
41 | ERROR | Expected 1 space(s) before asterisk; 0 found
53 | ERROR | Expected 1 space(s) before asterisk; 0 found
73 | ERROR | Expected 1 space(s) before asterisk; 0 found
77 | ERROR | Whitespace found at end of line
83 | ERROR | Expected 1 space after "=>"; 2 found
90 | ERROR | Whitespace found at end of line
100 | ERROR | Whitespace found at end of line
112 | WARNING | A comma should follow the last multiline array item. Found: ]
125 | ERROR | Whitespace found at end of line
146 | ERROR | Whitespace found at end of line
147 | ERROR | Whitespace found at end of line
153 | WARNING | Format should be "* Implements hook_foo()." or "Implements
| | hook_foo_BAR_ID_bar() for xyz_bar()."
154 | ERROR | Expected 1 space(s) before asterisk; 0 found
157 | ERROR | Whitespace found at end of line
160 | ERROR | Whitespace found at end of line
161 | ERROR | Whitespace found at end of line
165 | ERROR | Whitespace found at end of line
166 | ERROR | Whitespace found at end of line
170 | ERROR | Whitespace found at end of line
177 | ERROR | Data type of return value is missing
179 | ERROR | Expected 1 space(s) before asterisk; 0 found
191 | ERROR | If the line declaring an array spans longer than 80
| | characters, each element should be broken into its own line
194 | ERROR | Inline control structures are not allowed
200 | ERROR | Whitespace found at end of line
215 | ERROR | Whitespace found at end of line
216 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

Some other suggestions:

Line 155: hupso_node_view()
You're doing a strpos() to look for {hupso_hide} in order to hide the share buttons on specific nodes which is successfully hiding the buttons, but the {hupso_hide} string is still showing up in the content. Also, the proper way of doing this would be to add something like a checkbox on the node edit/create form rather than looking for a string in the content of the node.

Line 180: _hupso_get_button_code()
Rather than outputting a hard-coded string for the buttons please utilize hook_theme() and Drupals theme layer to render your button code otherwise it makes it very difficult to properly theme the buttons.

-Ian

Manual Review:
1. You really need to define your own admin rights, so that users can more fine-grained control.
2. .info file : Give your module a package name, now you haven't mention any package and it goes to other module list.
3. You need to change you implementation. In your admin setting form add setting for content type for which this Hupso Social Share Buttons should apply.
4. Line no 159 : $menu = menu_get_object(); variable is defined but not used anywhere.
5. Code :

if (strpos($text, '{hupso_hide}') === FALSE) {  
    $node->content['body']['0']['#markup'] = str_replace('{hupso_hide}', '', $node->content['body']['0']['#markup']);
    if (($view_mode != 'teaser') && node_is_page($node)) {
      $node->content['hupso']['hupso'] = array(
        '#markup' => _hupso_get_button_code(), 
        '#weight' => 10 , 
      );
    }
  }

is not a good idea to put your code directly after the body field. Try to put it node links like other social sharing modules do.

it will be great if you have explore "hook_content_extra_fields()". It seems that there not much documentation available for this hook but it is being user for drupal 7 module EVA: Entity Views Attachment (http://drupal.org/project/eva) and http://www.lullabot.com/articles/great-pretender-making-your-data-act-field.

hope it helps :)

@penga

Manual Code Review
I have found two problems in code.

1) You are setting many variables in module configuration page. But you didn't create any .install file for module. You need to create .install file for this module and delete those variable in hook_uninstall .

2) Why such code

$code = '<div style="padding-left: ' . $padding_left . 'px; padding-top: ' . $padding_top . 'px; padding-bottom: ' . $padding_bottom . 'px;">';
  $code .= '<!-- Hupso Share Buttons (http://www.hupso.com/share) --><a class="hupso_pop" href="http://www.hupso.com/share"><img src="' . $button_url . '" width="' . $width . '" height="' . $height . '" border="0" alt="Social Share Buttons"/></a><script type="text/javascript">';
  $code .= $hupso_services;
  $code .= '</script>';
  $code .= '<script type="text/javascript" src="http://static.hupso.com/share/js/share.js"></script>';
  $code .= '<!-- Hupso Share Buttons -->';
  $code .= '</div>';

You should implement hook_theme and separate html from module file. It will also help other people to alter the layout.

/**
* @file
* Ininstall and uninstall for Hupso Social Share Buttons Module.
*/

Spelling mistake; "Ininstall" should be "Install".

/**
 * Implements hook_install().
 */
function hupso_uninstall() {

This should read "Implements hook_uninstall()."

/**
 * Implements hook_help().
 *
 * Displays help and module information.
 *
 * @param string $path
 *   Which path of the site we're using to display help
 * @param string $arg
 *   Array that holds the current path as returned from arg() function
 */

Just writing Implements hook_help(). should be enough.

  $blocks['hupso_block_info'] = array(
    'info' => t('Hupso Social Share Buttons'),
    'cache' => DRUPAL_CACHE_PER_ROLE,
  );

You don't need to declare 'cache' => DRUPAL_CACHE_PER_ROLE, as it's the default setting.

  $items['admin/config/content/hupso'] = array(
    'title' => 'Hupso Social Share Buttons',
    'description' => 'Configuration for Hupso Social Share Buttons',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('hupso_form'),
    'access arguments' => array('access administration pages'),
    'type' => MENU_NORMAL_ITEM,
  );

You don't need to declare 'type' => MENU_NORMAL_ITEM, as it's the default setting. Also, the access argument should use the permission that you previously defined in hook_permission (administer hupso).

$image_path = $base_path . drupal_get_path('module', 'hupso') . '/buttons/';

I don't think you need $base_path here (similar code is found throughout the module and should also be updated).

Both

    '#options' => array(
      'article' => 'Article',
      'page' => 'Basic page',
    ),

and

    '#options' => array(
      'teasers' => 'Display in teasers',
    ),

should probably have the array values run through t() in order for them to be made translatable.

I'd suggest removing _hupso_get_button_code() in favor of providing a theme function and an associated template file which would allow the contents of the block to be more easily customized by themers.

I would also suggest building the block content as a render array and using #attached to add any required CSS or JS. There are a number of benefits to doing so including better caching and easier customization by other developers.

Also, I honestly don't see serving enough of a function (it only has one style rule to remove left padding from the buttons) and is highly theme dependent. I think you're better off just leaving any required styling to the site builder.

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 :-)

Would it be possible to post the latest files in a zip file? I don't know how to get hold of the files using GIT.