This module provides a content type for viewing chess games in PGN (Portable Game Notation) format. It is a convenience wrapper for the Ltpgn viewer by Lutz Tautenhahn ( http://www.lutanho.net/ ). For more details see here: http://drupal.org/sandbox/malcolmp/1426274

to checkout
git clone --branch 6.x-1.x malcolmp@git.drupal.org:sandbox/malcolmp/1426274.git ltpgn

example site: http://chess.popmalc.org.uk/pgngames

At the moment its D6, but I'll do D7 if anyone wants it.
Please can someone review, thanks.

Comments

phoenix’s picture

Status: Needs review » Needs work

Hi malcolmp

This could be a nice module for chess lovers.
- Could you add the git command to get your sandbox. This will make it easier for reviewers to quickly get your code:
git clone http://git.drupal.org/sandbox/malcolmp/1426274.git ltpgn
- You still have code in the master branch. Please empty the master branch. The only thing that you can put on the master branch is a README.txt referring people to the feature branches. Check it here.
- Use the t() function on all of your strings. This is necessary for multilingual sites. This way they'll be able to translate them.E.g.: I see this in your hook_menu:

'title' => 'Chess PGN Viewer Settings'

- I have the impression that you're not completely following the drupal coding standards. I'll check it with an automated script:
Here is the link if you want to run the script again to check your corrections: http://ventral.org/pareview/httpgitdrupalorgsandboxmalcolmp1426274git

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
Review of the 6.x-1.x branch:

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. Get a review bonus and we will come back to your application sooner.

<br />
Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards</p>
<p>sites/all/modules/pareview_temp/test_candidate/./ltpgn.admin.inc:<br />
 +19: [normal] The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.</p>
<p>Status Messages:<br />
 Coder found 1 projects, 1 files, 1 normal warnings, 0 warnings were flagged to be ignored</p>
<p>FILE: ...pareview/sites/all/modules/pareview_temp/test_candidate/ltpgn.admin.inc<br />
--------------------------------------------------------------------------------<br />
FOUND 52 ERROR(S) AFFECTING 44 LINE(S)<br />
--------------------------------------------------------------------------------<br />
   3 | ERROR | The second line in the file doc comment must be " * @file"<br />
   3 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
   4 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
   5 | ERROR | The last line in the file doc comment must be " */"<br />
   5 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
   8 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
   9 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
   9 | ERROR | Function doc comment must end on the line before the function<br />
     |       | definition<br />
  13 | ERROR | Inline comments must end in  full-stops, exclamation marks, or<br />
     |       | question marks<br />
  27 | ERROR | Inline comments must end in  full-stops, exclamation marks, or<br />
     |       | question marks<br />
  29 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  30 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  31 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  32 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  33 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  34 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  37 | ERROR | Inline comments must end in  full-stops, exclamation marks, or<br />
     |       | question marks<br />
  39 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  40 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  41 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  42 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  43 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  44 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  47 | ERROR | Inline comments must end in  full-stops, exclamation marks, or<br />
     |       | question marks<br />
  49 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  50 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  51 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  52 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  53 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  54 | ERROR | Array indentation error, expected 4 spaces but found 6<br />
  57 | ERROR | Inline comments must end in  full-stops, exclamation marks, or<br />
     |       | question marks<br />
  59 | ERROR | Array indentation error, expected 4 spaces but found 3<br />
  60 | ERROR | Array indentation error, expected 4 spaces but found 3<br />
  61 | ERROR | Array indentation error, expected 4 spaces but found 3<br />
  68 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
  69 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
  69 | ERROR | Function doc comment must end on the line before the function<br />
     |       | definition<br />
  73 | ERROR | Expected 0 spaces after opening bracket; 1 found<br />
  73 | ERROR | Expected 0 spaces before closing bracket; 1 found<br />
  75 | ERROR | Whitespace found at end of line<br />
  77 | ERROR | Expected 0 spaces after opening bracket; 1 found<br />
  77 | ERROR | Expected 0 spaces before closing bracket; 2 found<br />
  79 | ERROR | Whitespace found at end of line<br />
  81 | ERROR | Expected 0 spaces after opening bracket; 1 found<br />
  81 | ERROR | Expected 0 spaces before closing bracket; 2 found<br />
  83 | ERROR | Whitespace found at end of line<br />
  87 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
  88 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
  89 | ERROR | Expected 1 space(s) before asterisk; 0 found<br />
  89 | ERROR | Function doc comment must end on the line before the function<br />
     |       | definition<br />
  95 | ERROR | Inline comments must end in  full-stops, exclamation marks, or<br />
     |       | question marks<br />
 100 | ERROR | Whitespace found at end of line<br />
--------------------------------------------------------------------------------</p>
<p>FILE: ...-7-pareview/sites/all/modules/pareview_temp/test_candidate/ltpgn.module<br />
--------------------------------------------------------------------------------<br />
FOUND 93 ERROR(S) AND 13 WARNING(S) AFFECTING 81 LINE(S)<br />
--------------------------------------------------------------------------------<br />
   3 | ERROR   | The second line in the file doc comment must be " * @file"<br />
   3 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
   4 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
   5 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
   6 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
   7 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
   8 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
   9 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
  10 | ERROR   | The last line in the file doc comment must be " */"<br />
  10 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
  17 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
  18 | ERROR   | Function doc comment must end on the line before the function<br />
     |         | definition<br />
  22 | ERROR   | Line indented incorrectly; expected at least 2 spaces, found 0<br />
  22 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
  24 | ERROR   | Array indentation error, expected 4 spaces but found 3<br />
  25 | ERROR   | Array indentation error, expected 4 spaces but found 3<br />
  26 | ERROR   | Array indentation error, expected 4 spaces but found 3<br />
  27 | ERROR   | Array indentation error, expected 4 spaces but found 3<br />
  28 | ERROR   | Array indentation error, expected 4 spaces but found 3<br />
  29 | ERROR   | Array indentation error, expected 4 spaces but found 3<br />
  30 | ERROR   | Array indentation error, expected 4 spaces but found 3<br />
  34 | ERROR   | Inline comments must start with a capital letter<br />
  34 | ERROR   | There must be no blank line following an inline comment<br />
  37 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
  37 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
  37 | ERROR   | Whitespace found at end of line<br />
  38 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
  39 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
  39 | ERROR   | Function doc comment must end on the line before the function<br />
     |         | definition<br />
  51 | WARNING | A comma should follow the last multiline array item. Found:<br />
     |         | TRUE<br />
  52 | WARNING | A comma should follow the last multiline array item. Found: )<br />
  57 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
  67 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
  78 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
  85 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
  91 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
  92 | ERROR   | You must use "/**" style comments for a function comment<br />
  97 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 101 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 107 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 116 | ERROR   | Comment indentation error, expected only 1 spaces<br />
 116 | ERROR   | There must be no blank line following an inline comment<br />
 122 | ERROR   | String concat is not required here; use a single string<br />
     |         | instead<br />
 123 | ERROR   | No space before comment text; expected "// delete exiting file<br />
     |         | first" but found "//delete exiting file first"<br />
 123 | ERROR   | Inline comments must start with a capital letter<br />
 123 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 124 | ERROR   | Inline control structures are not allowed<br />
 126 | ERROR   | Inline comments must start with a capital letter<br />
 126 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 133 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
 133 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 133 | ERROR   | Whitespace found at end of line<br />
 134 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 135 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 136 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 142 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
 142 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 142 | ERROR   | Whitespace found at end of line<br />
 143 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 144 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 145 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 151 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
 151 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 151 | ERROR   | Whitespace found at end of line<br />
 152 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 153 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 154 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 155 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 158 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 167 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
 167 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 167 | ERROR   | Whitespace found at end of line<br />
 168 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 169 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 170 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 171 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 176 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 179 | WARNING | A comma should follow the last multiline array item. Found: 2<br />
 187 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
 187 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 188 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 192 | ERROR   | Array indentation error, expected 6 spaces but found 4<br />
 197 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 197 | ERROR   | There must be no blank line following an inline comment<br />
 199 | ERROR   | You must use "/**" style comments for a function comment<br />
 202 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 207 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 209 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 221 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
 221 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 222 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 223 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 223 | ERROR   | Function doc comment must end on the line before the function<br />
     |         | definition<br />
 230 | WARNING | Format should be "* Implements hook_foo()." or "Implements<br />
     |         | hook_foo_BAR_ID_bar() for xyz_bar()."<br />
 230 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 231 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 232 | ERROR   | Expected 1 space(s) before asterisk; 0 found<br />
 232 | ERROR   | Function doc comment must end on the line before the function<br />
     |         | definition<br />
 235 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br />
     |         | question marks<br />
 236 | ERROR   | Inline control structures are not allowed<br />
 247 | ERROR   | You must use "/**" style comments for a function comment<br />
 250 | ERROR   | Array indentation error, expected 4 spaces but found 20<br />
 251 | ERROR   | Array indentation error, expected 4 spaces but found 20<br />
 252 | ERROR   | Array indentation error, expected 4 spaces but found 20<br />
 253 | ERROR   | Array closing indentation error, expected 2 spaces but found<br />
     |         | 13<br />
 264 | ERROR   | You must use "/**" style comments for a function comment<br />
