Like Button

It appears 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.
Review of the master branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  
  likebtn.module
  

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

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.

FILE: /var/www/drupal-7-pareview/pareview_temp/likebtn.admin.inc
--------------------------------------------------------------------------------
FOUND 50 ERROR(S) AND 1 WARNING(S) AFFECTING 29 LINE(S)
--------------------------------------------------------------------------------
  12 | ERROR   | Whitespace found at end of line
  18 | ERROR   | Whitespace found at end of line
  26 | ERROR   | Whitespace found at end of line
  32 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
  32 | WARNING | Last item of an inline array must not be followed by a comma
  32 | ERROR   | There should be no white space before a closing ")"
  34 | ERROR   | Whitespace found at end of line
  42 | ERROR   | Whitespace found at end of line
  48 | ERROR   | Whitespace found at end of line
  56 | ERROR   | Whitespace found at end of line
  63 | ERROR   | Whitespace found at end of line
  70 | ERROR   | Whitespace found at end of line
  77 | ERROR   | Whitespace found at end of line
  84 | ERROR   | Whitespace found at end of line
  91 | ERROR   | Whitespace found at end of line
  98 | ERROR   | Whitespace found at end of line
 105 | ERROR   | Whitespace found at end of line
 111 | ERROR   | If the line declaring an array spans longer than 80
     |         | characters, each element should be broken into its own line
 111 | ERROR   | Expected 1 space between ""auto"" and double arrow; 0 found
 111 | ERROR   | Expected 1 space between double arrow and ""auto""; 0 found
 111 | ERROR   | Expected 1 space before "=>"; 0 found
 111 | ERROR   | Expected 1 space after "=>"; 0 found
 111 | ERROR   | Expected 1 space between ""en"" and double arrow; 0 found
 111 | ERROR   | Expected 1 space between double arrow and ""en - English""; 0
     |         | found
 111 | ERROR   | Expected 1 space before "=>"; 0 found
 111 | ERROR   | Expected 1 space after "=>"; 0 found
 111 | ERROR   | Expected 1 space between ""ru"" and double arrow; 0 found
 111 | ERROR   | Expected 1 space between double arrow and ""ru -
     |         | Русский""; 0 found
 111 | ERROR   | Expected 1 space before "=>"; 0 found
 111 | ERROR   | Expected 1 space after "=>"; 0 found
 111 | ERROR   | Expected 1 space between ""de"" and double arrow; 0 found
 111 | ERROR   | Expected 1 space between double arrow and ""de - Deutsch""; 0
     |         | found
 111 | ERROR   | Expected 1 space before "=>"; 0 found
 111 | ERROR   | Expected 1 space after "=>"; 0 found
 111 | ERROR   | Expected 1 space between ""ja"" and double arrow; 0 found
 111 | ERROR   | Expected 1 space between double arrow and ""ja - 日本語"";
     |         | 0 found
 111 | ERROR   | Expected 1 space before "=>"; 0 found
 111 | ERROR   | Expected 1 space after "=>"; 0 found
 113 | ERROR   | Whitespace found at end of line
 120 | ERROR   | Whitespace found at end of line
 126 | ERROR   | Whitespace found at end of line
 133 | ERROR   | Whitespace found at end of line
 139 | ERROR   | Whitespace found at end of line
 145 | ERROR   | Whitespace found at end of line
 151 | ERROR   | Whitespace found at end of line
 157 | ERROR   | Whitespace found at end of line
 163 | ERROR   | Whitespace found at end of line
 169 | ERROR   | Whitespace found at end of line
 175 | ERROR   | Whitespace found at end of line
 181 | ERROR   | Whitespace found at end of line
 187 | ERROR   | Whitespace found at end of line
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/likebtn.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 4 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/likebtn.module
--------------------------------------------------------------------------------
FOUND 188 ERROR(S) AND 5 WARNING(S) AFFECTING 77 LINE(S)
--------------------------------------------------------------------------------
   1 | ERROR   | End of line character is invalid; expected "\n" but found
     |         | "\r\n"
  18 | ERROR   | Expected 1 space before "=>"; 0 found
  18 | ERROR   | Expected 1 space after "=>"; 0 found
  18 | ERROR   | Whitespace found at end of line
  19 | ERROR   | Expected 1 space before "=>"; 0 found
  19 | ERROR   | Expected 1 space after "=>"; 0 found
  20 | ERROR   | Expected 1 space before "=>"; 0 found
  20 | ERROR   | Expected 1 space after "=>"; 0 found
  20 | ERROR   | Whitespace found at end of line
  21 | ERROR   | Expected 1 space before "=>"; 0 found
  21 | ERROR   | Expected 1 space after "=>"; 0 found
  21 | ERROR   | Whitespace found at end of line
  22 | ERROR   | Expected 1 space before "=>"; 0 found
  22 | ERROR   | Expected 1 space after "=>"; 0 found
  22 | ERROR   | Whitespace found at end of line
  23 | ERROR   | Expected 1 space before "=>"; 0 found
  23 | ERROR   | Expected 1 space after "=>"; 0 found
  23 | ERROR   | Whitespace found at end of line
  24 | ERROR   | Expected 1 space before "=>"; 0 found
  24 | ERROR   | Expected 1 space after "=>"; 0 found
  24 | ERROR   | Whitespace found at end of line
  25 | ERROR   | Expected 1 space before "=>"; 0 found
  25 | ERROR   | Expected 1 space after "=>"; 0 found
  25 | ERROR   | Whitespace found at end of line
  26 | ERROR   | Expected 1 space before "=>"; 0 found
  26 | ERROR   | Expected 1 space after "=>"; 0 found
  26 | ERROR   | Whitespace found at end of line
  27 | ERROR   | Expected 1 space before "=>"; 0 found
  27 | ERROR   | Expected 1 space after "=>"; 0 found
  27 | ERROR   | Whitespace found at end of line
  28 | ERROR   | Expected 1 space before "=>"; 0 found
  28 | ERROR   | Expected 1 space after "=>"; 0 found
  28 | ERROR   | Whitespace found at end of line
  29 | ERROR   | Expected 1 space before "=>"; 0 found
  29 | ERROR   | Expected 1 space after "=>"; 0 found
  29 | ERROR   | Whitespace found at end of line
  30 | ERROR   | Expected 1 space before "=>"; 0 found
  30 | ERROR   | Expected 1 space after "=>"; 0 found
  30 | WARNING | A comma should follow the last multiline array item. Found:
     |         | "large"
  33 | ERROR   | Space before opening parenthesis of function call prohibited
  33 | ERROR   | Space before opening parenthesis of function call prohibited
  34 | ERROR   | There should be no white space after an opening "("
  34 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  34 | ERROR   | Expected 1 space between double arrow and ""en""; 0 found
  34 | ERROR   | Expected 1 space before "=>"; 0 found
  34 | ERROR   | Expected 1 space after "=>"; 0 found
  34 | ERROR   | There should be no white space before a closing ")"
  35 | ERROR   | There should be no white space after an opening "("
  35 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  35 | ERROR   | Expected 1 space between double arrow and "true"; 0 found
  35 | ERROR   | Expected 1 space before "=>"; 0 found
  35 | ERROR   | Expected 1 space after "=>"; 0 found
  35 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  35 | ERROR   | There should be no white space before a closing ")"
  36 | ERROR   | There should be no white space after an opening "("
  36 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  36 | ERROR   | Expected 1 space between double arrow and "true"; 0 found
  36 | ERROR   | Expected 1 space before "=>"; 0 found
  36 | ERROR   | Expected 1 space after "=>"; 0 found
  36 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  36 | ERROR   | There should be no white space before a closing ")"
  37 | ERROR   | There should be no white space after an opening "("
  37 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  37 | ERROR   | Expected 1 space between double arrow and "false"; 0 found
  37 | ERROR   | Expected 1 space before "=>"; 0 found
  37 | ERROR   | Expected 1 space after "=>"; 0 found
  37 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  37 | ERROR   | There should be no white space before a closing ")"
  38 | ERROR   | There should be no white space after an opening "("
  38 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  38 | ERROR   | Expected 1 space between double arrow and "false"; 0 found
  38 | ERROR   | Expected 1 space before "=>"; 0 found
  38 | ERROR   | Expected 1 space after "=>"; 0 found
  38 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  38 | ERROR   | There should be no white space before a closing ")"
  39 | ERROR   | There should be no white space after an opening "("
  39 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  39 | ERROR   | Expected 1 space between double arrow and "true"; 0 found
  39 | ERROR   | Expected 1 space before "=>"; 0 found
  39 | ERROR   | Expected 1 space after "=>"; 0 found
  39 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  39 | ERROR   | There should be no white space before a closing ")"
  40 | ERROR   | There should be no white space after an opening "("
  40 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  40 | ERROR   | Expected 1 space between double arrow and "false"; 0 found
  40 | ERROR   | Expected 1 space before "=>"; 0 found
  40 | ERROR   | Expected 1 space after "=>"; 0 found
  40 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  40 | ERROR   | There should be no white space before a closing ")"
  41 | ERROR   | There should be no white space after an opening "("
  41 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  41 | ERROR   | Expected 1 space between double arrow and "false"; 0 found
  41 | ERROR   | Expected 1 space before "=>"; 0 found
  41 | ERROR   | Expected 1 space after "=>"; 0 found
  41 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
  41 | ERROR   | There should be no white space before a closing ")"
  42 | ERROR   | There should be no white space after an opening "("
  42 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  42 | ERROR   | Expected 1 space between double arrow and "'white'"; 0 found
  42 | ERROR   | Expected 1 space before "=>"; 0 found
  42 | ERROR   | Expected 1 space after "=>"; 0 found
  42 | ERROR   | There should be no white space before a closing ")"
  43 | ERROR   | There should be no white space after an opening "("
  43 | ERROR   | There should be no white space before a closing ")"
  44 | ERROR   | There should be no white space after an opening "("
  44 | ERROR   | There should be no white space before a closing ")"
  45 | ERROR   | There should be no white space after an opening "("
  45 | ERROR   | Expected 1 space between ""default"" and double arrow; 0 found
  45 | ERROR   | Expected 1 space between double arrow and "true"; 0 found
  45 | ERROR   | Expected 1 space before "=>"; 0 found
  45 | ERROR   | Expected 1 space after "=>"; 0 found
  45 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
     |         | found "true"
  45 | ERROR   | There should be no white space before a closing ")"
  46 | ERROR   | There should be no white space after an opening "("
  46 | ERROR   | There should be no white space before a closing ")"
  47 | ERROR   | There should be no white space after an opening "("
  47 | ERROR   | There should be no white space before a closing ")"
  48 | ERROR   | There should be no white space after an opening "("
  48 | ERROR   | There should be no white space before a closing ")"
  49 | ERROR   | There should be no white space after an opening "("
  49 | ERROR   | There should be no white space before a closing ")"
  50 | ERROR   | There should be no white space after an opening "("
  50 | ERROR   | There should be no white space before a closing ")"
  51 | ERROR   | There should be no white space after an opening "("
  51 | ERROR   | There should be no white space before a closing ")"
  52 | ERROR   | There should be no white space after an opening "("
  52 | ERROR   | There should be no white space before a closing ")"
  53 | ERROR   | There should be no white space after an opening "("
  53 | ERROR   | There should be no white space before a closing ")"
  54 | ERROR   | There should be no white space after an opening "("
  54 | ERROR   | There should be no white space before a closing ")"
  54 | WARNING | A comma should follow the last multiline array item. Found: )
  56 | ERROR   | Whitespace found at end of line
  58 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
  68 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
  86 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
  92 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  93 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  94 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  95 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
  98 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 103 | ERROR   | Whitespace found at end of line
 107 | ERROR   | Line indented incorrectly; expected 4 spaces, found 6
 107 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
     |         | found "false"
 112 | ERROR   | Inline comments must start with a capital letter
 112 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
 118 | ERROR   | Line indented incorrectly; expected 4 spaces, found 6
 119 | ERROR   | else must start on a new line
 120 | ERROR   | Line indented incorrectly; expected 4 spaces, found 6
 122 | ERROR   | Whitespace found at end of line
 124 | ERROR   | Concat operator must be surrounded by spaces
 124 | ERROR   | Concat operator must be surrounded by spaces
 125 | ERROR   | No space before comment text; expected "// '#value' => 'FFF',"
     |         | but found "//'#value' => 'FFF',"
 131 | ERROR   | Function comment short description must end with a full stop
 134 | ERROR   | Whitespace found at end of line
 135 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 135 | ERROR   | Concat operator must be surrounded by spaces
 135 | ERROR   | Concat operator must be surrounded by spaces
 135 | ERROR   | Concat operator must be surrounded by spaces
 135 | ERROR   | Concat operator must be surrounded by spaces
 136 | ERROR   | Whitespace found at end of line
 137 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 138 | ERROR   | Whitespace found at end of line
 139 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 139 | ERROR   | Expected 1 space before "=>"; 0 found
 139 | ERROR   | Expected 1 space after "=>"; 0 found
 139 | ERROR   | Expected 1 space before "=>"; 0 found
 139 | ERROR   | Expected 1 space after "=>"; 0 found
 140 | ERROR   | Whitespace found at end of line
 141 | ERROR   | Line indented incorrectly; expected 4 spaces, found 8
 141 | ERROR   | Concat operator must be surrounded by spaces
 142 | ERROR   | Line indented incorrectly; expected 4 spaces, found 8
 142 | ERROR   | Inline comments must start with a capital letter
 142 | ERROR   | Inline comments must end in full-stops, exclamation marks, or
     |         | question marks
 143 | ERROR   | Line indented incorrectly; expected 4 spaces, found 8
 144 | ERROR   | Line indented incorrectly; expected 6 spaces, found 12
 145 | ERROR   | Line indented incorrectly; expected 6 spaces, found 12
 146 | ERROR   | Line indented incorrectly; expected 8 spaces, found 16
 147 | ERROR   | Line indented incorrectly; expected 10 spaces, found 20
 148 | ERROR   | Line indented incorrectly; expected 8 spaces, found 16
 148 | ERROR   | else must start on a new line
 149 | ERROR   | Line indented incorrectly; expected 10 spaces, found 20
 152 | ERROR   | Line indented incorrectly; expected 6 spaces, found 12
 152 | ERROR   | Concat operator must be surrounded by spaces
 152 | ERROR   | Concat operator must be surrounded by spaces
 152 | ERROR   | Concat operator must be surrounded by spaces
 152 | ERROR   | Concat operator must be surrounded by spaces
 155 | ERROR   | Whitespace found at end of line
 156 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 156 | ERROR   | Expected 1 space after "="; 0 found
 163 | ERROR   | Line indented incorrectly; expected 2 spaces, found 4
 164 | ERROR   | Whitespace found at end of line
 165 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

