Hi guys,

Thanks very much for the great work on this module, it is really very practical and user friendly.

I tried to spend a little bit of time to take a look at the code of the module and would like to let you know about an easy change that could improve module's performance:

It seems Localization client has an administration configuration page that is defined in l10n_client_menu, in l10n_client.module, line 16 with the path admin/config/regional/language/share, which calls back the form l10n_client_settings_form in l10n_client.module, line 527.

I think it would be recommended to move all the administration code in a separate file, called l10n_client.admin.inc, different from the module file (which loads on every page), which would slightly lower the memory footprint of the module.

This should be a very standard way of coding, organizing files and would be recommended by the Drupal developer community.

I would assume this could also apply to all other branches (6.x or 7.x).

I would greatly appreciate to have your feedback on this request, and if you could let me know if I overlooked or missed anything in module's implementation, or the Drupal API in general.
Feel free to let me know if you would have any questions, comments or concerns on any aspects of the discussed implementations, I would be glad to explain in more details.

Thanks very much to all, in advance, for your comments, feedback, and insight.
Cheers!

CommentFileSizeAuthor
#2 Patch for 6.x-1.x to move all related admin config settings forms handling to a separate file called l10n_client.admin.inc.12.13 KBwangqizhong
#2 Patch for 6.x-2.x to move all related admin config settings forms handling to a separate file called l10n_client.admin.inc.5.82 KBwangqizhong
#2 Patch for 7.x-1.x to move all related admin config settings forms handling to a separate file called l10n_client.admin.inc.5.89 KBwangqizhong

Comments

wangqizhong’s picture

wangqizhong commented
Assigned: Unassigned » wangqizhong

Hi guys,

I am interested in fixing this problem, so I will take the issue.
Changing assignment to: wangqizhong.

Please let me know if you would have any questions or comments, I would surely try replying as soon as possible.
Thanks!

wangqizhong’s picture

wangqizhong commented
Status: Active » Needs review
StatusFileSize
new Patch for 7.x-1.x to move all related admin config settings forms handling to a separate file called l10n_client.admin.inc.5.89 KB
new Patch for 6.x-2.x to move all related admin config settings forms handling to a separate file called l10n_client.admin.inc.5.82 KB
new Patch for 6.x-1.x to move all related admin config settings forms handling to a separate file called l10n_client.admin.inc.12.13 KB

Quick follow up on this ticket.

Please find attached to this comment three patch files against all existing versions:

These three patches have been tested and seem to work as expected.

For all these versions, I didn't do any other change but to declare the file callback for the form page with 'file' => 'l10n_client.admin.inc',, and then simply copy/pasted all the code related with the form and other functions used to a new file called l10n_client.admin.inc.

This change doesn't add any new feature or function to the module, however, it improves its code organization, readability, maintainability as well as its memory footprint.

Please let me know if you would have any questions, objections, comments, suggestions, recommendations or concerns on the patch files or any aspects discussed in this ticket, I would be glad to provide more information, explain in more details or re-roll the patches if necessary.
I would greatly appreciate some help from module maintainers and if any of you could take a bit of time to look into any of the attached patches (6.x-1.x, 6.x-2.x or 7.x-1.x) to give me your feedback/opinion on this ticket.
 
Any questions, feedback, testing, changes, recommendations would be highly appreciated.
Thanks to all in advance.

SebCorbin’s picture

SebCorbin commented
Version: 7.x-1.x-dev » 6.x-2.x-dev

First off, thanks A LOT for these patches, that is a huge time save for me :)

Committed on 7.x-1.x for now, reviewing the two others...

SebCorbin’s picture

SebCorbin commented
Status: Needs review » Fixed

And also committed to 6.x-2.x

(6.x-1.x is no longer supported, so I did not committed on it)

Thanks again!

Status: Fixed » Closed (fixed)

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