[D6] Grouping question

I haven't tested the module yet, just pure code review.

Attached is a patch for mostly coding standards fixes.

The only real problem is the tpl.php files, they are full of PHP logic that should be in a template_preprocess function.

Using grouping-question-help.tpl.php is an interesting approach, but it makes it all untranslatable. Much of the text should be in the README.txt, and the rest should be condensed and printed out directly by hook_help.

The patch removes all author credit. That is given on the project page itself.

Thanks!

You are working wrong with branches in git. Please see the documentation about release naming conventions and creating a branch in git.

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

• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• ./grouping_question.classes.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
  

  61-   *  The group index
  75-   *  The group index
  

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

  
  theme/grouping-question-answering-form.tpl.php
  

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

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

I'm sorry!
Unfortunatelly also the 6.x-4.x branch has some issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxjohnennew1184556git-6...

I understand the sense of naming the branch 6.x-4.x but I don't know if this is allowed (?)

nah :D this isn't the kind of question you should ask dries ;)
you can wait until somebody clarifies this here, look for other applications that had the same issue or ask in IRC

the modules pareview.sh depends on are changed frequently, you have to update it and I think you will get the same results as at ventral.org
thank you

Hi, correct style is to have *one* \n character at each file's end. If you have this, you can safely ignore each warning as any reviewer will do.

There is no convention that forces you to start your major version id by "1". The release naming conventions are only about proper formatting of your branches and tags.

While I personally would not cut myself off from any chance to do my own version naming and look for a different idea (e.g. 6.x-1.0-v4 (...-v5); not sure if this would work but should), you may of course start with 6.x-4.0-alpha1 or similar. You should, however, not tag too early; tagging before you have full project status does not make any sense.

hth, dave

On the branch naming, there's nothing against matching your version numbering up with the quiz module, but it typically only makes sense when you are the maintainer of both projects and can ensure you are simultaneously releasing updates on both projects. Matching version strings will cause end users to assume they need to keep the module versions in synch, when this may not actually be the case. In most cases, the child module should generally work with any release with the same minor release prefix (i.e. work with any 6.x-4.x, instead of needing seperate releases for 6.x-4.1, 6.x-4.2, and 6.x-4.3).

What you will find is that it becomes more confusing for the end user (and more work for the maintainer) when you try to sync up version strings, as you start doing things like releasing new version with no code changes, just because the other module was forced to make a tweak due to a security vulnerability or other bug.

When you have different maintainers for the two projects, then the additional coordination needed to keep releases in synch becomes somewhat more troublesome than it's worth.

All that said, there are cases where projects are only supported with each other at the dot release level, and there isn't anything restricting you from doing this ... I'd just suggest you avoid it unless you have a specific reason that synchronizing releases is required.

Reviewed the 6.x version.

Only suggestion I'd have is to use the $form['#attached'] property to add your js when you complete the D7 version, instead of drupal_add_js ... otherwise, everything looks good!

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

• Drupal Code Sniffer has found some code style issues (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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• info file: "php = 5" this is not necessary as D7 already requires PHP 5.2.
• "cache_clear_all('autoload:', 'cache');": are you sure this is necessary in D7?
• grouping_question_schema(): It would be nice to have actual untranslated descriptions for the tables and the columns in there, and please do not define empty '' descriptions.
• class GroupingQuestion: "@param int $a": a class definition does not have any parameters?
• grouping_question.js: @file doc block is empty. Indentation should always be 2 spaces, same as for PHP files.
• grouping_question.theme.inc: @file block is empty. Please make sure that all @file blocks have descriptions in your module.
• I find it hard to check if your module is XSS safe from reviewing the code. Please insert <script>alert('XSS!');</script> into all form elements that you expose to users and check if you get any nasty javascript popups (If you do, you have a security problem).

But that are just minor issues.

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