[D7] Blizzard Community Platform API

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.
Review of the master branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• ./modules/guild_rank/guild_rank.install: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function guild_rank_install() {
  function guild_rank_requirements($phase) {
  function guild_rank_uninstall() {
  

• ./modules/guild_rank/guild_rank.character.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function guild_rank_character_presave($character) {
  

• ./modules/guild_rank/guild_rank.admin.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function guild_admin_ranks($form, &$form_state) {
  function guild_admin_ranks_order_submit($form, &$form_state) {
  function guild_admin_rank($form, $form_state, $rank) {
  function guild_admin_rank_validate($form, $form_state) {
  function guild_admin_rank_submit($form, $form_state) {
  function guild_admin_rank_delete_submit($form, &$form_state) {
  function guild_admin_rank_delete_confirm($form, &$form_state, $rank) {
  function guild_admin_rank_delete_confirm_submit($form, &$form_state) {
  

• ./modules/guild_rank/guild_rank.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function guild_rank_user_load($users) {
  function guild_rank_boot() {
  function guild_rank_menu() {
  function guild_rank_load($rid) {
  function guild_rank_load_by_name($rank_name) {
  function guild_rank_permission() {
  function guild_ranks() {
  function guild_rank_save($rank) {
  function guild_rank_delete($rank) {
  function guild_rank_theme() {
  

• ./modules/guild/guild.admin.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function guild_information_settings($form, &$form_state) {
  function guild_information_settings_validate($form, &$form_state) {
  function guild_information_settings_submit($form, &$form_state) {
  function guild_information_settings_submit_optional($form, &$form_state) {
  function guild_information_region_callback($form, $form_state) {
  

• ./modules/guild/guild.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function _character_rank_cmp($a, $b) {
  function guild_members_save($guild) {
  

• ./modules/guild/guild.install: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function guild_install() {
  function guild_schema() {
  function guild_schema_alter(&$schema) {
  function guild_uninstall() {
  

• ./modules/guild/guild.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function guild_load_multiple($gids = array(), $reset = FALSE) {
  function guild_load($gid = NULL, $reset = FALSE) {
  function guild_load_by_name($realm, $name) {
  function guild_save($guild) {
  function guild_fetch($realm, $guild_name, array $fields = array(), array $options = array()) {
  function guild_fetch_guild($guild, array $fields = array(), $force = FALSE) {
  function guild_members_select($alias = NULL, $gid = NULL, $options = array()) {
  function guild_is_default($guild) {
  function guild_set_default($guild) {
  function guild_character_set_main_access($character, $account, &$rights = array()) {
  function guild_menu() {
  function guild_hook_info() {
  function guild_entity_info() {
  function guild_theme() {
  function guild_cron() {
  function guild_permission() {
  function guild_rewards($id = NULL, $reset = FALSE) {
  

• ./modules/character/character.install: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function character_requirements($phase) {
  function character_install() {
  function character_schema() {
  function character_uninstall() {
  

• ./modules/character/character.pages.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function character_user($account) {
  function character_user_add($form, &$form_state, $account) {
  function character_user_add_validate($form, &$form_state) {
  function character_user_add_submit($form, &$form_state) {
  function character_set_main_status($main, $account, $rights) {
  function character_user_delete_form($form, $form_state, $account, $cid = 0) {
  function character_user_delete_form_submit($form, &$form_state) {
  function character_set_main($character) {
  function character_user_unblock_form($form, &$form_state, $account, $cid = 0) {
  function character_user_unblock_form_validate($form, &$form_state) {
  function character_user_unblock_form_submit($form, &$form_state) {
  function character_page_title($character) {
  function character_view_page($character) {
  function character_show($character) {
  function character_view_multiple($characters, $view_mode = 'teaser', $weight = 0, $langcode = NULL) {
  function character_view($character, $view_mode = 'full', $langcode = NULL) {
  function character_build_content($character, $view_mode = 'full', $langcode = NULL) {
  

• ./modules/character/character.admin.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function character_admin($callback_arg = '') {
  function character_filter_form() {
  function character_filter_form_submit($form, &$form_state) {
  function character_admin_account() {
  function character_admin_account_submit($form, &$form_state) {
  function character_admin_account_validate($form, &$form_state) {
  function character_admin_settings() {
  function character_filters() {
  function character_build_filter_query(SelectQuery $query) {
  function character_character_operations($form = array(), $form_state = array()) {
  function character_character_operations_set_main($character) {
  function character_character_operations_unblock($characters) {
  function character_character_operations_block($characters) {
  

• ./modules/character/character.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function character_menu() {
  function character_help($path, $arg) {
  function character_access($op, $character, $account = NULL) {
  function character_use_guild_ranks($character) {
  function character_permission() {
  function character_load_multiple($cids = array(), $reset = FALSE) {
  function character_load($cid = NULL, $reset = FALSE) {
  function character_load_by_name($realm, $name) {
  function character_delete($cid) {
  function character_delete_multiple(array $cids) {
  function character_save($character) {
  function character_hook_info() {
  function character_entity_info() {
  function character_races($id = NULL, $reset = FALSE) {
  function character_classes($id = NULL, $reset = FALSE) {
  function character_fetch($realm, $character_name, $fields = array(), array $options = array()) {
  function character_theme() {
  function character_uri($character) {
  function character_preprocess_forum_submitted(&$variables) {
  

• ./tests/guild.test: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function guild_test_fetch($realm, $guild_name, array $fields = array()) {
  

• ./modules/guild/guild.admin.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  139- */
  

• ./modules/guild/guild.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  236- */
  

• ./wow.admin.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  126- */
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  
  css/roster.css
  css/wow.css
  modules/guild_rank/guild_rank.css
  modules/guild_rank/guild_rank.install
  modules/guild_rank/guild_rank.character.inc
  modules/guild_rank/guild_rank.info
  modules/guild_rank/guild_rank.admin.inc
  modules/guild_rank/guild_rank.module
  modules/guild/guild.admin.inc
  modules/guild/guild.info
  modules/guild/guild.inc
  modules/guild/guild.install
  modules/guild/guild.module
  modules/character/character.install
  modules/character/translations/ressources.php
  modules/character/character.info
  modules/character/character.css
  modules/character/character.theme.inc
  modules/character/character.pages.inc
  modules/character/character.admin.inc
  modules/character/character.module
  modules/character/blizzard.tooltip.js
  modules/character/character.tooltip.js
  modules/character/blizzard.core.js
  modules/wow_item/wow_item.module
  modules/wow_realm/wow_realm.module
  modules/wow_realm/wow_realm.info
  tests/wow_test.test
  tests/wow_test.info
  tests/wow_test.module
  tests/guild_rank.test
  tests/resources/eu.battle.net/item/classes.json
  tests/resources/eu.battle.net/guild/rewards
  tests/resources/eu.battle.net/guild/perks
  tests/resources/eu.battle.net/character/races.json
  tests/resources/eu.battle.net/character/classes.json
  tests/guild.test
  wow.admin.inc
  wow.info
  wow.install
  wow.module
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  modules/guild/guild.install:2:// $Id$
  modules/character/character.install:2:// $Id$
  modules/character/translations/fr.po:1:# $Id$
  modules/character/translations/character.pot:1:# $Id$
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./modules/guild_rank/guild_rank.install ./modules/guild_rank/guild_rank.character.inc ./modules/guild_rank/guild_rank.info ./modules/guild_rank/guild_rank.admin.inc ./modules/guild_rank/guild_rank.module ./modules/guild/guild.admin.inc ./modules/guild/guild.info ./modules/guild/guild.inc ./modules/guild/guild.install ./modules/character/character.install ./modules/character/translations/ressources.php ./modules/character/character.info ./modules/character/character.css ./modules/character/character.theme.inc ./modules/character/character.pages.inc ./modules/character/character.admin.inc ./modules/character/character.module ./modules/character/blizzard.tooltip.js ./modules/character/character.tooltip.js ./modules/character/blizzard.core.js ./modules/wow_realm/wow_realm.info ./css/roster.css ./css/wow.css ./wow.install ./wow.admin.inc ./wow.info ./wow.module ./tests/wow_test.test ./tests/wow_test.info ./tests/wow_test.module ./tests/guild_rank.test ./tests/guild.test
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

Yep your' right, seems like false positives.

IMHO your submodules sound a little unspecific, maybe something like blizzard_guild would be better ? just a suggestion.. ;)

See http://ventral.org/pareview/httpgitdrupalorgsandboxsylvainlecoy1256604git

Switched back to needs review, so in-depth reviews won't be blocked by coding standart issues.

Regarding comment #5, I'd suggest adding the wow_ prefix to your submodule functions, even if it starts to look a bit silly.

The reason behind this suggestion is so that users who are attempting any customization or extension of your module's functionality can look at the main module file to determine your function naming pattern; rather than needing to look at every submodule in order to avoid declaring any duplicate php function names.

As an example, if I were to attempt to build some additional enhancements or functionality in a custom module after installing this one, I would initially assume that you are following common Drupal naming patterns, and using the 'wow_' prefix consistently throughout your module and submodules. Based on this assumption, I may decide to name my custom module 'guild', thus creating the possibility of namespace collisions with the php functions in your guild_rank submodule.

The difference between your examples is that (in contrib) the field types are packaged as seperate modules ... address, name, etc can all be downloaded individually. These are 'field modules', but they are not 'submodules included in the field module package'.

The pattern I'm strongly suggesting is "one download, one namespace". The fact that there are other projects that don't follow this pattern is irrelevant to the application and/or recommendation ... we can go back and forth all day finding exceptions to best practices; but one of the goals of this process is to educate and enforce best practices in a new contributor's initial module.

Once approved by this process, contributors are granted free reign to produce new modules without review or restriction; and plenty of non-best practices are introduced into contrib due to this freedom. Since this application process is the only touch point where we can actively educate applicants and encourage best practices to be used once the applicant is released into contrib, reviewers will tend to stick to their guns and enforce these practices to their full extent. Arguments that 'Contrib module X did it this way' don't really carry much weight from this perspective.

That said, we also recognize that interpretations can differ from person to person, and what one might take as a strong opinion might be consider trivial to the next ... so we do offer a forum for discussion the best practices themselves (and/or a place to request a second opinion if a debate arises with a reviewer) ... please feel free to raise this debate at http://groups.drupal.org/code-review, so that it can be discussed and tracked outside of this individual application (and so the discussion can be easily found the next time the same issue is raised).

Your 'these namespaces aren't taken' comment reinforces the point ... the guild namespace isn't taken at drupal.org/project/guild ... but how can you easily confirm or prove that it hasn't already been taken as the name of a 'sub-module' in another project?

My concern would be primarily with php function names, as a duplicate function name can take down the entire site. I'd also recommend that at least 'character' be namespaced, as 'character' is a pretty generic term that can mean any number of things outside of the scope of WoW or RPGs, and thus the possibility of collisions is higher. Based on this, and from the perspective of consistency, the rest should thus probably be prefixed as well.

Your correct that this is only a recommendation, which is why I've used the term 'suggest' and not bumped your application down to 'needs work' because of it. But I would say it is at least enough of a convention that whether the resulting function name looks silly should not be part of the decision process. :)

We'll leave it in 'needs review', since it hasn't yet received a true in-depth review yet ... most reviewers will read the history as well, so should see the above conversation. (That said, be patient with them if they don't!)

The way it works is that anyone can review and mark RTBC, and then a 'Git Admin' will visit and approve the application (or bounce it back to needs review if any issues have been overlooked). If you'd like to solicit another member of the Drupal community to review your application, they can find review guidelines and other information at http://groups.drupal.org/code-review

Unfortunately, there are around 150 projects awaiting reviews at any given time, and only a small handful (3-10 volunteers) of active 'deep-dive' reviewers ... so the wait in the queue can be somewhat frustrating.

While the longest wait between responses currently being experienced is in the neighborhood of 3-4 weeks, the good news is that this is a vast improvement over 6 months ago, when the wait for some applications could grow to three times that. :( Rest assured that the delay is simply a function of an overload of applications and too few reviewers ... and should not be interpreted as a lack of interest in your particular project.

Thanks again for your patience!

Have a look at #1410826: [META] Review bonus to get a review quicker

Large suite of modules, leveraging multiple Drupal Apis, active repository, and waiting way to long ... Only did a partial review, but what I saw looks good. I'm willing to suggest we make this happen.

On second thought, I don't see a single sanitization function in the theme character code ... And since I can't really do a proper deep dive review on a tablet, I can't confirm whether this lack of check_plain or filter_xss calls in the theme components is covered elsewhere - and thus can not second you're RTBC claim at this time. (And I don't think I need to point out that, despite the wait and understandable frustration, marking your own issues as RTBC without review is heavily frowned upon in the Drupal community.)

On page admin/config/wow/guild, I chose "Lightning's Blade" and set my guild to "Section Eight". This brings up an error of "The guild name Section Eight does not exist on Lightning's Blade. You should first check that your guild exists at: http://us.battle.net/wow/guild/Lightning's Blade/Section Eight/."

You are not URL encoding spaces, so it is causing an error when trying to wow_http_request anything with a space (realm name or guild name would cause this).

Hi,

I found following

1. In wow.install file you delete same variable on hook_uninstall as you do in wow_update_7000 witch seems unnessessary since if module is updated there is no longer variable to delete and if module is not updated that hook_update_ won't be executed.

function wow_update_7000() {
  // Removes unused.
  variable_del('wow_cdn');

2. param $host is not existing

/**
 * There are no required query parameters when accessing this resource, although
 * an array parameter can optionally be passed to limit the realms returned to
 * one or more.
 *
 * @param $host
 *   Optional host to use. If null, uses the site's default.
 * @param array $realms
 *   The list of realm.
 */
function wow_realm_fetch($region, array $realms = array()) {

3. Please consider using drupal_json_decode instead of json_decode. It's basically the same, but you will get array.

  $response = drupal_http_request($url, $options);

  if ($response->code == 200) {
    return json_decode($response->data);
  }

4. If you set your own variables, please delete them in hook_uninstall, check all forms and make sure you delete variables saved by them

   variable_set('wow_item_classes', $values);

5. Please document your custom hooks in *.api.php, see http://drupal.org/node/161085#api_php

Other than that, it looks nice (and big) module and hopefully we get lot of guild-pages to Drupal ;)

You should also check http://ventral.org/pareview/httpgitdrupalorgsandboxsylvainlecoy1256604git witch an help you to see Drupal coding style issues.

Many of those propably can be skipped, but following coding style helps readability.

Thanks for your patience.

Re: #29: Because the API call is not within the scope of your module's control (i.e. it is a 3rd party API call), it should not be inherently trusted as you have suggested. I would still stand by the suggestion that you need to sanitize on output. (EDIT: Or at least on the DB Select as in your posted example .. but for all items.)

Please consider that there are about 450 other projects in the queue sitting either in 'needs work' or 'needs review' in the queue, and about 70 of those have been in there longer than your project. We realize that the wait is unacceptable, but do the best with the volunteer manpower that we have.

This delay is a bit part of the reason why initiatives such as http://drupal.org/node/1410826 have been introduced ... many applicants have jumped on board with the attempts to help improve the situation; if you are frustrated with the delay, I can only recommend you pursue the 'PAReview Bonus' as well (which will result in certain reviewers granting your project increased priority when reviewing).

Removing review bonus tag, you have not listed any reviews in the issue summary as specified in #1410826: [META] Review bonus.

Updated issue summary.

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

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. You have to get a review bonus to get a review from me.

manual review:

1. wow_http_request(): use drupal_json_decode() instead of json_decode(). Also elsewhere.
2. wow_test.info: "version = VERSION": Remove that line from all your info files.
3. "class GuildSaveTestCase": all classes in your project need to be properly prefixed with your module's name to avoid collisions. Also elsewhere.
4. CSS: "@CHARSET "ISO-8859-1";": Remove that, all code files in Drupal are UTF-8.
5. translations folder: remove that, translations are now done on localize.drupal.org.
6. wow_character_user_add_validate(): User names are user provided input and need to be sanitized before printing in error messages to avoid XSS exploits. Use the "@" placeholder with t() and will get check_plain()'ed. Same for the realm name I guess. See http://drupal.org/node/28984
7. Same in theme_wow_character_description() and wow_guild_rank_form_user_admin_account_alter(). You should only use the "!" placeholder if you have previously escaped the data anyway and want to make use of markup in it. Please check all your code where you falsely use "!".
8. wow_realm: empty module, so remove it.
9. wow_item_install(): no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once dirname(__FILE__) . '/mymodule.inc';. Applies to all module_load_include() calls in your project.

The security issues are blockers, so I have to set this back to "needs work". Overall the code looks good so I think we are nearly there. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Please don't remove the security tag, we keep that for statistics.

Added one project.

The "PAReview: security" tag is used to get an estimation how many project applications contained security issues and to show examples of security problems.

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

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./modules/wow_character/wow_character.install:                         PHP script, ASCII text, with CRLF, LF line terminators
  ./modules/wow_character/wow_character.api.php:                         PHP script, ASCII text, with CRLF, LF line terminators
  ./modules/wow_character/wow_character.module:                          PHP script, ASCII text, with CRLF, LF line terminators
  ./modules/wow_realm/wow_realm.module:                                  PHP script, ASCII text, with CRLF, LF line terminators
  

• 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. You have to get a review bonus to get a review from me.

manual review:

1. For including files of parent folders: I'm not sure about about a clean solution for that, so I guess keeping module_load_include() is ok in that case. Using "../.." or similar should be fine, too.
2. wow_character.api.php: *.api.php files should only be used for documentation purposes and never be actually included or executed, see http://drupal.org/node/161085#api_php
3. wow_character_user_add_validate(): no need to use check_plain() here as the "@" and "%" placeholder in t() will sanitize for you. And double escaping is bad.
4. wow_character_user_add_validate(): "form_set_error('name', t('@name is already in your character list.'));": replacement arguments for t() are missing.
5. wow_character_user_add_validate(): the user name is still using the "!" placeholder, so this is still vulnerable when malicious user names come into play.
6. "class GuildRankUserRolesAssignmentTestCase" misses the wow prefix.
7. The automated review shows still some trivial errors you should fix.

Although you should definitively fix those issues I think my points are no blockers, so I say RTBC. The user name security issue is minor as most special characters are forbidden anyway in standard Drupal installations. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

After installing this module on a fresh install here are some things I've come across after giving the module/code a quick glance (using commit aec336bf):

Miscellaneous bugs:

• Line 64 of wow_character.install has a require_once to a file that does not exist (wow_character.resources.inc).
• Line 76 of wow_character.module references an undefined callback. I assume this should actually be 'wow_character_admin_settings'.
• Line 276 of wow_guild.module has a call to wow_guild_character_set_main_access() in the middle of nowhere. Said function is tagged with @deprecated in the documentation and also causes various exceptions, preventing the installation of the module.
• You cannot uninstall the Item Resources module (which also blocks removal of the main module). Drupal registers a field in use by a non-existant 'Item' type and disables the module's enable/disable checkbox.

Project issues:

In wow_item.install your hook_uninstall() implementation does not cooperate with other modules. You're technically using the Drupal 7 equivalent of the code fragment shown on that page.

While the sub-modules in this project seem to be fairly unique, could you explain how the main module is different from this project: Blizzard API?

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

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./modules/wow_character/wow_character.resources.inc:                   PHP script, ASCII text, with CRLF, LF line terminators
  

• 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. You have to get a review bonus to get a review from me.

Still some code style errors, but no blockers. RTBC again.

Thanks for your contribution, Sylvain Lecoy! 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.

Overall, any unresolved bugs may or may not become issues for your project later on. However, they aren't a priority for this review.

As for the module comparison, the goal was to check your awareness of Drupal's collaboration over competition ethos. Throwing away one of the modules would be a bit drastic!

Also, this guy beat everyone, but sandbox projects aren't "existing solutions" to compare against, especially since there are a lot like his with no code in them. Still, waiting around for 7 months (this issue and your first commit were on Nov 23) really sucks. As Blizzard API became a full project on Oct 17, I guess the early bird really does get the worm. =/

That said, it looks like your wait is over. Congratulations!