[D7] World of Warcraft Tools

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

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.

Hi,

Please find my review comments :
a. Please remove your functions from wowtools.api.php, usually modulename.api.php is used to describe your hook or api usage of your module.
b. Please move code from wowtools.api.php to .inc file.
c. Remove .gitignore file from your repository.
d. Remove require_once from wowtools.module and use file key in hook_menu.
e. Use ajax commands instead of writing inline js for ajax callback function wowtools_apikey_remove_ajax

@pfumi, thankyou for your contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Yes, http://pareview.sh/pareview/httpgitdrupalorgsandboxpfu2193049git reported number of issues that need to be addressed.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Not Sure: Does not cause module duplication and fragmentation. I found World of Warcraft API module that seems something similar, but as I'm not familiar with this module so can't say anything.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified. Since I've not completed review of this module.

Coding style & Drupal API usage

1. (*) At configure page (admin/config/wowtools/settings), unable to add API key, getting following fatal error.
   Recoverable fatal error: Argument 1 passed to wowtools_api_data_valid() must be of the type array, null given, called in C:\xampp\htdocs\drupal731\sites\all\modules\2193049\wowtools.module on line 156 and defined in wowtools_api_data_valid() (line 111 of C:\xampp\htdocs\drupal731\sites\all\modules\2193049\wowtools.inc).
   
   
   
2. wowtools_menu(): The menu callback is using "administer blocks" but a separate permission would be good, too.
3. (+) wowtools_apikey_set_ajax(): To add js code, you are directly using <script>jQuery(document).ready that looks weird, same thing you are using at number of places. I would recommend you to use Drupal.behaviors, not jQuery(document).ready().
4. Instead of using json_decode(), should use drupal_json_decode().
5. In wowtools_build_curl() function you are using PHP cURL to send http get request, I would recommend you to use drupal_http_request() to hit the external URL.
6. Instead of rendering image using following code better to render it using theme_image()
   <img width='20' height='20' src='{$img_path}/images/classes/class-{$key}.png'><span class='wowclass wowclass-{$key}'>{$class['name']}</span>";

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

I'll stop review as one blocker issue is there and this project is huge too.

I'll review this module in next week, once you addressed above issues.

Thanks again for your hard work.

Hi @pfumi. In the process of composing a full review for your module today, so I will update this comment when i am finished. Just a few initial points.

As of False positives on submodule hooks support thread, you need to rename your submodule prefixed with your module name: wowtools_wowblocks. I would also do the following for consistency.

1. Prefix all .inc include files with your module name. E.g. - wowtools_wowblocks.raidprogress.inc
2. Move all the include files in your submodule wowtools_wowblocks to a sub folder includes within the submodule directory

I would also advise changing the git clone directory to that of the machine name of the module, as seems to be the standard. I can't think of a module that doesn't follow this convention off the top of my head.

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/pfu/2193049.git wowtools

Automated Review

Automated Review issues.

- Recommendations in previous comment.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

• Please detail the differences between this module and other WoW modules such as wow, wowguild, etc. on the project page / README and project application page.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

• What about adding a table of contents to the README as in the README Template.
• The README states the install location to be "/sites/all/modules/", but this may not be the case in multi-site or other setup variation.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) I got the following error for the setting of the API key the first time I tried.
   

         key="2yw7mchvrb2cs6zgg6yt62b65dekshs7" not valid
   Battle Net Error: There is a problem with Battle Net, try again later!
   

   Did the Server deny the key the first time or is the error detection incorrect?
   It worked the second time.
2. Is it possible to make the form work without AJAX also? It is unresponsive with javascript off. If not, how about adding the default AJAX progress throbber? I wasn't sure if the form button was broken.
3. For consistency with other modules I would change the permission to "Change WoWTools Settings" to "Administer wowtools", "Configure settings for wowtools" or even the human readable form might be best "Administer world of warcraft tools" to respect naming and case 'conventions'
4. Maybe add some comments to the README and the install file about the JSON file and it's purpose.
5. I would probably recommend an increase in the amount of inline comments throughout the .js, .inc and .module files, as of section 5.3 in Project application checklist
6. Is there any way we can get some test content into these blocks. I have a "Recruitment" block stating "closed" and an empty "Progress" block as shown in the screenshot attached.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

This review uses the Project Application Review Template.

A quick review of branch 7.x-1.x, commit 01a98db:

