The Audio/Video Chat module provides users with ability of chatting and broadcasting video and audio without necessity to install expensive media server.
https://www.drupal.org/sandbox/rayzzz.com/2073153
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/rayzzz.com/2073153.git rzchat
Reviews of other projects:
https://www.drupal.org/node/2228407#comment-9005159
https://www.drupal.org/node/2237645#comment-9005211
https://www.drupal.org/node/2308015#comment-9007107
https://www.drupal.org/node/2102313#comment-9075503
https://www.drupal.org/node/2319575#comment-9075713
https://www.drupal.org/node/2313065#comment-9075809
Comment | File | Size | Author |
---|---|---|---|
#21 | coder-results.txt | 2.64 KB | klausi |
#18 | coder-results.txt | 2.78 KB | klausi |
#9 | coder-results.txt | 54.06 KB | klausi |
Comments
Comment #1
PA robot CreditAttribution: PA robot commentedThere 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.
Comment #2
rayzzz.com CreditAttribution: rayzzz.com commentedComment #3
rayzzz.com CreditAttribution: rayzzz.com commentedComment #4
rayzzz.com CreditAttribution: rayzzz.com commentedComment #5
rajiv.singh CreditAttribution: rajiv.singh commentedNice module, for Social sites
Installation was okay, worked fine.
Code needs to be formatted as drupal standard,
Please see http://pareview.sh/pareview/httpgitdrupalorgsandboxrayzzzcom2073153git
Comment #6
sonu.raj.chauhan CreditAttribution: sonu.raj.chauhan commentedFirst 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.
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.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
Comment #7
rayzzz.com CreditAttribution: rayzzz.com commentedHello
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.
Comment #8
MissterX CreditAttribution: MissterX commentedSince 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?
Comment #9
klausiReview of the 7.x-1.x branch (commit fbd3ef8):
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:
Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #10
rayzzz.com CreditAttribution: rayzzz.com commentedComment #11
rayzzz.com CreditAttribution: rayzzz.com commented@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.
Comment #12
irfworld CreditAttribution: irfworld commentedSpace in variable name found in file rz_module.php in following code:
The space from variable "$s_ ontents" should be removed.
Also the swfobject.js file should be formatted as per a standard JS file.
Comment #13
rayzzz.com CreditAttribution: rayzzz.com commentedThanks 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.
Comment #14
davidam CreditAttribution: davidam commentedAutomated 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.
Comment #15
klausiYep, 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.
Comment #16
PA robot CreditAttribution: PA robot commentedClosing 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.
Comment #17
rayzzz.com CreditAttribution: rayzzz.com commentedLong 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
Comment #18
klausiLooks 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:
Comment #19
PA robot CreditAttribution: PA robot commentedClosing 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.
Comment #20
rayzzz.com CreditAttribution: rayzzz.com commented1. 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
Comment #21
klausiReview of the 7.x-1.x branch (commit dfd1a8b):
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:
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.
Comment #22
PA robot CreditAttribution: PA robot commentedClosing 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.