Tournaments series

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:

• tournaments_series.module in tournaments_series.info: It's only necessary to declare files[] if they declare a class or interface.
• ./includes/tournaments_series_helper.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function _insert_tournaments_series($rule, $table) {
  function _played($id) {
  function _delete_tournaments_series($id, $table) {
  function _load_names_tournaments_series($db_table) {
  function _load_series_id($tournament_id) {
  function _load_players_names_tournaments_series() {
  function _explode_textarea($textarea) {
  function _implode_textarea($area) {
  function _ioc_codes() {
  function _get_land_name_from_ioc_code($code) {
  function _delete_standings($serie) {
  function _load_rules($serie) {
  function _load_players_points($serie, $rules) {
  function _save_standing($players_positions, $serie) {
  function _standings($series) {
  function tournament_series_rankin_options_array() {
  function _set_up_ranking(&$players_positions, $rules_key) {
  function _standings_sum(&$players_positions, $player_key, $player, $option, $rules_key) {
  function _standings_mutual(&$players_positions, $player_key, $player, $rules_key) {
  

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

  function ajax_ts_autocheckboxes_callback($form, $form_state) {
  

• ./includes/tournaments_series_helper.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
  

  14- *  A string width table name
  40- */
  61- * 
  63- */
  96- *  table name 
  99- *  row "name"
  115- * 
  117- *  row "sid"
  134- *  row "player"
  170- */
  399- *  full name of the country
  415- * Series Id
  427- * Series Id
  430- */
  453- */
  485- *  All data in one array  
  

• ./includes/tournaments_series.pages.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
  

  11- * 
  

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

  ./tournaments_series.install ./includes/tournaments_series_helper.inc ./includes/tournaments_series.admin.inc ./includes/tournaments_series.pages.inc ./tournaments_series.info ./tournaments_series.module
  

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

PAReview.sh:
http://ventral.org/pareview/httpgitdrupalorgsandboxmarkoc1335722git

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

1. Please fill out the "TODO: Please describe this field" values in your module.install file.

2. hook_uninstall: no need to manually drop your tables, as this will be handled by the Schema API.

3. Deleting variables: I'd recommend performing a db query to find the actual existing variables, and then loop through them to unset, rather than the existing 1 to 99 loop.

4. 'tournament series' permission: Should contain a verb to make it clear what is granted via the permission (eg. 'administer tournament series info')

5. tournaments_series_block_contents: Block header values should be run through t() to make them translatable. Also in tournaments_series.admin.inc functions.

6. CRITICAL: tournaments_series_block_contents: Database output must be run through a sanitization function such as check_plain() or filter_xss() before being output. Also in tournaments_series.admin.inc functions, amongst others. Check EVERYWHERE you pull strings from the database.

7. $question = filter_xss(t('Are you sure')); Generally, we trust our translators. No need for filter_xss().

8. tournaments_series_add_tournaments_form: Default values for checkbox and radio form items must be run through a sanitization function such as check_plain or filter_xss().

Didn't finish reviewing the other files in the /includes folder, but apply the above rules, and you should be pretty good.

MarkoC,

That is only an issue if you are passing dynamic (i.e. user supplied) data into the function. Coder flags this as a 'potential' problem because the missing sanitization is a fairly common mistake with severe security consequences.

In your case, since you are defining the string itself inside code, the question can never by anything but 'Are you sure'; and thus there is no risk of a user injecting malicious code ... so as long as you don't have any user-defined $variables in the string, you can safely ignore the message as a false positive.

MANUAL REVIEW
---------------

1. The project page and README.txt file are lacking. What are the requirements for the module? Here are some great tips in how to improve the project page: http://drupal.org/node/997024 .

2. This is related to #1.....I installed the module, but I am not sure what to do at the point. You really can't assume people will know what to do, unless you include these instructions. A step by step process in how to setup a tournament after the module installed, would be very helpful.

I looked through the code, and didn't notice any major issues. If you can fix the above, I'll do some more testing of the module on my local server.

AUTO REVIEW
------------
Run the module against the coder with the "minor" option selected. There are only a few issues that came up, that should be easy to fix (ignore the filter_xss() warnings that were already discussed).

There were also a few additional errors the online PAReview tool brought up: http://ventral.org/pareview/httpgitdrupalorgsandboxmarkoc1335722

Hi,

First of: really nice module! I would like to use it for some LAN-party!

But I still have a few remarks:

• Try to avoid developers jargon: like Count in your Rules-page.
• Use the country_get_list() from locale so you always have an up to date country list & don't have to define it yourself.
• In your code you use db_query() to select from your db, in other lines you use Dynamic queries to select. This is not mandatory but would be nice if you would be consistent.
• I was to fast while following your guide on web-kod.com and added a tournament before any rules where defined. This gave me an warning when editing my tournament:
  

  Warning: Invalid argument supplied for foreach() in tournaments_series_check_plain_array() (line 525 of /tournaments_series/includes/tournaments_series_helper.inc).

  Try to use checks before using foreach-loops.

Very nice module, can't wait until it's a full project ! (I'm gonna use it nonetheless until it promotes ^^)

I'm very sorry for the delay here, as we do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732

Manual review of the 7.x-1.x branch:

1. "require_once drupal_get_path('module', 'tournaments_series') . '/includes/tournaments_series_helper.inc';": Are you sure that you need to load your helpers on every single page request? O r are that important API functions that you want to expose to other modules? Also, no need to use drupal_get_path() 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';
2. "'#default_value' => check_plain(variable_get('tournament_series_serie_no_' . $delta, '0')),": no need to use check_plain() here, the form API will sanitize #default_value for you. Please read http://drupal.org/node/28984 again.
3. "$blocks[$i]['info'] = t($_standings_blocks);": do not pass dynamic variables to t() where possible as this cannot be detected by translation extraction tools. Also elsewhere, e.g. in tournaments_series_standings().
4. "$standing[$key]['nation']['class'] = check_plain('nation-' . strtolower($row->nation));": no need to use check_plain() here, as the table theme function will use drupal_attributes() anyway. And double escaping is bad.
5. "return '<p>' . t('No data') . '</p>';": you can pass this as "empty" entry to the table theme function instead. Also elsewhere.
6. Your doing a lot of table listing stuff. I would strongly recommend a Views integration of your data, then users can build more flexible listings themselves.
7. I would also recommend using the Entity API to define the building blocks of your system, then you get a lot of integration with other modules for free. But that would basically mean a rewrite at this point, so this is just a tip for future considerations.
8. "$ranking_array[] = check_plain($row->player);", again, not necessary, as #default_value will get sanitized.
9. "'#description' => 'Each row is for each position.',": all user facing text must run through t() for translation. Also elsewhere, e.g. "'#value' => 'Save tournament',"
10. "$form['tournaments_series_id']": If a form has a submit function, then hidden form values are not needed. Instead, any values that you need to pass to $form_state['values'] can be declared in the $form array as #value.
11. tournaments_series_check_plain_array(): you could just use array_map() to sanitize all values of an array?
12. tournaments_series_helper_standings_sum(): please see http://drupal.org/node/1354#functions on how to document functions.
13. tournaments_series_helper_load_names(): do not select * if you are just interested in id and name.

No security blockers here, although you should definitively fix the issues I pointed out. Otherwise RTBC for me.

Your project page looks a little short, you may have a look at the tips for a great project page.

Code looks good and it seems like your up-to-the-task of being a good maintainer :)
(Your functions are well documented but making more inline comments would be a good practice)

Thanks for your contribution 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.