--------------------------------------------------------------------------------<br />

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

malcolmp’s picture

Issue summary: View changes

Added git command

malcolmp’s picture

Status: Needs work » Needs review

Thanks for the quick response. I didn't have coder module set to pick up all the issues, and I had not seen drupalcs at all. I think I have fixed all those issues now.

kongoji’s picture

Hi malcolmp,
Nice module, I like chess!
Here below some stuff I noticed during a manual review of your module:
1) Consider creating a .install file to delete variable ltpgn_settings on module uninstall
2) It's a very good thing that you use sites/all/libraries for your module, but wouldn't be more simple to use Libraries API for that?
In some websites there is the needing to have specific libraries for each domain, so sites/all/libraries sometimes could become sites/<domain_name>/libraries. Libraries API would manage this for you.
3) Can you please document why you have validation checks for 'Frame height must an integer greater than 50' , 'Frame width must be greater than 50' and 'Maximum file size must be greater than 10'? Are them chess application limits?

Greetz

kongoji’s picture

Status: Needs review » Needs work

Forgot to move the project to needs work status.

malcolmp’s picture

Status: Needs work » Needs review

Hi kongoji,
Thanks for taking the time to review my module.
1) I added a .install to delete variable ltpgn_setttings.
2) I thought the libraries API might be overkill, but it turned out to be easy, so I added that in. Also added it as a dependency and to installation instructions in README.txt
3) I thought there should be some validation on these fields. There is no special reason for these values, but if you had an IFrame as small as 50x50 you wouldn't be able to see the chess game, and if you had a file size of less than 10 then there wouldn't be many moves in the game! I guess its just finding some sensible minimum values. I added some documentation for this. Also added some descriptions on the form to explain what they are for.

