Support from Acquia helps fund testing for Drupal Acquia logo

Comments

PA robot’s picture

Status: Needs review » Needs work

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

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.

rayzzz.com’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus
rayzzz.com’s picture

Issue summary: View changes
rayzzz.com’s picture

Status: Needs work » Needs review
rajiv.singh’s picture

Nice module, for Social sites
Installation was okay, worked fine.
Code needs to be formatted as drupal standard,
Please see http://pareview.sh/pareview/httpgitdrupalorgsandboxrayzzzcom2073153git

sonu.raj.chauhan’s picture

Status: Needs review » Needs work

First of all nice concept.

But here are the list of issues which needs to fixed
1) Since you are contributing this module to drupal.org, you need to make sure that your module follows the proper drupal coding standards and uses its core api functions. It is one thing to extend the behaviour of drupal which is fine, but it's another to reinvent the wheel which i found is the case in your module. for eg

a) using init.inc.php to define your table schema. You can simply use hook_schema to define your table.
b) use .inc extensions instead on .php.
c) using raw sql queries in your code. You should use drupal database api for the same
d) Use of module_load_include is recommended over require_once
e) Instead of using

tag you should use drupal_add_js f) You are not supposed to add header and licensing information in your module or install file. It will be added by drupal.org into the .info file once it is approved. g) Use of drupal renderable arrays and/or theme api to return output to page callbacks is recommended 2) Once done with coding standards and api issues you can check your module at http://pareview.sh Its not that the module will not work but it will be difficult to maintain and upgrade the module if these standards are not followed.
rayzzz.com’s picture

Status: Needs work » Needs review

Hello
Thanks for your review. Fixed some issues in the list. Others need your advise.
1)
a) using init.inc.php to define your table schema. You can simply use hook_schema to define your table. - DONE
b) use .inc extensions instead on .php. - DONE
c) using raw sql queries in your code. You should use drupal database api for the same - this module is being used in other CMS as well and I can't change the main "rz_module" file like that as the code is being used in other CMS too. Maybe there is a way to leave raw sql queries?
d) Use of module_load_include is recommended over require_once - the same problem as in c)
e) Instead of using tag you should use drupal_add_js - DONE
f) You are not supposed to add header and licensing information in your module or install file. It will be added by drupal.org into the .info file once it is approved. - DONE
g) Use of drupal renderable arrays and/or theme api to return output to page callbacks is recommended - DONE. The only output is:
return '

';
Should I use any theme for that?

2) Once done with coding standards and api issues you can check your module at http://pareview.sh
Here is the message I found there: We're upgrading :) please check back later!
So I wasn't able to check that, but will as soon as they are upgraded.

MissterX’s picture

Since the code was commented above I will stick to functionality.
Server: CentOS Linux version 2.6.32-358.23.2.el6.x86_64
PHP: 5.3.3
Drupal: v7.31
Webserver: Apache/2.2.14
Modules: a bunch of 'em

Client OS: Windows XP
Browsers on one above workstation:
1) Firefox 20.0 - admin login
2) Opera 12.17 build 1863, anonymous user
3) IE 8, anonymous user
4) Google Chrome Version 36.0.1985.125 m, anonymous user

Result:
a) On all clients Flash: "Adobe developers key is invalid...." - Not so pleasent message.
b) 2) connected, 1) sent a text which 2) did NOT receive
c) 2) sent a text which 1) received
d) 4) connected
e) 3) connected sent a text, 4) received and nobody else
f) 4) sent a smiley, 3) received and nobody else
g) 1) sent a smiley, nobody received
h) 2) sent a smiley, nobody received
...
a "couple" of more of these iterations occured but this should give you a hint.

By the way - do you check what Flash version is installed at the client side?
Do you have a min. req. for Flash version?

klausi’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: single application approval
FileSize
54.06 KB

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

  • 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/rzchat.install
    --------------------------------------------------------------------------------
    FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
    --------------------------------------------------------------------------------
     17 | WARNING | Do not use INSERT queries with db_query(), use db_insert()
        |         | instead
    --------------------------------------------------------------------------------
    
  • 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. README.txt is not in UTF-8 encoded, please change the encoding. Same for rz_module.php and possibly other files.
  2. Why does the module name start with "rz"? What does "rz" mean?
  3. The project page should mention first that this module requires a third party account and API key. There should be appropriate links.
  4. rzchat_install(): why does the table name change for the insert query? Shouldn't it always be the same? Please add a comment.
  5. rzchat.module: why do you need to include the init.php in the global scope? Just register your classes with files[] in the info file and Drupal will autoload them for you. See https://www.drupal.org/node/542202
  6. rzchat_menu(): doc block is wrong, this is a hook implementation. See https://www.drupal.org/coding-standards/docs#hookimpl
  7. rzchat_menu(): why is the page callback and the access arguments dynamic? That does not make sense, because then the permission might not be available? Please add a comment.
  8. rzchat_page(): do not use jQuery(document).ready(), use Drupal.behaviors instead. See http://drupal.org/node/756722
  9. It seems like a lot of PHP files are supposed to be used standalone without a Drupal bootstrap involved? This will not work on secure server configurations which forbid direct execution of PHP files in subdirectories such as module directories. Please add a hint aqbout that to the README.txt.
  10. Please document all class variables on all classes. And use better names and less abbreviations. What does $oDb mean? Why to "o" prefix?
  11. rz_module.php and db.php: you are not using the Drupal database API at all, so this will only work on MySQL? A lot of code looks prone to SQL injection because you directly concatenate variables into DB strings. I did not try to exploit this, so I'm not sure if there is a security issue. Anyway, you must use the DB API correctly, make sure to read http://drupal.org/writing-secure-code again.
  12. Since there is not really much Drupal API usage present I think this project on its own does not warrant giving you the git vetted user role. We can promote this single project manually once this is ready.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