1. Why do you need to specify font-family in CSS file? It may be out of tune with current theme.
2. You included data.json file with predefined values. Are you sure there's no other way to request those values from somewhere and just to store them in your variables instead? Or you are going to watch over that data all the time and keep the file up-to-date?
3. (*) Constants defined in the module must be prefixed with a module's short name (Naming Conventions), also the documentation comments describing them should be above each (Drupal API documentation standards (general)). For example, in this file. Also I think all constants definition should live in .module file, so other modules could use them w/o including specific files. (This is a personal opinion.)
4. (*) _wowtools_api_data_valid function has untranslatable strings (String translation is mandatory). I don't know where and how this function is used, so maybe there shouldnot be translations? In this case add a comment about the reasons to the function doc.
5. (+) Optional parameters should be documented as "(optional)" - in round brackets (@param: Function parameters). Also some functions are not properly documented, so you should re-read "API documentation and comment standards" page carefully, especially a chapter about Form-generating functions.
6. (*) Consider using the url() function when building URLs like in wowtools_api_url function. Basically because you are not taking care of url-encoding some of URL parts by yourself.
7. .info file: Description is not very "descriptive". Also it should end with a dot. Don't know about how many other modules exist to work with WoW, but I think that the "WoWTools" package would be unique to your modules only. In general, this property should only be used by large multi-module packages, or by modules meant to extend these packages, such as Fields, Views, Commerce, Organic Groups, and the like (Writing module .info files (Drupal 7.x)).
8. (*) Never do this:
   

     function wowtools_uninstall() {
       db_delete('variable')->condition('name', 'wowtools_%', 'LIKE')->execute();
       cache_clear_all('variables', 'cache_bootstrap');
     }
   

   Always delete all the module variables with the variable_del() function.

9. Is there a real need of time gap? Add inline comment with the reasons.
10. There's no need to set a second argument, because a function takes only one.
11. (+) Avoid breaking the strings for translation into multiple lines, using other spaces (incl. tabs, not visible spaces) other than the default one, or using multiple consequtive spaces. It will be hard to work on localization with such strings.
12. (+) System paths should not start with a slash when getting passed to the system functions like url().
13. (+) Only the first character of the first word should be capitalized (unless a proper noun or acronym is used) in form element titles (User interface standards: Form elements).
14. Why you need to disable #ajax progress? A comment needed.
15. (*) When using explicitly set HTML id attribute prefix it with module name to avoid name collisions. Alternatively, id should run through drupal_html_id().

This is only for the main module, so please apply any new rules you learn to the included module as well.

Assigning to myself for next review.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

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

• 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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

(*) NO: Does not follow the guidelines for 3rd party assets/code. Unless you can point me to a place where it says usage of all images inside your module is OK as it is third-party.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template. Could be expanded more.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*)No: List of security issues identified.

1. (*) wowtools_wowblocks_recruitment_configure(): This is vulnerable to XSS exploits. If I enter <script>alert('XSS Apply Link');</script> in apply_link element, every time If I visit the associated page get a nasty javascript popup. You need to sanitize this before rendering, make sure to read https://www.drupal.org/node/28984 again. Also check the same for other places too in your code.
   
   
   
   
   
2. (*) _wowtools_guild_form_submit(): $guild['err_msg'] data coming from third party response and you are printing the same as it is (drupal_set_message($guild['err_msg'], 'error');), that is vulnerable to XSS exploits. You should sanitize this before printing See https://api.drupal.org/comment/35298#comment-35298
3. (*) _wowtools_wowblocks_raidprogress_settings_update_members_submit(): Same applies here for drupal_set_message($guild['err_msg'], 'error'); that need to be fix. You can check the same by changing this variable value to $guild['err_msg'] = '<script>alert("XSS")</script>';, will get a nasty javascript popup on form submission. Also check the same at other places in your code.

Coding style & Drupal API usage

1. (*) wowtools.info: You are adding wowtools.js file that include this file to every page that looks weird. Instead you add this js only where it is actually required.
2. Instead of using date(), should use format_date().
3. Instead of using time(), should use REQUEST_TIME.
4. (+) wowtools_install(): Do not set variables in hook_install() with variable_set(), you can just use default values with any variable_get().
5. wowtools_wowblocks_recruitment_content(): You are doing so html work in this function to theme the rendered array better to make separate template using theme().
6. (+) wowtools_wowblocks_recruitment_content(): you are directly keeping hyperlink within markup string instead insert hyperlink markup into the translatable string with placeholder. See https://www.drupal.org/node/322774. You can also use l() to make this anchor link. <a href=' . $view['apply_link']['link'] . '>' . $view['apply_link']['text'] . '</a>
7. wowtools_wowblocks_update_raid_progress(): To initiate curl you are not using drupal_http_request() here as you are using at other place in your code. A comment needed.
8. (+) At number of places you are using id (especially in form elements) HTML id attributes should be passed through drupal_html_id().

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

I do that now instead and removed everything I can't keep in my git repo. I hope this is acceptable :)

Yes this is fine :)

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.