[D7] SocialMediaBar

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

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.

Hi dragonmantank,

This seems like a useful module. I noticed a couple things off the bat:

• Whereas you mentioned that an API key for Sharethis is not required, it is actually marked as required by your admin form.
• Despite it being required on the form, the form doesn't actually stop submitting if it's empty.
• It's recommended to use a system_settings_form for this kind of thing so that you don't have to write your own submit/validate functions: https://drupal.org/node/1111260.
• The API key isn't actually being saved. In your submit function, you're looking for the value in $form_state['values'], but it lives in $form_state['input'], since you're collecting using '#value' instead of '#default_value'.
• '#default_value' in the above item should probably be used so that users can see the saved API key when looking at the form, unless you mean for it to be hidden.
• You should probably register your config page in your .info file so that the modules page can correctly register a config page for your module: https://drupal.org/node/542202
• You may also want to consider creating an admin permission for your module.
• Personally, I think it would benefit UI to open up the share functionality in new (popup) windows, instead of new tabs.
• It's a little confusing that the sharethis email share button opens on hover, whereas the rest open on click.
• I know that it's a separate project, but your jquery_visible module could use a README file with a link to the repo and instructions for installing the library.

also, forgot to mention in last comment, but the ShareThis API variable isn't uninstalled along with the other two in hook_uninstall()

Hi dragonmantank,

I have some suggestions, about your module.

* Use drupal_get_query_parameters instead of $_GET;