It's a good module.
I have installed it and tried it. I have a suggestion:

There is an option to define which content types would have this feature as well as comments. Nevertheless the feature for comments will apply to all comments for any content type. Maybe an administrator would not want this feature for one or more content types. It would be great if the module could give administrators the option to exclude that behavior for one or more content types.

I had forgot to mention that you'll need to add the *.install file to invoke hook_uninstall in order to delete persistent variables

Hi likebtn!

1. Pareview

No errors.

2. Project page

Please provide more details on the project page and add a Live Demo link. Generally, it is a good start if you can follow the D.O. "Tips for a great project page". At the moment project page is missing an Overview section.

3. Translation support

Please add translation support for strings with t() function. Examples on lines 70, 81 of likebtn.module and 35, 36, 37, 46, 47 (maybe more...) of likebtn.admin.inc

WITHOUT translation support:
return 'Provides a fully customizable like button widget (for more information visit <a href="http://www.likebtn.com">LikeBtn.com</a>)';

WITH translation support:
return t("Provides a fully customizable like button widget (for more information visit !url)", array("!url" => l("LikeBtn.com", "http://www.likebtn.com")));

IMPORTANT! Remove translation support for variable descriptions if they are used as parameters on the configuration. For example on line 68 of likebtn.admin.inc the 'shared_enabled' should not be translated since it is a parameter.

