According to http://drupal.org/coding-standards, all functions should be prefixed with the module name and "internal" functions should begin with an underscore.

In HEAD, we currently have multiple prefixes and few functions without any prefixes
- disallow_anon_access
- pm_xy
- private_message_xy
- privatemsg_xy

(in privatemsgapi.inc there are two functions called _save_privatemsg_group_xy, but they are never used and should imho be removed anyway because they refer inexistent tables)

I'd suggest to use privatemsg_xy everywhere and add a _ prefix to internal functions.

Further steps/ideas about cleanup/"global refactoring"
- move internal functions to privatemsgapi.inc
- move all "menu-functions" into an own file, privatemsg.pages.inc, to save parsing time if they are not used. That's how core does it, too...

No patch yet, would like to get some feedback/other ideas first...

CommentFileSizeAuthor
#4 privatemsg_split.patch107.74 KBberdir

Comments

naheemsays’s picture

naheemsays commented

Is there a point in keeping the separate privatemsgapi.inc file?

Would it not be just as well to move all that stuff into the module file? it is always loaded, so there does not seem to be too much difference.

as for function naming, I would prefer privatemsg_ for public stuff, but I am not too bothered even with the current situation.

Having a privatemsg.pages.inc and a privatemsg.admin.inc would IMO be a good idea (even though the latter will currently be sparsely populated).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

There is one case when privatemsgapi.inc doesn't get loaded, when an guest visits a site with aggressive cache enabled.

However, I don't know how often that's used and moving them into one file would save two privatemsg_setup_includes() calls and the loading of an additional file. Not sure what's better in the long term.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Clean up function names/files » Clean up function names/files/aliases

We should also rename the aliases so that it's more clear what they are... max_timestamp/max_mid and so on...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new privatemsg_split.patch107.74 KB

Ok, here is a patch. Let's do that now since it's going to break a lot of patches anyway.

Patch is quite big but only moves stuff around except a few function renames (those that didn't used the privatemsg prefix like private_message_... or pm_...)

Tests still pass and clicking around does work too.

Anyone up for a short tests so that I can commit this?

litwol’s picture

litwol commented

omg. talk about fucking with revision history...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)

Commited, this needs to be done for 7.x-1.x too... Will probably be easier to just create a new patch, though..

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Patch (to be ported) » Fixed

This has now also been commited to 7.x-1.x-dev

Status: Fixed » Closed (fixed)

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