Mollom has a bunch of functions that are only called certain places (not just admin stuff). Let's split them up and use the awesome 'file' => 'mollom.*.inc' now that it's available!

CommentFileSizeAuthor
#8 296063-split-incs.patch33.8 KBdave reid
#6 296063-mollom-admin-pages-inc-D6.patch29.12 KBdave reid
#5 296063-mollom-admin-pages-inc-D6.patch29.11 KBdave reid
#3 296063-splitting_mollom-3-d6.patch29.1 KBFreso
#2 296063-mollom-admin-inc-D6.patch15.26 KBdave reid

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Assigned: Freso » dave reid

Agreed. I will look into splitting at least an admin.inc out. Not sure if a pages.inc is needed.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Needs review
StatusFileSize
new 296063-mollom-admin-inc-D6.patch15.26 KB

Patch ready for review. The only other function able to be split out into the mollom.admin.inc file was _mollom_verify_keys().

Freso’s picture

Freso commented
Assigned: dave reid » Unassigned
StatusFileSize
new 296063-splitting_mollom-3-d6.patch29.1 KB

There are eight functions (a little more than 150 lines) that are possible to take out to a mollom.pages.inc, for the various mollom/* callbacks. I think that's enough reason to do this split as well.

Freso’s picture

Freso commented

And all (563) tests pass with the patch from #2.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 296063-mollom-admin-pages-inc-D6.patch29.11 KB

Fixed a very minor coding style change in mollom_report_contact() in mollom.pages.inc. Confirmed all tests pass and manually reviewed the CAPTCHAs work and the report pages work as well.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 296063-mollom-admin-pages-inc-D6.patch29.12 KB

Rerolled with coder patch landing.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Postponed

I don't think this is important. I'd prefer to keep everything in a single file for the time being. There is no real performance advantage to splitting things up, and it makes it more difficult to develop.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Assigned: Unassigned » dave reid
StatusFileSize
new 296063-split-incs.patch33.8 KB

Re-rolled to split out a few functions into mollom.inc that don't need to be loaded on every full bootstrap.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I'd really like this to be re-considered. Performance was the primary reason we can split modules into .admin.inc and .pages.inc in Drupal 6. There's no need to have the admin settings code (a fairly largish function) loaded on every single page request, and the same thing with all the report node/comment/mail code. After working with D6 for so long, having a larger module file is harder for me to scan. Any developer on an IDE with code completion shouldn't have any problems dealing with separate files. But that's still a personal preference. The main reason for this is its-the-way-Drupal-core-does-it. Looking forward to Drupal 7 and it's higher memory usage, splitting module files will be even more important.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Postponed » Needs review

I'd also very much like this to land before any work on the UI refresh as part of the any-forms patch. Having a mollom.admin.inc will make things a *lot* easier since there will be a lot more code added.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Fixed

Committed to CVS. Thanks Freso for helping!

Status: Fixed » Closed (fixed)

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