'#description' => t('share_enabled'),

4. Drupal API

The module uses constants to set some values and installs variables on the database (on variable table). These variables should be deleted when the module is unistalled. You have to create a likebtn.install file and add the hook_unistall() function. Example:

/**
 * Implements hook_uninstall().
 */
function likebtn_uninstall() {
  db_delete('variable')->condition('name', 'likebtn_%', 'LIKE')->execute();
  cache_clear_all('variables', 'cache_bootstrap');
}

Furthermore, I think it's not a good idea to use constants... You can set default values with the variable_get() function. Is there a reason for using contants?

5. Configuration menu

You need to make a fix for the hook_menu() implementation. The callback for the path admin/config/likebtn but it should be something like admin/config/services/likebtn. See Information Architecture page for more information.

6. Functionality

I get an error when I set any of the i18n_% value on configuration page. Also, the values are saved on the database but they are not visible on the inputs. Please check the functionality works well for any combination of values a user can set.

Notice: Undefined index: default in likebtn_get_markup() 
(line 139 of \sites\all\modules\like_button\likebtn.module)

Also, there is a checkbox to enable the button on comments but the comments module is not enabled. You can add an if statement here.

7. Coding standards

Try to keep the code well formatted all over the lines. On the likebtn.admin.inc the formatting is good until line 49. After this line tabs and indent are not justified.

