Regbar

Hi nmudgal,

• You should use the Doxygen and comment formatting conventions in your module.
• You use the ereg() function. This function has been deprecated as of PHP 5.3.0. You should use preg_match(). See this page

Please don't post the output of automated review tools like pareview.sh inline in a comment, as it just clutters the issue. Better add it as .txt attachment instead.

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

manual review:

• regbar_install(): no need to set variables here, you can use default values with variable_get() anyway.
• "include 'Barcode.php';": do not globally include you library, only include if you are actually making use of it.
• theme_regbar(): do not create image markup yourself, use theme('image', ...) instead.
• "variable_get('height', 50);": all variables your module uses must be prefixed with your module name to avoid name collissions.
• regbar_admin_settings(): $barimage_path: do not validate the image path here, that belongs into a form validation function.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects. And please always do a manual review, not an automated review alone.

manual review:

• README.txt: useses the term "Barcode" instead of "Regbar" as application name.

Otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects. Assigning to tim.plunkett, as he might have time to finally review/approve your code.

As tim seems to be quite busy, I hope he don't mind if I take this over

Two days is "quite busy"? :(

Sorry @nmudgal, and thanks @patrickd. I'll get the next one ;)

@tim - for many applicants waiting in rtbc 2 days are longer then the whole process before ;)

Some minor stuff I found

1. There's still some space to enhance your project page, so please take a moment to make your project page follow tips for a great project page.

2. Make sure to put Barcode.php & arial.ttf (6) to put in modules folder.

   Is it my english or does this sentence not make much sense?

3. Are there related modules that put themselfs into the "Regbar" package? if not -> please remove it from the .info

4. function regbar_install() {
     // Set the variables.
     variable_set('regbar_type', 5);
   

   It's not the common practice to set the default values for variables on install (As far as I can see it's also not necessary as you already provide a default value in most/all variable_get().). You could also define() some constants that contain the default values, so you don't have to change the default values on all variable operations manually if you want to change one. But it's okay if you want to handle it that way.

5. When I try to install your module by drush and I don't meet the lib requirement, your message is shown but the link where I can download it wont be printed because it's printed into console. Maybe It's possible here to test whether drush is used and (in case) you can output the link in (link) instead.

6. warning: imagettfbbox(): Could not find/open font in /home/patrickd/www/drupal-6-test/sites/all/modules/regbar/regbar.module on line 190.
   warning: imagettftext(): Could not find/open font in /home/patrickd/www/drupal-6-test/sites/all/modules/regbar/regbar.module on line 193.
   warning: imagejpeg(): Unable to open 'sites/default/files/regbar/temp_admin.jpg' for writing: Permission denied in /home/patrickd/www/drupal-6-test/sites/all/modules/regbar/regbar.module on line 196.

   DIrectly after I installed it I clicked on my Icard and got a wosd, with the errors shown above.
   I know your install instructions say that I have to make sure that I have the right font and that I have to take a look at the status report - but anyway it would be better to catch these errors and provide some instructions as (I bet) most users won't read the install.txt. For example checking whether arial.ttf exists could be done just as easy as done with Barcode.php.
   On the permission issue: on my computer the regbar directory in files/ was created - but not with the right permissions, also this should be possible to catch quite easy.

7. The encoding type setting is saved - but the default value always stays the same, that's because you're getting the wrong:
   '#default_value' => variable_get('type', 5), -> '#default_value' => variable_get('regbar_type', 5),
8. It's nice to set a custom default image that easy, but you should also verify that it's really a jpg by code/extension.
9. You should put your module configuration from admin/regbar/settings to admin/settings/regbar/settings, as all the other modules do (In fact I had to look into your code to figure out what path it is^^). Also your menu titles should start with a capital letter.
10. I think a hook_help just saying $output = '<p>' . t("Generates ICards of registered users") . '</p>'; - is not very usefull, you should either provide more information or remove it
11. Is it right that the icard, - even if I select that it should use my username - generates the image everytime I visit my icard-page? You should try to provide some caching here as gd-operations are quite exhaustive
12. You got two spaces between => and variable_get very often
    '#default_value' => variable_get('regbar_barcode_center_y', 100),

After all it does a good job :D

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

Oops, forgot to fix