OneAll Social Login

Welcome to Drupal project development. Please have a look at the following link that list Drupal coding errors. http://ventral.org/pareview/httpgitdrupalorgsandboxoneallcom1455166

I highly recommend that you install the Coder module which will help you review your code, on your server as you work, and ensure it meets drupal coding standards.

Once you are happy that your code is fixed and you have updated GIT please visit the Ventral link above and click on "Repeat Review". This will identify any remaining errors that Coder may have missed. When you do not have any coding errors please post an new comment and change the status of your request to "Needs review".

Drupal uses a database abstraction layer for added security and so that it can uses different database backends. The module.install file should have a hook_schema() function that defines the data storage. The hook_install and hook_uninstall function can call the schema to have it created or removed. See http://api.drupal.org/api/drupal/includes%21database%21schema.inc/group/...

If you have an INSTALL.txt file you do not need to repeat install instructions in the README.txt file

Clean stored variables before they are stored in oneall_social_login_admin_settings_submit_set_option() rather then every time you get them.

oneall_social_login_admin_settings()

• You can (and probably should) use "return system_settings_form($form)" at the end of oneall_social_login_admin_settings(). It will add/remove buttons for you.
• You do not need to specify a $form ['#submit'] callback if it has the same name plus "_submit" as your form. It is added by default.

oneall_social_login_admin_ajax_check_api_connection_callback()

• Sanitize all values from $_POST and $_GET See: http://api.drupal.org/api/drupal/includes%21common.inc/group/sanitization/7
• Don't think you can assume curl. I have seen many shared hosting sites that don't allow it. At least have a warning in the INSTALL.txt
• Use drupal_set_message() rather then generating your own HTML message code. See http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/drupa...

oneall_social_login_widget_form_alter()

