Social media module

Needs some work to bring the code to proper coding standards. Once you've fixed the stuff on the page below, I'll be sure to investigate its functionality!

http://ventral.org/pareview/httpgitdrupalorgsandboxenxebre1431492git

http://ventral.org/pareview/httpgitdrupalorgsandboxenxebre1431492git

These have been fixed?

why we need this module? While we already have better than yours. Did you do something different?

check this one..
http://drupal.org/project/sharethis

You should remove the files from the master branch, and only use the 7.x-1.x branch (http://drupal.org/node/1127732)

You might also explain what the difference is between this module and http://drupal.org/project/service_links and http://drupal.org/project/social-share (I assume your module is more flexible because it also supports links on entities)

Also, is there a reason why you use

variable_get("social_media_buttons") != NULL ? variable_get("social_media_buttons") : array();

in social_media.admin.inc instead of just

variable_get("social_media_buttons", array()); ?

Please don't assign the issue to your self, only the current reviewer should do this.

Sorry for the delay
as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

This is quiet useful module.

There are still issues in codestyle, see here: http://ventral.org/pareview/httpgitdrupalorgsandboxenxebre1431492git-7x-1x
please fix them.

these are just minor issues, not worth blocking deeper reviews

Hi Enxebre,

I have just done a manual and automatic review of your module. Here are my findings:

There are no license issues with the code in this module, no third party libraries are included.

While there are similar modules out there, I do not consider this to be a duplicate of any existing module, and this is echoed in the readme file.

There still appear to be some coding issues that need to be fixed: http://ventral.org/pareview/httpgitdrupalorgsandboxenxebre1431492git-7x-1x

As your module has an admin page (if the UI module is enabled) you should add the configuration parameter to the UI module's .info file so that the admin link shows up in the admin/modules/list page.

social_media.admin.inc is loaded on line 8 of .module file but is also included in hook_menu.
If this include is providing functions that are called upon throughout the .module file I would
separate these into another include file, leaving just the code for the menu callback in the file
included by hook_menu.

line 56 social_media.admin.inc: change comment to read:

/**
 * Implements hook_form_BASE_FORM_ID_alter().
 */

In the file 'social_media_ui.module', change line 21 from

'type' => MENU_LOCAL_TASK,

to

'type' => MENU_DEFAULT_LOCAL_TASK,

Doing this will make sure that the first tab is active when you go to your admin section from the main admin menu.

social_media_gplusone.module:

In this function social_media_gplusone_social_media_process_element you have used " instead of ' for
consistency I would correct this.

The module installed and uninstalled cleanly.

IMHO the some of the points above should be fixed before going any further.

Hi Enxebre,

After a manual review of your code, I found the following:

+ Inconsistent use of single and double quotes between modules, within modules and within the same functions. Some examples:
- social_media.admin.inc - line 22 and line 29 - $form[.
- social_media_ui.admin.inc - line 273 and line 277 - $form_state[.
- social_media.module - line 107 and line 108 - $element[.
+ Inconsistent spacing between array elements, variable declarations, etc. Some examples:
- social_media.module - line 134 versus social_media_ui.admin.inc - line 218.
- social_media.module - line 94 versus line 164.
- social_media.module - line 157 - "weight" =>40 versus "weight" => 40.
+ What is the purpose of social_media_ui.module? - line 68 - if (1 == 1) { - It will always evaluate to true.
+ Comment issues that do not follow coding standards. See Doxygen and comment formatting conventions page
- Various cases of missing param - social_media_ui.admin.inc - line 137 - $button or line 231 - $value or line 150 - $current_button.
+ Newline in comment in social_media_ui.module - line 86. Saw all of the coding standard review comments above. Not sure why some of this stuff wasn't caught then.
+ Missing default value in variable_get in social_media.api.php - line 21, social_media_ui.module - line 66 and social_media_ui.admin.inc - line 249.

The examples should give you an idea of where I found issues although there are more cases. Please be sure to thoroughly check each of the cases across all of your code.

Good luck.

Manual review:

- In general I don't like the use of _ (underscore) in urls. You make use of it in your admin paths. But this is just my point of view.
- There is some coding practises which are a bit wierd, such as l.90, l.151 of social_media.module. You are casting a value to an integer, and check if its equals to 1. Why just not using a boolean instead ?
- The README file is very nice, however notice that you pointed your URL to http://drupal.org/user/, instead of your own user ID.
- There is a temporary file to clean up in your social_media_ui: social_media_ui.info~.
- In social_media_gplusone, l.36, you can use drupal_add_js() instead of.

Although you should fixe these issues, I think I can RTBC you.

Sorry, actually you should really fixe these issues. I put it as need work because there is not so much to do.

README.txt
1. Has a lot of trailing whitespaces on lines of text.

social_media.module

2. The $output variable in function social_media_help should perhaps be rendered via a themeable template
3. Throughout this project there is inconsistent user of single quotes and double quotes. Which can look a little untidy in some cases. In most situations it is preferred to use single quotes when no variable substitution is required and there is limited escaping required.
example:
social_media.module:242: $extra[$entity_type][$bundle_name]["display"]['social_buttons'] = array(
try
social_media.module:242: $extra[$entity_type][$bundle_name]['display']['social_buttons'] = array(

It looks especially odd when single and double quotes are used together when accessing a specific element of a multidimensional associative array.

4. In function social_media_preprocess_social_media_container you have a superfluous ; on
$variables['element']['social_buttons'] = social_media_constructor($variables);;
not a biggie

modules/social_media_ui/social_media_ui.install
5. in function social_media_ui_install
What is the reasoning behind doing variable_initialize(array("social_media_ui_buttons")); and not using it's return value?
Please add an in-line comment to explain this?

modules/social_media_ui/social_media_ui.info
6. Please consider adding a configure parameter to the info file for your social_media_ui.

With the above issues attended to, as far as I can tell, apart from coding standards issues, of which there are a few, this module is looking pretty good now.

There are a lot of applications waiting, if you you would like to speed up your process, please take part in the review bonus program: http://drupal.org/node/1410826

As already said there are numerous modules that do very similar things:
http://drupal.org/project/sharethis
http://drupal.org/project/service_links
http://drupal.org/project/addthis

Module duplication is a huge problem on drupal.org and we prefer collaboration over competition. Please take a look at these 3 modules, I think you could easily develop your improvements for one of them. I guess there are even more similar modules out there that I have not found right now.

Get in contact with the maintainer of the module that you prefer. If that fails for whatever reason and you absolutely have to create yet another social media module, then please add a comparison to each of the existing projects to your project page. That will ease the module selection process for site builders. Of course they would not have to choose if you just incorporate your stuff into one existing module ;-) If you have to do that set this issue back to "needs review".