Also reran the code review

thanks.

prashantgoel’s picture

Status: Needs review » Needs work

please visit http://ventral.org/pareview/httpgitdrupalorgsandboxmalcolmp1426274git for the list of errors being generated.

patrickd’s picture

Status: Needs work » Needs review

don't block deeper reviews because of minor issues found by automated reviews

crobinson’s picture

Status: Needs review » Needs work

1. README.txt has the wrong path for libraries (all/sites/libraries -> sites/all/libraries).

2. Please number or otherwise mark your instructions. It is hard to see these are separate steps.

3. The Drupal standard is not to include author names in code (@file comments in .install, etc.).

4. Whitespace formatting is inconsistent. Two lines in ltpgn.module (210-211), then one line (218, which is correct), then none (nothing between 229 and 230). Please review. When you do this, also correct the automated code review in http://drupal.org/node/1434388#comment-5843356.

5. You have implemented libraries_api, but not cleaned up lpgn_admin():

  // Check that the pgn viewer exists.
  global $base_path;
  $settings = ltpgn_settings();
  $root = $_SERVER['DOCUMENT_ROOT'];
  $viewer = $root . $base_path . $settings['viewer'];
  if (!file_exists($viewer)) {
    drupal_set_message(t('@viewer not found. You must install ltpgnviewer as per the installation instructions in the README file for this module to work', array('@viewer' => $viewer)), 'error');
  }

  $settings = ltpgn_settings();
  $frame_width = $settings['frame_width'];
  $frame_height = $settings['frame_height'];
  $max_size = $settings['max_size'];

