Alternate Label

Hey Mantish

Nice idea, I could think of some use-cases for this myself!

I found some general issues with your code regarding Drupal's coding standards and GIT standards:

• You are missing a README.txt file containing some general info about your module
• The comments in your file should be of format /** , * and to close */ like so:
  

  /**
   * @file
   * Some text here.
   */
  

• Also remember to end your comments with a full stop (.)
• Check your formatting for functions, don't start the opening bracket on a new line always do it like this
  function my_module_hook() {
  Note the space between the () and the {
• Try also to respect the 80 column marker for your code. If possible, lines of code should not exceed them.
• It seems that sometimes your indentation is off
• I notice that you don't have a correct GIT setup, there is no development branch (6.x-1.x) and your master branch should be clean, containing only a general README.txt
• I also noticed that you use one-line control structures in your code, its also better Drupal coding practice to write out the full structure.

You can always use the automated project reviewer to check your syntax.
I made a check for your project, you can review it here: http://ventral.org/pareview/httpgitdrupalorgsandboxmantish1655164git

Hey Mantish

No problem, we are all here to learn and contribute to the community. I'm also fairly new to coding to Drupal, and also had to review my code the first time to fit the Drupal standards.

I reviewed the code again on standards and found only a few small issues:

1. In your master branch you cleaned out everything but the README.txt. Thats good, but it should not be the same README.txt as your module. It should be a text file that tells the user to check out the correct branch.
2. Try to leave a new line between the end of your function and the beginning of the next one, or the beginning of the next block of comment.

Regarding your code I have one general comment about your syntax of db_query. Would it not be better to not try escaping the single quotes, but rather use the printf() notation? For example line 51 of your module file reads:

db_query('update {alternate_label} set alternate_text = \'' . $alternate_text . '\', status = \'1\' where field_name = \'' . $field_name . '\'');

This can be rewritten as

db_query("update {alternate_label} set alternate_text = %s, status = %b where field_name = %s", $alternate_text, 1, $field_name);

I did a test on a clean Drupal 6 installation running PostgreSQL and the module seems to work perfectly.
I can enable Alternate Label on a text field and it displays when viewing the node, but shows the normal label when editing.

Maybe it is also a good to work this inside the display modes of the node...so the user can choose on which type of display (full, teaser, rss, ...) the label is visible and on which type it is not? Maybe also allow for multiple labels for multiple types of display modes? Just an idea :)

Cheers
T

Hey Mantish

Good work, I can confirm that you updated the above issues.
I tested the module again and everything seems to work the same.

One very tiny detail: In your .install file, on line 19 you say it is an implementation of hook_uninstall, but the function below is actually hook_schema. And also try to be consistent in your comments, try always to write hook() instead of HOOK().

Thats it actually...if you update that, I think I can RTBC it!

I totally understand that the above feature requests will take more time. Maybe views integration is also another nice feature, that you can choose which of the alternate labels you want to show in the view...but I'll stop making feature requests now , lets do that once your module is fully accepted :)

Cheers
T

Hey Mantish!

Ok, I can confirm that the changes are made...looks good to me!

Cheers
T

We are currently quite busy with all the project applications and I can only review/approve projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

There is still a master branch, make sure to remove it. See also step 6 and 7 in http://drupal.org/node/1127732
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. You have to get a review bonus to get a review from me.

manual review:

1. "'length'=>255,": there should be a space before and after "=>".
2. "return($schema);": paranthesis are not necessary here? Also elsewhere.
3. alternate_label_form_alter(): if you are only targeting one specific form you should use hook_form_FORM_ID_alter().
4. "db_query("select * from {alternate_label} where field_name = '%s'", $field_name);": SQL keywords must be all upper case, see http://drupal.org/node/2497 . Also elsewhere.
5. alternate_label_submit(): use drupal_write_record() instead of the manual insert/update queries and you get schema validation for free.
6. notice: Trying to get property of non-object in alternate_label/alternate_label.module on line 48. Please enable PHP notices in your development environment.
7. alternate_label_preprocess_content_field(): this is vulnerable to XSS exploits. The alternate label is user provided input and needs to be sanitized before printing. See http://drupal.org/node/28984

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hey Mantish / Klausi

In my understanding it was optional to delete the master branch as described in step 7 of the tutorial.
If this was misleading information, I apologize Mantish.

Good luck with the module!

Cheers
T

Added the Other projects reviewed.

On really minor thing:

* In the .install: drupal_set_message($t('Installing Alternate Label module')) - there is an extra space between Label and module

This is RTBC for me.

sidenote: if your code is looking shitty because your trying to keep the line smaller then 80 cols - then f*** of that automated reports. (to clarify the 80 cols limit is actually ONLY for comments)

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.