8. The Drupal Way tips

You can add the http://www.likebtn.com/js/widget.js as external js file using drupal_add_js()

drupal_add_js("http://www.likebtn.com/js/widget.js", array('type' => 'external'));

The problem is that Drupal currently does not support the 'async' attribute of HTML5.

Postscript: Sorry if some things are already mentioned some hour before. I was writing the review before these were published!

It's getting better!

a) function likebtn_get_markup() on line 73 of .module
Replace the settings values with the user variables using variable_get(). Default values and tyoe are already set on the form on .inc. Example of new code:
"lang" => variable_get('likebtn_settings_lang', 'en'),

b) You can disable form $form['likebtn_comments_nodetypes'] on line 28 of .inc using the #disabled attribute when comment module is not enabled. To check the latest use the module_exists() function. Example:

$form['likebtn_comments_nodetypes'] = array(
    '#type'          => 'checkboxes',
    '#title'         => t('Enabled for comments to the following content types'),
    '#description'   => t('Select the content types for comments to which you want to activate like button. You must enable comment module for this option.'),
    '#default_value' => variable_get('likebtn_comments_nodetypes', array()),
    '#options'       => $options,
    '#disabled'      => !module_exists('comment'),
  );

c) Change README.txt according to the new project page. Try not to copy paste from HTML to txt and beautify the formatting.

