CivicActions is reviewing and upgrading multiple modules for use on client sites. Part of this work is a coding standards and i18n review. Attached you will find a patch based on a review with the coder module and a careful examination of the code.

The title and description of a hook_menu item don't need to be t()ed. More information can be found here: http://drupal.org/node/140311

Thanks!

CommentFileSizeAuthor
#9 nodecomment_st.patch1.16 KBstella
#7 nodecomment_i18n.patch1.74 KBstella
#4 nodecomment.patch980 byteswmostrey
nodecomment.patch616 byteswmostrey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented
Status: Active » Needs review
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Patch looks good and applies cleanly.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Code review » Fix incorrect calls to t()
Status: Reviewed & tested by the community » Needs work

Not quite, there's also an unnecessary call to t() in _node_comment_delete_thread() - watchdog descriptions aren't passed through t() either now.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented
Status: Needs work » Needs review
FileSize
nodecomment.patch980 bytes

Here's the updated patch.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

This looks great now.

sirkitree’s picture

sirkitree CreditAttribution: sirkitree commented
Version: 6.x-1.2-rc1 » 6.x-1.x-dev
Status: Reviewed & tested by the community » Fixed

I've taken rc1 and applied that to HEAD so it can be worked on from there. Haven't had a lot of time to spend on this, but I applied this patch to HEAD as well. Thanks.

stella’s picture

stella CreditAttribution: stella commented
Status: Fixed » Needs review
FileSize
nodecomment_i18n.patch1.74 KB

I'm not sure those changes are being included in the 6.x-1.x-dev tarball. The changes were committed to HEAD, but the tarball appears to be generated from DRUPAL-6--1 branch, where other changes have also been made since. I'm pretty sure these i18n changes aren't included.

I've re-rolled the patch, so it includes the above 2 changes and one additional internationalization issue.

Cheers,
Stella

sirkitree’s picture

sirkitree CreditAttribution: sirkitree commented
Status: Needs review » Fixed

small error in the patch:

-    'description' => t("Administer your site's comment settings."),
+    'description' =>("Administer your site's comment settings.",

should be:

-    'description' => t("Administer your site's comment settings."),
+    'description' => "Administer your site's comment settings.",

committed to DRUPAL-6--1 branch.
http://drupal.org/cvs?commit=144468

stella’s picture

stella CreditAttribution: stella commented
Status: Fixed » Needs review
FileSize
nodecomment_st.patch1.16 KB

That's great, thanks! However, I just noticed one other small error - one instance introduced by the patch and one that already existed. The t() function can't be used in install files as it may not always be available. Instead the get_t() function should be used to determine whether t() or st() should be used. The attached patch fixes this.

Cheers,
Stella

sirkitree’s picture

sirkitree CreditAttribution: sirkitree commented
Status: Needs review » Fixed

ah, interesting. good to know, thanks!

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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