rayzzz.com’s picture

Issue summary: View changes
rayzzz.com’s picture

Status: Needs work » Needs review

@MissterX you should add Adobe key to make it work properly. The light version doesn't guarantee messages posting.

automated review:
FIXED

manual review:

README.txt is not in UTF-8 encoded, please change the encoding. Same for rz_module.php and possibly other files.
FIXED

Why does the module name start with "rz"? What does "rz" mean?
"rz" is extension from rayzzz (rayzzz.com is my site) and "rzchat" is a unique name comparing to "chat"
The project page should mention first that this module requires a third party account and API key. There should be appropriate links.
FIXED

rzchat_install(): why does the table name change for the insert query? Shouldn't it always be the same? Please add a comment.
FIXED

rzchat.module: why do you need to include the init.php in the global scope? Just register your classes with files[] in the info file and Drupal will autoload them for you. See https://www.drupal.org/node/542202
FIXED

rzchat_menu(): doc block is wrong, this is a hook implementation. See https://www.drupal.org/coding-standards/docs#hookimpl
FIXED

rzchat_menu(): why is the page callback and the access arguments dynamic? That does not make sense, because then the permission might not be available? Please add a comment.
FIXED

rzchat_page(): do not use jQuery(document).ready(), use Drupal.behaviors instead. See http://drupal.org/node/756722
these 2 functions are different from each other: jQuery(document).ready() happens just once and behaviors can happen more than 1 time

It seems like a lot of PHP files are supposed to be used standalone without a Drupal bootstrap involved? This will not work on secure server configurations which forbid direct execution of PHP files in subdirectories such as module directories. Please add a hint about that to the README.txt.
FIXED, now everything goes through Drupal.

Please document all class variables on all classes. And use better names and less abbreviations. What does $oDb mean? Why to "o" prefix?
"o" means object, "i" - integer, "s" - string and etc. Documented.

rz_module.php and db.php: you are not using the Drupal database API at all, so this will only work on MySQL? A lot of code looks prone to SQL injection because you directly concatenate variables into DB strings. I did not try to exploit this, so I'm not sure if there is a security issue. Anyway, you must use the DB API correctly, make sure to read http://drupal.org/writing-secure-code again.
Since there is not really much Drupal API usage present I think this project on its own does not warrant giving you the git vetted user role. We can promote this single project manually once this is ready.
Yes, I am trying to leave the main file "rz_module" unchanged to make it easier to integrate this module into other CMS. So if it's possible, then just make this project full. All other projects (if there will be some) should start their way from sandbox as well.

irfworld’s picture

Space in variable name found in file rz_module.php in following code:

$s_ ontents = parent::actionConfig();
    $i_file_size = (int) $this->getSettingValue("fileSize");
    $i_max_file_size = min((ini_get('upload_max_filesize') + 0), (ini_get('post_max_size') + 0), $i_file_size);
    $s_ ontents = str_replace("#fileMaxSize#", $i_max_file_size, $s_ ontents);
    $s_ ontents = str_replace("#soundsUrl#", $this->sUrl . "data/sounds/", $s_ ontents);
    $s_ ontents = str_replace("#smilesetsUrl#", $this->sUrl . "data/smilesets/", $s_ ontents);
    $s_ ontents = str_replace("#loginUrl#", $this->sLoginUrl, $s_ ontents);
    return $s_ ontents;

The space from variable "$s_ ontents" should be removed.

Also the swfobject.js file should be formatted as per a standard JS file.

rayzzz.com’s picture

Thanks for bug report. $s_ ontents variable is fixed.
swfobject.js file is grabbed its main project source and shouldn't be changed to avoid big file size.

davidam’s picture

Automated Review

I've found some errors. You can introduce this link in the summary of the project
http://pareview.sh/pareview/httpgitdrupalorgsandboxrayzzzcom2073153git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
No: You must announce similar projects to give a good evaluation and
explain the differences, to become interesting install it. For example, exists:
+ https://www.drupal.org/project/videochat
+ https://www.drupal.org/project/vconf
+ https://www.drupal.org/project/v2wvc
Master Branch
Yes: Follows the guidelines for master branch.

Licensing
No: Follows the licensing requirements, attending specially to 15.

3rd party code
No: Follows the guidelines for 3rd party code. For instance, swfobject.js is a third party code.

