[D7] Social Circle

Hi tush@r

Thanks for your module. I guess people building community sites will love this kind of module.

I did a review and saw the following problems:
- Could you start by adding the git command to get your code. This will make it easier for reviewers:
git clone http://git.drupal.org/sandbox/harsh/1431342.git social_circle
- You are working on the master branch. This is not good drupal practice. You should create a feature branch. The master branch must only contain a README.txt to refer developers to the feature branches. Check the info here
- You made a social_circle folder in your repository, that is not necessary. When using the git clone command above I end up having a social_circle folder with a social_circle folder in it.
- Also try to add some more information on you project page. The Tips for a great project page page is a good read and will show you how to do it.
- I am using an automated script to check for drupal coding standards. You can find it here: http://ventral.org/pareview/httpgitdrupalorgsandboxharsh1431342git
Please visit the page, it gives you a list of errors to fix (too extensive to post here).

When you have fixed the errors you can change the status again to "needs review".

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.

It appears you are working in the "feature" 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

Quick manual review:

1. Arrange CSS properties in alphabetical order see Drupal CSS Coding Standards
2. Remove Thumbs.db file from following folder
   images
   images\ie6
   images\img_inquisitor
3. To include external JS (jquery.livequery.min.js, jquery.corner.js, jquery.colorbox-min.js .... ) use Libraries API module
   which will i) enable multiple modules to use the same library, ii) simplify upgrades to the library code itself, and iii) avoid any potential issues with multiple versions of the same 3rd party library being installed by different modules
4. Rather than using $(document).ready() in your JavScript you should possibly employ Drupal's native method using Drupal.behaviors
5. In social_circle.install instead of t() use get_t() to ensure translations don't break at install time. You can see system.install for reference
6. Collect module path in a variable and use that variable value instead of calling drupal_get_path() 7-8 times in function show_social_circle from social_circle.module file

Automated review

http://ventral.org/pareview/httpgitdrupalorgsandboxharsh1431342git-feature

You can configure editor to avoid following error

• Spaces must be used to indent lines; tabs are not allowed
• Whitespace found at end of line

eclipse is very useful for above both
win version
windows->Preference->General->Editors->Text Editors => Displayed tab width and Insert spaces for tabs
windows->Preference->PHP->Editor->Save Actions => Remove trailing whitespace

Hi tush@ar, I found some coding standards issues, please check http://ventral.org/pareview/httpgitdrupalorgsandboxharsh1431342git-7x-1x

Personal review:

1. Remove scripts/jquery.ui.js, this file is included in Drupal core (7.x).
2. Remove translation files, this are added via localization.drupal.org.
3. In social_circle.info remove line 12 ,,version = "7.x-1.0" ’’ it's will be added by the deploy-system. => #881720: Remove "version = VERSION" from .info

Hi,

Manual Review:

1. Use ":" instead of "{" for foreach in all your tpl files.
ex : foreach ($mid as $mem_id) { in mutual-friends.tpl.php
2.Use t() for titles in social_circle.module file.
3.Add coment for parameters in social_circle.module line 287 & 288.
4.It seems you are concadenating expressions inside preg_match() in social_circle.module line 320.Concadenation is not required here,use a single string instead.
5.There are so many errors in your code validation.Please PAREVIEW your code and try to get it cleared.

Thanks,

My personal suggestion would be a sub folder named "themes" for als *.tpl.php file.

social_circle_menu(9
"add/circle" 'access callback' => TRUE,
are you shure that realy every one should be able to access this menu items.
Should you not better let the site admin choose.
And if i not completly miss understud the fact that you need $user->uid in social_circle_add_submit().
I would also say that you at least need the "registerd user" permissions.

define('PATH_TO_MODULE', drupal_get_path('module', 'social_circle'));
define('PATH_TO_LIBRARIES', 'sites/all/libraries/');
Constans should be prefixed with underline and module name, to avoid collusions with other modules.

$obj->removemember($_GET['mid'], $_GET['gid']);
With drupal 7 we have prepared statements, because of this it is secure.
But for persistant code use: (INT), check_plain(), filter_xss()

if (isset($_GET['uid']) and isset($_GET['gid'])) {
use "&&" insted of "and"

function social_circle_form_alter(&$form, &$form_state, $form_id) {
if ($form_id == 'user_register_form') {
Use "social_circle_form_user_register_form_alter(&$form, &$form_state, $form_id) {"
to save performance.
http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...

social_circle_validate_firstname_lastname()
Sufix methode with _validate( to let users see fast what for this methode is.

social_circle_schema()
You should prefix you table names with module name.
"user_group" could be used from another module.

Why you add "firstname" and "lastname" to drupal "user" table?
Have a look at http://your.drupal/admin/config/people/accounts/fields

multipleDiv.class
Define you methodes as public / private / protected

$out .= '

';
$out .= '

';
user drupal_add_js() drupal_add_css()

Dont use alert() in javascript for error messages.
Better theem it like http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_statu...

i m waiting for this module, plz realease it soon

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Remove all .DS_Store files from your repository.

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. jquery-ui.js and other javascript files appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
2. social_circle_schema(): all database tables should be prefixed with your module's name to avoid name clashes with others.
3. social_circle_init(): why do you need your CSS on every single page request? Same for the JS, only add it if it is actually needed.
4. social_circle_menu(): Using user_is_logged_in() is access callback might not be a good idea, because then you can't hide that feature from authenticated users. Better use standard permissions that can be assigned to user roles.
5. "require_once 'multipleDiv.class.php';": no need for that, the registry will autoload the class for you.
6. "public function addnewmembers()": method names should be camle case, see http://drupal.org/node/608152
7. social_circle_add_submit(): use drupal_write_record() instead of db_insert() or db_update() and you get schema validation for free.
8. social_circle_add(): javascript should be added with drupal_add_js(). Also elsewhere.

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

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

additional features updated.