d) Make sure this is done for lines 147+ of .inc

e) You are using i18n_% in the parameters as also as in their description. This may cause a problem with the i18n module if users try to translate the values with this module. At the moment the module supports only one language for display (one of EN, RU, DE, JA) but the user cannot use the module on a multilingual site. See if you can make variables translatable.

f) It would be better if you could split the form settings on the .inc with collapsed #fieldset. Also, make each pair of values show_%_label - i18n_% display on queue and disable the i18n_% input if the show_%_label checkbox is not checked. The same for the PRO, VIP and PLUS options.
These changes will make the UI more easy to understand and control (and lead this module to more success!)

g) It would be super useful if you could create the Like Button as field. See for example the link module and the field.api.php documentation. But this can be a support request later.

Sorry for d) Make sure this is done for lines 147+ of .inc I forgot to write it.
It is about Justified tabs and indents. Make sure they are justified well (this is minor of course but it makes coding more easy to understand).

Will check other changes ASAP.

Back again!

1) Project page

- You can totally remove the 'Installation' section. This module has a normal installation. Otherwise just leave only the configuration (No 3). Other data are obvious and add no useful information. Old maintainers used a link to Installing contributed modules and themes but I think it's useless too.

2) likebtn.install

- Remove word 'core' from comment on line 5
- Also, #332123: Remove t() from all schema descriptions