* Use drupal_goto instead of header('Location:' ...

* Use drupal_exit() instead of die()

* Add watchdog_exception('socialmediabar', $e) in catch block;

* Missed t() for titles of some elements.

* Wrong variable name:

function socialmediabar_uninstall() {
  variable_del('socialmediabar_sharethis_apikey');
  variable_del('socialmediabar_enabled_networks');
  -variable_del('socialmediabar_sharethis_apikey');
  +variable_del('socialmediabar_cache_time');
}

* Place description to fieldset description instead of separate element:

  $form['sharethis_settings'] = array(
    '#type' => 'fieldset',
    '#title' => 'ShareThis Settings',
+   '#description' => 'This module requires signing up with ShareThis and getting an API key through the developer program. Please visit <a href="http://developer.sharethis.com/">http://developer.sharethis.com/</a> to get signed up and request a key.'
  );

- $form['sharethis_settings']['sharethis_instructions'] = array(
-    '#type' => 'markup',
-    '#markup' => 'This module requires signing up with ShareThis and getting an API key through the developer program. Please visit <a href="http://developer.sharethis.com/">http://developer.sharethis.com/</a> to get signed up and request a key.',
-  );

* Use url() function:

-  $url = 'http://rest.sharethis.com/v1/count/urlinfo?'
      . http_build_query(compact('url', 'provider', 'api_key'));

+  $url = url('http://rest.sharethis.com/v1/count/urlinfo', 
      array('query' => array(compact('url', 'provider', 'api_key'))));

* Specify module-related wrapper in selector:

 -$('.share-button').click(function() {
 +$('.socialmediabar .share-button').click(function() {

* It sould be more fast and looks better if front-end send single request for all counters to path (/socialmediabar/countproxy);

* Make feature with .visible() optional. Some time it`s not looks good. Also it requires extra module from sandbox.

I hope it will help you to make module better.

Thank you.

Hello dragonmantank.
I still see a lot of warnings. Pls look attacment. It is very difficult to review.

Also i found one error in css file:
.socialmediabar .container:hover { background: }

About Network.
Not all PHP installations come with cURL.
Perhaps this should be indicated in the readme.txt?

Thanks.

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

• Remove the LICENSE, drupal.org packaging will add a LICENSE.txt file automatically.
• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  FILE: /home/klausi/pareview_temp/socialmediabar.module
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
  --------------------------------------------------------------------------------
    79 | WARNING | Unused variable $jq_visible_library.
   112 | WARNING | #description values usually have to run through t() for
       |         | translation
   139 | WARNING | #description values usually have to run through t() for
       |         | translation
   158 | WARNING | #description values usually have to run through t() for
       |         | translation
  --------------------------------------------------------------------------------
  

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:

1. Please add the differences to existing projects to the project page and follow the tips for a great project page: https://drupal.org/node/997024
2. socialmediabar.module: licensing text: drupal.org is GPLv2+, so you cannot license it as GPLv3+. Please remove all copyright and licensing terms, all code on drupal.org is automatically GPLv2+ and a license file will be added to packaged downloads.
3. socialmediabar_admin_main(): you don't need this function, jsut use drupal_get_form as page callback in hook_menu() and it will use your form automatically.
4. socialmediabar_countproxy(): looks like there is user facing text in there? Then that should run through t() for translation.
5. socialmediabar_countproxy(): the 500 status code means that something is broken on the server. You should use some 4xx status code to indicate that the client did somethig wrong, right?
6. As you are using PHP 5.3 namespaces you should specify the minimum PHP version in the info file (Drupal 7 only requires PHP 5.2).
7. socialmediabar_admin_form(): no need to document $form and $form_state here, see https://www.drupal.org/node/1354#forms
8. socialmediabar.tpl.php: the template should not do any logic like filtering the networks, that should happen beforehand. Fully prepared variables hsould be passed to the template.
9. socialmediabar.tpl.php: There should not be any script tags in the template. Any Javascript should be added to the render array with #attached.

The licensing issue is a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hello dragonmantank!
Thanks for your work, your module is pretty good,

I have few remarks:
you don't register file of admin panel in .info
and there is no admin permission for your module

Things that got my attention right away;

• Please add the line git url to the bottom of your project info;
• git clone http://git.drupal.org/sandbox/dragonmantank/2279295.git socialmediabar
• After enabling the module I got two notices of undefined index module
• Notice: Undefined index: module in FieldInfo->prepareInstanceWidget()
• Notice: Undefined index: module in FieldInfo->prepareInstanceDisplay()

Functional testing:

• I downloaded and enabled the module the usual way.
• I went to configuration (/admin/config/services/socialmediabar) and configured it.
• I added echo socialmediabar_render(); to page.tpl.php
• And got the social bar, great!
• But got a 404 Not Found popup (using localhost) stating;
• The requested URL /socialmediabar/shareproxy was not found on this server.

PAReview: (results)

• Some incorrect indentation (two spaces too many) on line 270, 271, 273

Coder review (using the minor (most) option to see everything.

• socialmediabar.module
• You should not enclose menu titles within a t(). (line 36 and 42).
• $visibleLib should be $visible_lib (no camelCase) (line 79).
• Always have a space after 'if' statements (control statements) (line 81, 82, 246).
• String concatenation should be 'space' + 'dot' + 'space' + 'the rest' (Line 266).
• socialmediabar.js
• I think the @file block should be the first thing in the file, it says @file block missing.

PaReview Result:

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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.

FILE: /var/www/drupal-7-pareview/pareview_temp/js/socialmediabar.js
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 22 | ERROR | [x] Expected 1 space before "+"; newline found
 23 | ERROR | [x] Expected 1 space before "+"; newline found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/js/socialmediabar_email.js
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 12 | ERROR | [x] Additional whitespace found at end of file
 13 | ERROR | [ ] Files must end in a single new line character
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/css/socialmediabar.css
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 71 | ERROR | [x] Whitespace found at end of line
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 59 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Time: 316ms; Memory: 14Mb

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

Manual Review:

Also i was just going through your code & the admin form, and found that the following message coming TWICE whenever some one submits the module admin setting form:

"SocialMediaBar requires cURL to be enabled in your PHP setup. Please enable cURL to activate SocialMediaBar."
"You have enabled "Always Visible" but have a broken installation of jQuery Visible. Please download and install the jQuery Visible module. Until this is corrected, Always Visible will not function."
(Please refer the attached screenshot("SocialMediaBar.png") for the same.)

Thank you for the contribution @dragonmantank!

Automated reviews

PAReview didn't throw any errors.

Manual review

The variables which are set using the system_settings_form should be deleted in hook_uninstall() in a install file of the module. Naming conventions of the variables follow the standards.

Hi,

This seems to be a very useful module.
Please do fix the Pareview issues. It's still showing some issues.
Please have a look on the attached file and fix those as quick as possible.

Thanks

Looks like you forgot the review bonus tag. Assigning to myself for the next review.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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:

1. socialmediabar.module: why do you need to globally require NetworkFactory.php? You should use the files[] directive in the info file for autoloading instead, see https://www.drupal.org/node/542202#files
2. '#markup' => '<strong>Always Visible</strong> - Please download and install the jQuery Visible module to enable "Always Visible" options.',: all user facing text must run through t() for translation. Same for "$errors[] = 'Please provide a URL to share.';". Please check all your strings.
3. socialmediabar_modules_enabled(): this should be moved to hook_requirements, where you can actually check before installation if cURL is available. Example: https://api.drupal.org/api/drupal/modules!simpletest!simpletest.install/...
4. socialmediabar_render(): no need to check function_exists('curl_version') here again, you should just rely on that when you checked it in hook_requirements().
5. Twitter.php: no need for the require_once statements I believe, you can again just use the class registry with files[]?
6. socialmediabar.js: instead of using $(document).ready() here you should use Drupal.behaviors, see https://www.drupal.org/node/756722#behaviors
7. The project page should mention that this module does not do anything out of the box and that users need to add something to their templates.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

1. .module: line 48 should pass the text through t().
2. Line 98: The URL should be extracted and passed as an argument to t().

3.   if (count($errors)) {
       drupal_add_http_header('Status', '400 Bad Request');
       drupal_json_output(array('errors' => $errors));
       drupal_exit();
     }

   This gets repeated several times, maybe move to a common error function?

4. try/catch for Exception is bad form. Anything could throw an exception. Not all of them should be caught. Instead, catch the appropriate curl exceptions or throw an application specific exception inside NetworkFactory, etc.
5. .install file: the hook_updates should use the format socialmediabar_update_7001. It is missing a zero.
6. There is no sanitization of $url and $message as it gets passed into buildShareURL(). This could lead to parameter tampering when used with a specially crafted URL. Not much can be done about this, so it isn't a blocker.

However, none of this is blocking. So...

Thanks for your contribution, dragonmantank !

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.

Well, generally would be nice to have a block which will just render the social media bar.

The second point is about centering the popup window, when you're trying to share smth:

js/socialmediabar.js, line: 24

window.open(proxy_url, '_blank', 'menubar=no,width=500,height=500');

could be changed to:

var width = 500;
var height = 500;
var left = ($(window).width() - width) / 2;
var top = ($(window).height() - height) / 2;

window.open(proxy_url, '_blank', 'menubar=no,width='+width+',height='+height+',left='+left+',top='+top);

And btw, is it possible to make a shares counter for LinkedIn as well?

Thanks.

re #26: This issue is focused on vetting the user's ability to use the Drupal API and understand how to securely code. Your feedback is great but doesn't affect the decision to grant the ability to promote this to a full project. At this point, your feedback should move into an issue in the project's issue queue.