Hi guys,

It seems Sharethis has an administration configuration page.

I think it would be recommended to move all the administration code in a separate file, called sharethis.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.

CommentFileSizeAuthor
#5 move_admin_config_form-1956802-5.patch26.76 KBpurushotam.rai
#3 sharethis-move-to-admin-inc-file.1956802.1.patch24.29 KBGaofengzzz
#1 sharethis-move-to-admin-inc-file.1956802.1.patch24.29 KBGaofengzzz

Comments

Gaofengzzz’s picture

Gaofengzzz commented
Version: 7.x-2.0 » 7.x-2.x-dev
Status: Active » Needs review
StatusFileSize
new Patch again sharethis-7.x-2.x to move admin config form page to admin.inc file.24.29 KB

Quick follow up on this task.

Please find attached to this comment a patch file against 7.x-2.x version, at 9fe4cde:
File named: sharethis-move-to-admin-inc-file.1956802.1.patch.

This patch has been tested and seems to work as expected.

For the 6.x-1.x branch and versions this change was already commited at 5c33ed4, refering to #432290: ShareThis jQuery Plugin.

For 7.x-2.x, I didn't do any other change but to declare the file callback for the form page with 'file' => 'sharethis.admin.inc',, and then simply copy/pasted all the code related with the form and other functions used to a new file called sharethis.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 file or any aspects discussed in this ticket, I would be glad to provide more information, explain in more details or re-roll the patch 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 patch (7.x-2.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.

Chaulky’s picture

Chaulky commented
Status: Needs review » Needs work 
+++ b/sharethis.admin.incundefined
@@ -0,0 +1,235 @@
+  // Load the css and js for our module's configuration.  ¶
+  drupal_add_css($my_path . '/ShareThisForm.css');
+  // This is ShareThis's common library - has a serviceList of all the objects

Trailing white space should be cleaned up.

Pretty straightforward patch, looks good except the trailing white space. I agree that it will help organization.

Gaofengzzz’s picture

Gaofengzzz commented
Status: Needs work » Needs review
StatusFileSize
new sharethis-move-to-admin-inc-file.1956802.1.patch24.29 KB

Thank you,
Has removed these trailing white space and upload again

Gaofengzzz’s picture

Gaofengzzz commented
Issue summary: View changes

patch it later

purushotam.rai’s picture

purushotam.rai commented
Issue summary: View changes
Status: Needs review » Needs work

Patch is outdated. Needs re-roll.

purushotam.rai’s picture

purushotam.rai commented
Status: Needs work » Patch (to be ported)
StatusFileSize
new move_admin_config_form-1956802-5.patch26.76 KB

  • purushotam.rai committed bf0b722 on 7.x-2.x 
    Issue #1956802 by gaofengzzz, purushotam.rai: Move admin config form...
purushotam.rai’s picture

purushotam.rai commented
Status: Patch (to be ported) » Fixed

Thanks all for your efforts.

purushotam.rai’s picture

purushotam.rai commented

Notice: Undefined index: nodeType in sharethis_configuration_form() (line 50 of /Applications/MAMP/htdocs/drupal/sites/all/modules/contrib/sharethis/sharethis.admin.inc).

Patch seems to produce error produced. Have created new issue to solve this: #2703259: Notice: Undefined index: nodeType in sharethis_configuration_form() line 50

Status: Fixed » Closed (fixed)

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