3) likebtn.module

- Remove word 'core' from comment on line 5
- Line 15: return module_exists('markdown') ... Is this ('markdown') a typo error or you want to add markdown support for help display?
- Lines 244 - 291 ($likebtn_settings_js): This code works but is a good method to hide/enable elements. I was talking about hidding input elements using the common form methods. You can see examples in most theme-settings.php of most themes (Zen included). For example, they hide breadcrumbs settings if user selects no custom breadcrumbs.
- About the default variable values: Why do you need to know if the user has the default values? The variable_get('VARIABLE_NAME', VALUE) is enough. Is it a technical requirement somewhere? For the predefined language support?
- Line 603 needs a better comment
- All t() that contain a link (eg line 241) must use !link. This is also true for other files that contain links.

4) Functionality

- Since a user can add the Likebtn as field in any entity the old configuration is not needed.
You should remove the configuration settings shown below.

- Translated variables are saved in the database but they are not displayed!
- More than 2 fields do not work on the same enity. For example, a user may want to add a Likebtn for an image and one for the text. If this is not supported the module must provide a solution or clarify it on the documentation.
- Multiple instances also do not work for the same field. Same as above.

- Cannot disable module if I have created a Likebtn field even if I delete it! Related issue #943772: field_delete_field() and others fail for inactive fields

5) Propose

- A live demo of the user settings can be shown on the configuration page.

This module is getting better and better day by day!

1) likebtn.install

- Line 171: typo error, nested double quotes on the end

- For each 'description' argument you can use a full stop or not at the end. Make all descriptions use a full stop or the opposite (minor)

2) Field UI

- On admin/config/services/likebtn as also as field settings page the labels above each input field ('Like Button label translation', 'Dislike Button label translation' etc) maybe should not have the word 'translation'. Because these are the labels a user can override and then translate. If i18n is not enabled they are just labels.
Furthermore, the whole group should change it's name into something different (maybe 'Labels') with corresponding changes to the documentation.
This is a UX (user experience) notice. Not minor in my opinion.

- On admin/config/services/likebtn there are allowed only some default entity modes to display the likebtn. You can allow here all entity modes that exist with a multi select list form or checkboxes. User can create more entity modes but they will be unavailable here. See likebtn.module below.

- On admin/config/services/likebtn there are 2 default positions with default weight. Since other modules and the user can set a weight for their field the position 10 may not be always bottom etc. A weight select list is preferred with integer weight values () so you can reduce feature requests on the module issue.

- About default variable values: Maybe you should add default values on settings page (admin/config/services/likebtn) and on script (print all parameters) from the beginning. Is this a traffic issue?

3) likebtn.module

- Lines 243, 357. Probably they shouldn't be translated

- Line 40: You can can use hook_enity_view() for all entities that are enabled on the site. This way you can have nodes, comments, taxonomy terms and other entities in one multiple select list and can remove also function hook_comment_view() on line 144.

4)

Translation works!

I installed the module on a new drupal site.

Friendly advice. The project applications administrators will ask for real testing of modules to get the Review Bonus. Imagine me reviewing this module just looking at the styling issues. You seem to know Drupal coding if you make these changes so quickly.

1) About

- Lines 243, 357. Probably they shouldn't be translated