• By the time hook_form_alter() processes $form is always an array and $form['#form_id] is set so you do not need to test these.
• It is better to use switch($form['#form_id]) rather than multiple elseif statements.

'side_pannel' should probably be 'side_panel'. Just a typo.

Hi Claude,

CodeSniffer is much more thorough but more difficult to set up locally. You can recheck your code after commits to drupal.org using the ventral.org site. Go to http://ventral.org/pareview/httpgitdrupalorgsandboxoneallcom1455166 and click on "Repeat review" tab.

If you're using NetBeans have a look at the following to aid setup to auto follow drupal coding standards:
http://drupal.org/node/1019816
http://drupal.org/project/nb_templates (great for hook templates)

Then you can Source -> Format and it will remove most formatting errors.

Hey nice job on all the fixes.

Error in oneall_social_login.admin.inc -> oneall_social_login_admin_ajax_check_api_connection_settings():
drupal_set_message(check_plain(($success_message), 'status oneall_social_login');
- has one too many '(' by the check_plain.

i don't think you can have copyright on GPL code
@copyright Copyright 2012 http://www.oneall.com - All rights reserved.

You should create a version branch in GIT and empty 'master' is preparation of going public:
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.

Do you have a demo account I could use for testing?

Well it looks like drupal.org has removed the personal contact form so I guess I will just create API keys.

Yeah I am no expert in IP law. I just wanted to bring it up. Just in case you should be aware of http://drupal.org/licensing/faq

Hi, Claude, i'm hoping you get the approval quickly so more people know about your project, i came across searching for an alternative solution and found it on your site.

i honestly went ahead and used your Module on my site Punto Android.
I was using JanRain, but never convinced me, your module works like a Charm,

Is your module going to incorporate social Widgets in content? To share the nodes, teaser full-view?

Never the less great work and thanks again!!

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review:

1. oneall_social_login_core.module: no need to include the full GPL header in the code files as there will be a LICENSE.txt added to packaged downloads anyway.
2. "Setup the callback handler uri": Comments should use proper punctuation and end in "." or "!" and such.
3. oneall_social_login_core_check_curl(): If your module requires cURL you have to check for that in hook_requirements(). See for example http://api.drupal.org/api/drupal/modules!simpletest!simpletest.install/f...
4. do not use json_decode(), use drupal_json_decode() instead.
5. oneall_social_login_core_user_insert(): this is a hook implementation and should be documented as such: http://drupal.org/node/1354#hookimpl . Also elsewhere.
6. oneall_social_login_core_preprocess_page(): I think you want to use hook_page_build() instead.
7. oneall_social_login_widget.info: why is there no dependency to your core module?

@tinker

@copyright Copyright 2012 http://www.oneall.com - All rights reserved.

GPL is no copyleft licence, so it always remains the ownership of the developer, who can do whatever he wants with his own code, even if it would not allowed by the GPL. That's in fact the meaning of copyright.

By the way anything without @copyright notice, is automatically copyrighted by it's developer (or creator for art projects).

The automated script does not find any errors.
In overall, I only see one serious issue (10.)
If those issues should be corrected (some are only recommandations), and I succeed the tests, I would give a "reviewed and tested" by the community. Here my review:

1. I suppose it is not recommanded using TAB's as in the DOC comments.
2. Comment " // Done.", don't know what you mean with that one ;)
3. is_object($result) AND property_exists($result, 'code') AND $result->code == 200
=> it's recommanded using && instead of AND
=> the same at other places, also with OR => ||
4. IMO: It would be better splitting long if conditions into more rows, it makes reading and controling much easier, example:
Before:
if (is_array($social_data) AND isset ($social_data['response']) AND isset ($social_data['response']['request']['status']['code']) AND $social_data['response']['request']['status']['code'] == 200) {
After:

if (is_array($social_data) 
  AND isset ($social_data['response']) 
  AND isset ($social_data['response']['request']['status']['code']) 
  AND $social_data['response']['request']['status']['code'] == 200) {

(I just hate scrolling, maybe other people think differently)
5. IMO: One should do the same for query building:
Before:

$aid = db_select('authmap', 'a')->fields('a', array('aid'))->condition('module', 'oneall_social_login', '=')->condition('authname', $user_token, '=')->execute()->fetchField();

After:

$aid = db_select('authmap', 'a')
  ->fields('a', array('aid'))
  ->condition('module', 'oneall_social_login', '=')
  ->condition('authname', $user_token, '=')
  ->execute()
  ->fetchField();

6. Typ: One can use the l() function for creating links, instead of adding them manually, as in t('Social Login powered by OneAll'

7. @license GNU/GPL 2 or later
Should be omitted, as automatically every code published on drupal.org must be GPL, and will be added automatically to projects

8. Tabs used instead of spaces, you should always use double-spaces in place of tabs:

    $js = "oneall.api.plugins." . $plugin_type . ".build(\"" . $containerid . "\", {
						'providers' : ['" . implode("','", $providers) . "'],
						'callback_uri': '" . $callback_uri . "',
						'force_reauth' : " . ($plugin_type == 'social_link' ? 'true' : 'false') . ",
						'user_token': '" . (!empty($token) ? $token : '') . "',
						'css_theme_uri' : '" . (($https_enabled ? 'https://secure.' : ' http://public.') . 'oneallcdn.com/css/api/socialize/themes/drupal/default.css') . "'
					 }); <!-- oneall.com / " . ucwords(str_replace("_", " ", $plugin_type)) . " for Drupal / v1.0 -->";

9. It's recommanded putting HTML into theme-functions or into theme files (see hook_theme()):

        '#prefix' => '<div style="margin:20px 0 10px 0"><div id="' . $containerid . '">',
        '#suffix' => '</div></div>',

at least i would recommand setting margins per css.

10. WARNING: foreach ($oneall_social_login_available_providers as $key => $provider_data) {
$oneall_social_login_available_providers is never initialized!

--
The response time for a review is now approaching 4 weeks.
Get a review bonus and we will come back to your application sooner.
See: http://drupal.org/node/1410826

You could consider evaluating my own application:
http://drupal.org/node/1302786

Removing "PAReview: review bonus"

2. Comment " // Done.", don't know what you mean with that one ;)

Hmm...I sometimes tend to over-comment. I have now removed these ;)

Well don't tell anyone that I would have told you to remove a comment ... someone told me to never recommend deleting comments at another review ^^

10. WARNING: foreach ($oneall_social_login_available_providers as $key => $provider_data) {
$oneall_social_login_available_providers is never initialized!

Sorry, but you are wrong ;) Above the call you have require_once 'oneall_social_login.providers.php';. $oneall_social_login_available_providers is defined in this file. I have now put require_once directly above the loop and added a comment. It should be clearer now.

Hmm, would it not be better making a functioncall, which returns that array?
If not I would suggest adding comments explaining why do did that way ...

I will recheck your project when I have time ...

I took a look at your code and I much more like it now, I would just suggest modifying the following.
I did not see any big issues, and didnt't yet have time for testing, so I let the status unchanged.

1. t('<a href="@link_social_login">Social Login</a> powered by <a href="@link_oneall">OneAll</a>',
Here you could also use l() function.

2. watchdog('oneall_social_login_core', 'CURL Error:' . curl_error($curl), WATCHDOG_ERROR);
curl is not defined, or am i missing something?

3. You could use settings, see: http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_add... , for the following:

// Build Javascript.
$js = "oneall.api.plugins." . $plugin_type . ".build(\"" . $containerid . "\", {
'providers' : ['" . implode("','", $providers) . "'],
'callback_uri': '" . $callback_uri . "',
'force_reauth' : " . ($plugin_type == 'social_link' ? 'true' : 'false') . ",
'user_token': '" . (!empty($token) ? $token : '') . "',
'css_theme_uri' : '" . (($https_enabled ? 'https://secure.' : ' http://public.') . 'oneallcdn.com/css/api/socialize/themes/drupal/default.css') . "'
});

";

4. IMO: Could be adivsable putting the following into theme functions / tpl files

         '#prefix' => '<div style="margin:20px 0 10px 0"><label>' . $title . '</label><div id="' . $containerid . '">',
        '#suffix' => '</div></div>',

Small hint: I found the following quote by patrickd:

Please remove all license information out of your readme, also the copyright stuff. this should be fully handled by the license.txt which will be added later automatically.

Sorry for the delay, you can speed up your application by doing 3 more reviews of other project applications and adding the review bonus tag.

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

• Drupal Code 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. Get a review bonus and we will come back to your application sooner.

manual review:

1. oneall_social_login_core_menu(): this is a hook implementation and should be documented as such, see http://drupal.org/node/1354#hookimpl . Please check all your hooks.
2. oneall_social_login_core_unmap_identity_token(): that DB table is not defined in this module? Maybe you should move the schema definition to oneall_social_login_core?
3. oneall_social_login_widget_add_js_plugin(): module_exists() is useless here as you already depend on the core module.
4. oneall_social_login_widget_is_ssl(): why can't you use the global $is_https variable?
5. oneall_social_login_admin_ajax_check_api_connection_settings(): why do you need check_plain() here for $api_key and $api_secret, you are not printing them to the user?
6. "drupal_set_message(check_plain($success_message), 'status oneall_social_login');": $success_message does not contain user provided input so it is useless to sanitize it (you can ignore false positives thrown by the coder module in this case). Same for $error_message.
7. oneall_social_login_admin_settings(): why do you use system_settings_form() to store your settings in the variable table if you never access them with variable_get()? It seems that you save your settings in your own table, so remove that.
8. "'#title' => htmlspecialchars($provider_data['name']),": use check_plain() instead. Also elsewhere.

This module is great, works "out of box". Only thing you have to do is register with oneall.com and enter keys on settings page. Code is good written, using drupal api and looks clean. Code commented good enough, it's shame to put it on waiting list so long.

My only suggestions (for future improvement):

- to see this module even more integrated and cut down lines of code, You can use change system_settings_form ( http://api.drupal.org/api/drupal/modules%21system%21system.module/functi... ) for configuration and variable_get/variable_set later on. You can make use of drupal caching later on and also spare one table on mysql server.
- You could also save user authentication tokens as user entity fields, but that would be unnecessary overkill I guess ;)
- You could auto-register user whenever email and username is available from provider (such as google or facebook), because site visitor actually does not want to "register" on site, they just press "login with google" and that's it. It should happen "behind the scenes".

Anyway, I really believe this project is well built and good to go, we should release it for users. In case something comes up, there's always an issue queue ;)

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

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

  FILE: .../modules/pareview_temp/test_candidate/oneall_social_login_widget.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   25 | ERROR | The PHP language keyword "global" must be all lower case
  --------------------------------------------------------------------------------
  

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. "drupal_set_message($success_message, 'status oneall_social_login');": the second parameter is the message type and that type does not exist. Also elsewhere.
2. Remove all @copyright headers from your source code files, all code on drupal.org is automatically GPL. You can add credits to your README.txt file.
3. oneall_social_login_menu(): The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.
4. oneall_social_login_admin_settings_submit(): use drupal_write_record() instead of db_insert() and db_update() and you'll get schema validation for free.
5. oneall_social_login_core.install: do not use drupal_install_schema()/drupal_uninstall_schema() in you install hooks, this is handled automatically in Drupal 7.
6. oneall_social_login_core_form_alter(): why do you need to check if $form is an array here? Also, the form id is provided as third parameter to hook_form_alter() so you can simply use that.

Although the wrong permission is a severe issue I don't consider it an application blocker at this point. Just make sure to fix it.

Thanks for your contribution, ClaudeSchlesser! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.