Hi,

CivicActions is reviewing and upgrading multiple modules for use on client sites. Attached you will find patches based on the results from a "upgrade to Drupal 6" review and a coding / commenting standards review using the coder module.

An additional problem (not covered by the patches) was also found by the "interface text translatability" review provided by the potx module. The t() function is not intended for use on dynamic variables and really you should consider using the tt() function provided by the i18nstrings module.

sites/sony/modules/contrib-6--2/votingapi/votingapi.admin.inc:
 +39: The first parameter to t() should be a literal string. There should be no variables, concatenation, constants or other non-literal strings there.

Cheers,
Stella

CommentFileSizeAuthor
votingapi_upgradeD6.patch2.24 KBstella
votingapi_style.patch18.04 KBstella

Comments

eaton’s picture

eaton commented
Status: Needs review » Fixed

Thanks for the patches! They've been tested and applied. I'll take a look at the t($foo) issue; the code in question was pulled from devel_generate module, and only appears when it's in use, so I have the feeling that it's a relatively low risk.

stella’s picture

stella commented

The only other thing coder threw up was the use of db_query() instead of update_sql() in the install file, and the use of "alter table", etc, sql queries which should be done using the new D6 schema API, again in the install file. However, these seem to be old update functions copied over from the D5 branch, so I wouldn't be too fussed about fixing them - though maybe you should consider removing these functions entirely from that file since they're unneeded.

The t($foo) isn't a security risk I think (though don't quote me on that!) but rather those strings can't be extracted by the potx module and then translated. It's also not the correct usage of t(), hence the i18nstrings module.

Anonymous’s picture

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

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