I mean why translate t(LikeBtn.com) and t('Profile ID')? Are they supposed to be translated?
How can you translate the 'LikeBtn.com'?
Also, the 'Profile id' for Addthis must remain as it is since it represents a parameter from this service.

The translation method was not the problem.

2) About default values you can test the loading time for both sort and full widgets as also as the loading time for the query of the module with or without the default variables (use devel for that). This will be a real testing.
But if you want to add empty default values you can just set empty default values. And if user adds no values and default are used don't print them in the $data.
Maybe I am missing something here but this code for variable_get() does the same work with

'#default_value' => ($default_values ? (isset($default_values['likebtn_settings_i18n_unlike_tooltip']) ? $default_values['likebtn_settings_i18n_unlike_tooltip'] : NULL) : variable_get('likebtn_settings_i18n_unlike_tooltip', NULL)),

A more advanced user can give us an opinion here...

According to this message you must manually delete the variables when the module is uninstalled.

Sorry for that, I had a wrong information before.

Will check other stuff ASAP.

I get an error after installation on path "admin/config/services/likebtn". Please check that.

Fatal error: Call to undefined function variable_get_value() in \sites\all\modules\likebtn\likebtn.module on line 110

Hi!

1) I get an error again on the configuration page. See that on a fresh installation. But it does not affect functionality.

Warning: Invalid argument supplied for foreach() in form_type_checkboxes_value() (line 2288 of MYDOMAIN\includes\form.inc).

2) My only concern is about translated links for example on lines 206, 207, 321, 322 of likebtn.module. See examples of Dynamic or static links and HTML in translatable strings. I got a warning about this for my modules. The correct pattern is to use the url() function with the appropriate options.

3) You may ask a Project Application Admin for the default values and if you should use variable_get() function or not.

After fixing 1 (and optional 2) I will change the status as RTBTC.

Message for the admins.

There is an (probably a no limitation) issue with default values of variables (see comment 14, 2). It would be great if you could propose a better method to find out if the module is using default values...

manual review:

1. "'access arguments' => array('administer likebtn settings'),": that permission is not defined with hook_permission()?
2. likebtn_entity_view(): doc block is wrong.
3. likebtn_entity_view(): this hook will fire on any entity, so you should add a check if the type is actually node before you do something. Or you could implement your functionality for any entity type? And why do you need to load a node when the $entity is the node already? And this will throw PHP notices if the entity does not have a nid property.
4. You could easily implement the like buttons as extra fields? See hook_field_extra_fields(). That would make it more flexible for admins to configure in what view mode they should appear.
5. _likebtn_get_markup(): do not add you JS like this, use drupal_add_js() or #attached in your render array.

The permission is a blocker. I have to go right now, so this review is not completed yet. I want to do some XSS testing with the settings when I get back.

manual review:

1. likebtn_menu(): you need to specify a permission that is needed to configure your module, how about you re-use "administer site configuration"? You cannot access the config page currently.
2. likebtn_entity_view(): the $node variable is very confusing, sometimes it is a node object, sometimes it is a comment. Why not leave it at the generic name entity?
3. And why do you need to load a node when the $entity is the node already?
4. likebtn_get_markup(): the Javascript should be put into a dedicated JS file and you should not directly add it to the markup. Use Drupal.settings to pass down variables from PHP to JS. See http://drupal.org/node/756722

As the config page is broken right now I cannot do the promised XSS testing for the $data variable that is injected unsanitzed in _likebtn_get_markup(), that would have been interesting. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Don't forget to set the status to "needs review" once you are ready again. See http://drupal.org/node/532400

manual review:

1. _likebtn_get_markup(): I would rather run check_plain() for sanitization, but I could not come up with an exploit scenario to work around the quote replacements. See also http://drupal.org/node/28984
2. likebtn_field_info(): so you specify your own field type, but I cannot edit any like button settings on a particular node?
3. likebtn_field_widget_form(): so that are only invisible form values that the user cannot edit?

