[D7] MyPuzzle Sudoku

Hi!

I installed you module, and it works correctly.
Only a few little things:
- When enable the module, first i must to go to admin page and setup something, because without this, the game didnt appear.
(there is some default value problem). After saving, everything appears and works correctly
- You should check you coding with pareview, it finds a few error.
- The pareview can't find it, but there is still a problem: you have a whitespace before <?php in the first row of the .module file.
It can cause a few serious problems on sites with a lot of installed modules. (I experienced it very often)

Hi there, just spotted a few minor things that i've listed below.

Manual Review

Could do with a configure = admin/settings/mypuzzle_sudoku in the .info file

Not too keen on the credit links attached to each sudoku, these could be optional and should definately include the rel="nofollow" tag.

You have hard coded 'font-size: 10px' in a style tag, this makes it a pain to override for themers.... maybe a tag instead, or give it a class so that it can be targeted.

Use the l() function to create links instead of writing the markup directly.

You have created 7 variables, might it be better to create just 1 and set '#tree' = TRUE using the FAPI to keep all the settings in one place.

line #176 t("VeryHard") missing space.

Minor problems

1. The config page is on the root level of the admin menu, it would be more user friendly to put it on a sublevel (under configuration for example)

2. You still have code standard problems. See: http://ventral.org/pareview/httpgitdrupalorgsandboxtomseidel1743022git

3. You should really add your project sandbox page on this issue by editing it... otherwise reviewers could pass this module as they cannot easily find the needed information.

4. On the project description it sais that I can show the sudokus on the theme files. Maybe you should explain better or remove that part.

Other Issues

5. mypuzzle_sudoku_getrndanchor().
If you want to get a random item of the array, why dont use the php function rand() ?
http://be2.php.net/manual/fr/function.rand.php

6. Line 23, 24, 160 and 161. There are texts not translated. Use t() for that.
Check also other strings shown to the users without translation.

The rest looks good to me.
I'm following this module, change those things and I set it as RTBC next week.

You can also apply to the review bonus to get a faster answer from administrators.

Regards,

Sergio

1. You forgot to add the random function... that produces a fatal error.

function mypuzzle_sudoku_getrndanchor(){
	return rand();
}

2. The variables are created but not deleted.
You should create an install file to delete those variables using variable_del().

3. There is a security issue here. this is vulnerable to XSS exploits. Variables are user provided input and need to be sanitized before printing into HTML. See http://drupal.org/node/28984 . Please also check the other variables used in this function and whether the field content has been sanitized already or not.
Putting this on the background value:
'></iframe><script>alert('hello');</script>
Shows an alert.

Hi Thomas,

It seems a lot issues have been addressed and I had no problems while testing, although I did notice a few issues -
1) Some of the code has over 80 characters per line. I don't think this is a strict requirement of Drupal but it will neaten up your module file.
2) There's a typo in your description (embeded rather than embedded).
3) I wasn't able to trigger any kind of scripting from user inputs, although non-numeric data can still be put into the cells and rendered if pasted in - perhaps someone with greater knowledge of XSS can verify if this a problem that can be exploited.

Thanks,
Tom

Hi Thomas,

I had no problems while testing your module, but did notice that PAReview did come back with a couple errors:

FILE: /var/www/drupal-7-pareview/pareview_temp/mypuzzle_sudoku.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
41 | ERROR | Case breaking statements must be followed by a single blank line
42 | ERROR | Line indented incorrectly; expected 4 spaces, found 6
--------------------------------------------------------------------------------

It would also be nice if the project link was included at the top of this page with the Issue Summary.

Regards,
Pete

Minor coding standard issues are surely no application blockers. Anything else?

mypuzzle_sudoku_menu()

Just needs to say Implements hook_menu()
you do not need to re comment parameters

mypuzzle_sudoku_block_view() $block not defined/initiated as array
Perhaps return array('content'=>$content); ?

mypuzzle_sudoku_block_view()

perhaps use some theaming and a template placing markup in there?

removed reference to themes

Hi Thomas,

I found your current code in the module really clean and easy to read.

But, in the README.txt file I think you should add a line under REQUIREMENTS section.
You can add as follows:-
-- REQUIREMENTS --
* Client machine should be able to access http://mypuzzle.org

As per my manual review of 'mypuzzle_sudoku' module, what it does is, it defines a drupal block and puts an iframe linked to
http://mypuzzle.org/sudoku/sudoku_plugin.php with various GET parameters set as configured from settings.

So, the whole functionality of this module is dependent on "Accessibility of the client machine/browser to mypuzzle.org site"

Manual review:

1. mypuzzle_sudoku.module:
- mypuzzle_sudoku_help(): what about formatting "mypuzzle.org" as a link:
return '<p>' . t("Includes a mypuzzle.org 9x9 Sudoku on your drupal site or blog.") . '</p>';
- mypuzzle_sudoku_help(): what about using t() for
$content .= "<div><a href='http://mypuzzle.org/sudoku/' rel='nofolllow'>Sudoku</a> by mypuzzle.org</div>";
- line 88:

  $bgcolor = variable_get('mypuzzle_sudoku_bgcolor', '#ffffff');
  if (!preg_match('/^[a-f0-9]{6}$/i', $bgcolor)) {
    $bgcolor = 'FFFFFF';
  }

If admin enters "AAA" as color he will see white background, as "AAA" is not passing the validation. What about using validation for the admin form: http://drupal.org/project/fapi_validation (see regexp).

I'm finding a warning in your code:

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

FILE: /var/www/drupal-7-pareview/pareview_temp/mypuzzle_sudoku.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
38 | WARNING | Code after RETURN statement cannot be executed
--------------------------------------------------------------------------------

mypuzzle_sudoku_block_view() is crazy! I'm not sure what you're setting up there, but try and refactor this into some simpler functions that make clear what you're calculating.

This and the notes above are fairly minor, RTBC from me.

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.

You should also use a theme function to generate the output on mypuzzle_sudoku_block_view() - this will allow users to override the block content to their liking.

Neither issue is a blocker though, so...

Thanks for your contribution, tomseidel!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Enterprise modules from the community for the community.