In general, avoid depending on DOCUMENT_ROOT (use base_path()), and there's no need to call ltpgn_settings() twice.

6. Maybe it is because I am new to PGN but I did not understand the max_size setting. Please add a #description that explains what the limit will do if the file is out of range. Will it be blocked? Ignored? Deleted?

7. 'chess pgn game' is a confusing permission since you also have a 'chess pgn admin'. Consider an alternative like 'chess pgn edit'.

8. This is just a suggestion but please consider making the file_directory_path() . '/pgn' storage location configurable in your admin settings. Hard-coding this is bad because there might be a file or directory called 'pgn' on the site already, and you do not detect this.

Note to other reviewers: I am still learning Drupal's security standards. This module executes a third-party library in an IFRAME on the local site. Because it is same-domain the IFRAME can access the parent container. Does this need additional review/vetting?

ankitchauhan’s picture

manual review:

Major things you should fix:

  • ./pltpgn.module: all functions should be prefixed with your module/theme name to avoid name clashes. See
    http://drupal.org/node/318#naming
    function _ltpgn_add_update($node) {}
  • when I am creating the content, in default/files/pgn folder file isn't creating. There is a permission issue, its not writable.

regards

malcolmp’s picture

Hi everyone, thanks for the reviews.

crobinson

I have fixed points 1,2,3,4.

For point 5) I have removed the redundant call to ltpgn_settings(). base_path() only seems to give me the url of the viewer but I need DOCUMENT_ROOT in order to check the existence of a file. I wanted to put in a check that the viewer was correctly installed. I don't see any other way of doing this with the libraries api for D6. Have you any suggestions?

6)I added a #description

7)I changed chess pgn game to chess pgn user

8)I made the pgn storage location configurable.

Regarding the question about security, this is a good point. I guess there is not much I can do if I don't trust the author of the ltpgnviewer library, although it does occur to me that I can make it more secure by preprocessing the pgn file as it is loaded (to remove any html etc.) - so I will have a look at this.

ankitchauhan

I renamed _ltpgn_add_update() to ltpgn_add_update().

Please can you give me some more information regarding the permissions problem as I was unable to reproduce this error. Is default/files writable to the web server? Did default/files/pgn not get created or was it unwritable? did you get any error messages? I tried both as an admin and authenticated user on a fresh drupal 6 install and I could not get any problem. I added an extra check to the return code from file_check_directory() to make sure the pgn directory got created ok.

Thanks.

ankitchauhan’s picture

Hi

my default/files writable to the web server and default/files/pgn get created but it wasn't unwritable

malcolmp’s picture

That is very strange. default/files/pgn is created by file_check_directory() in the same way as other modules do. The chmod in there would have to be failing. If you use another module that creates a file, for example change the color scheme on the Garland theme, so that default/files/color is created does that work?

I made some changes to the error checking, so maybe you will get an error message out now. Please can you try again. Thanks.

Also I have added code to remove html tags from the .pgn file for security reasons, added an examples directory with a sample pgn file in, and mentioned this in README.txt.

malcolmp’s picture

Status: Needs work » Needs review

Setting this back to needs review, as I have made required changes, and I cannot reproduce the permissions issue.

crobinson’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +PAreview: security

I can't reproduce it either, and permissions are a frequent problem in Drupal. Maybe it was something server-related? In any event, the code changes look good to me. However, I'm adding the :security tag because I think I was supposed to do that last time around. I'm not sure anybody actually does anything with the flag, and stripping HTML is a very good idea, but at one point I had a message from Patrick that the security tag should be left even if an issue was fixed, for "statistical purposes."

misc’s picture

Status: Reviewed & tested by the community » Fixed

Suggestions:

Your module need the pgnviewer, it could be a godd thing to check for it on the install, you could use hook_requirements
Also it could be a great thing to add a make-file tom make it easier for the users to download the library.

Other than that:

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

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Added link to example site