But otherwise I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to dman as he might have time to take a final look at this.

Looks like you've been putting a lot of effort in here! Good work.

User testing:

The installation message directing me to the settings is very welcome :-) Great start.

While it seems to be pretty clear on the settings page that you may need information on pricing plans, it's *not* clear on the project page that this module connects to a paid third-party service. It would be good if it said that plainly on the project page (before* someone downloaded and installed it.
Although $1-$3 per year is tiny, it's something a user would want to know.

FANTASTIC documentation over at http://www.likebtn.com/en/#settings

It's nice that you allow configuration of the tooltips etc. However, I'd expect that the defaults would be *shown* pre-populated in the form so that I could decide whether I wanted to change them. As it is, it requires a little trial and error. Also, setting it to blank/no-tooltip doesn't seem to be possible this way.
Just minor user feedback.

Using:

The small popup that shows after "liking" doesn't go away until you close it manually. Normal behavior of that sort of feature would be for it to dismiss if you click away.

Functionally it seems to do what it says, although it seems to be a wrapper around the addthis server more than anything.

Code review:

Others above have done a lot of this. Looks sane, very readable.

As in #31.2 above, I'm a little confused about whether this utility is expected to be managed as a configurable field or a global per-content type setting. as a Field looks most useful. ... I see you've explained your thinking in #11 above. Maybe an explanation "you can set up a like button globally on the settings page, or per-content-type as a field - whichever method you prefer" on the settings instructions would clear this up.

I see that every instance on a page triggers a new json request, so if you have 40 comments on a page, that's 40 calls back to the likebtn.com server. :-(
I'd be concerned about the performance implications of making this service a dependency for hundreds or thousands of sites.

However

I'm really not thrilled that this utility adds 36K (half-minified) of unreviewable script in realtime from a server managed in Russia that we have to just trust!

Domain Name: LIKEBTN.COM
Registrar: REGTIME LTD.
Whois Server: whois.webnames.ru
Referral URL: http://www.webnames.ru

Registrant, Administrator, Technical:
Legato LLC
City: Samara
ZIP: 443002
Country: Russia

I realize that that's just the way that these social widgets have to work, but admins should really examine how much they trust the suppliers (Google, Facebook, Typekit, etc have a track record)
I don't want to unfairly point out that it's a .ru business ... the one saving grace is that it *didn't* try to hard to cloak itself, and the script is not yet fully obsfucated, so I'll give it benefit of the doubt for now.

HOWEVER. There is no privacy statement, no terms of service or security policy, and no clear disclosure of what data is being sent from your site to the Russian server, or what control the script may exert over your admin interface (technically, it could do whatever it wants to your site, including hijack UID1 or keylog everyones passwords. Maybe not todays script, but if it changes next month).

As someone who has to evaluate a module for a client, I would need to see this sort of stuff very clearly spelled out on the project page and think hard before I endorsed it or let anyone install it on a live site.

For this reason I find it hard, personally, to green-light this.
The code is great, the execution and behavior on the page is good, the documentation and admin UI is everything it should be. Responsiveness in the issue queue here has been exemplary.

It's just that the *potential* security hole this (type of service) opens up - means I could *never* install it on any site of mine or my clients. :-(
I'm not sure what sort of reassurance I would need to convince me otherwise. I may be completely wrong in this evaluation, and would love to hear why I should not worry so much.

Unassigning from me, because I'm unwilling to make that call. Sorry.

.dan.

Please note: All user accounts are for individuals. Accounts created for more than one user will be blocked when discovered. So I would recommend to personalize your account page so that there is no doubt.

While I agree with some points of dman I also acknowledge that when third party services are integrated the site administrator has to make the call whether or not to trust the service. Since dman agrees that the code is up to our standards I think we can continue here since we don't have any policies which third party service to allow/forbid. The security team or webmasters will act accordingly if we get reports of abuse or whatever.

Thanks for your contribution, likebtn!

I updated your account to let you 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 get 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.