README.txt/README.md
Yes: Follows the guidelines for in-project documentation and the README Template. Consider include the external documentation inside the module.

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
No. If "no", list security issues identified. See issue below.
1.(+) You must use the database abstraction layer to avoid SQL injection attacks

Coding style & Drupal API usage
1.(*) You must change lang directory by translations directory.
2.(+) I suppose that is better all swf in an only directory.

klausi’s picture

Status: Needs review » Needs work

Yep, swfobject.js needs to be removed. 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.

PA robot’s picture

Status: Needs work » Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

rayzzz.com’s picture

Status: Closed (won't fix) » Needs review

Long time absent here. Just fixed all problems except adding correct class documentation comments. Somebody provide me with the correct example of it, please.
As for differences from other videochats listed on this site, they are:
https://www.drupal.org/project/videochat
having smilesets, different users statuses, "watching me" section, chat history and others
https://www.drupal.org/project/vconf
different design, users' created rooms, password protected rooms, different users statuses, "watching me" section, chat history, 1 to 1 video chat integrated and others
https://www.drupal.org/project/v2wvc
This one is just for 1 to 1 videochats without rooms and many to many chat possibility

klausi’s picture

Status: Needs review » Needs work
Issue tags: -PAreview: single application approval +PAreview: review bonus, +PAreview: security
FileSize
2.78 KB

Looks like your forgot to add the review bonus tag? Added it for you.

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

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. audio_video_chat.info: why is this info file in the repository? There is no module with that name?
  2. RzchatIntegration: this looks vulnerable to SQL injection. Never concatenate dynamic variables directly into SQL strings, always use placeholders with db_query() instead. Make sure to read https://www.drupal.org/writing-secure-code again. Please check all your queries. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
  3. rzchat_uninstall(): drupal_uninstall_schema() will be called automatically for you, so this can be removed.
  4. RzchatDbConnect: why is this class needed? Please add a comment. It doesn't really do anything?
  5. RzchatDbConnect::fetch(): the foreach loop is not necessary here, just use ->fetchAssoc() on the result set.
  6. project page: there is a lot of marketing speak, but little info how this module actually works. Please improve it with https://www.drupal.org/node/997024
  7. Now that the third party JS file is removed do users have to download it somewhere? What is the install/config process now?
  8. I think there is enough code here to grant you the git vetted user role once everything is cleared up, so removing the single promote tag.
PA robot’s picture

Status: Needs work » Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

rayzzz.com’s picture

Status: Closed (won't fix) » Needs review

1. audio_video_chat.info: why is this info file in the repository? There is no module with that name?
removed
2. RzchatIntegration: this looks vulnerable to SQL injection. Never concatenate dynamic variables directly into SQL strings, always use placeholders with db_query() instead. Make sure to read https://www.drupal.org/writing-secure-code again. Please check all your queries. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
fixed queries. Please let me know what the security tag is.
3. rzchat_uninstall(): drupal_uninstall_schema() will be called automatically for you, so this can be removed.
fixed
4. RzchatDbConnect: why is this class needed? Please add a comment. It doesn't really do anything?
removed
5. RzchatDbConnect::fetch(): the foreach loop is not necessary here, just use ->fetchAssoc() on the result set.
all queries are fine now except 2 with GROUP_CONCAT operator. Autoreview reports that these queries are vulnareable for Possible SQL injection. Please advise what to do with them.
6. project page: there is a lot of marketing speak, but little info how this module actually works. Please improve it with https://www.drupal.org/node/997024
Do you mean the main page of the project with its description?
7. Now that the third party JS file is removed do users have to download it somewhere? What is the install/config process now?
Now swfobject is not used, the html code is used instead

klausi’s picture

Status: Needs review » Needs work
FileSize
2.64 KB

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

  • 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/rz_module.php
    ---------------------------------------------------------------------------
    FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
    ---------------------------------------------------------------------------
      571 | WARNING | Do not use TRUNCATE queries with db_query(), use
          |         | db_truncate() instead
      575 | WARNING | Do not use TRUNCATE queries with db_query(), use
          |         | db_truncate() instead
     1105 | WARNING | Do not use TRUNCATE queries with db_query(), use
          |         | db_truncate() instead
    ---------------------------------------------------------------------------
    
  • 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. project page: yes I mean the page at https://www.drupal.org/sandbox/rayzzz.com/2073153 should be improved with the instructions from https://www.drupal.org/node/997024
  2. as you can see the automated script complains about SQL injection. Not sure if it is applicable in your case but please always use placeholders with db_query() to be on the safe side. You are doing it already, so just do the same for the variables that you concatenate into your queries. See https://www.drupal.org/writing-secure-code the Drupal 7 section for db_query().
  3. rz_module.php: why do you suppress PHP warnings with the "@" operator here, for example with "@fopen($s_file_name, "rt");"? Either remove that or add a comment why you do it.

Can't really say more about the code since I don't know if and how XSS could be a thing in Flash/SWF. The missing db_query() placeholders are a blocker right now, otherwise I would say let's go on with this.

PA robot’s picture

Status: Needs work » Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.