_privatemsg_assembly_query currently simply executes the requested hook in a more or less random order (it can be controlled by the weight field in the system table, but we would need to manage that by hand). However, in reality, there is always a "basic" (I can't find a better name) hook, which does the basic things like defining the table. Other modules which want to extend the query should be able to rely on the first one. This is for example important when submodules want to add joins on tables which are itself joined... so the order needs to be correctly set up.

I already needed to hack around that problem by using array_unshift in privatemsg_privatemsg_list_alter and having the issue again in pm_block_user (clean up as part of the api change, will post that later)

What about doing something like:

_privatemsg_assemble_query('privatemsg_somequery') -> privatemsg_somequery_sql() -> hook_privatemsg_somequery_sql_alter() -> generate and return query string.

Advantages:
a) other modules can rely on the fact that $fragments is basically set up and they can extend it
b) _privatemsg_assemble_query can check if the function privatemsg_somequery_sql() exists and report an error if not...
c) We would get rid of the doubly prefixed functions like privatemsg_privatemsg_somequery_alter()
d) As we are having more and more hooks (especially with the api), the added "_sql" suffix would help to distinguish between query alter hooks and message alter hooks.

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

CommentFileSizeAuthor
#9 privatemsg_querybuilder.patch8.06 KBnaheemsays
#7 privatemsg_querybuilder.patch7.74 KBnaheemsays
#4 privatemsg_querybuilder.patch7.91 KBnaheemsays
#3 privatemsg.improve_assemble_2.patch5.24 KBberdir
#2 privatemsg.improve_assemble.patch5.22 KBberdir

Comments

litwol’s picture

litwol commented

Sounds interesting however lets be careful not to make it too complicated, this already start to look and sound closer to what DB api does in drupal 7 :-p.

i am in favor of this if we have patch though, i already like to see technology being puched to limits of our abilities. that gives us better chance to learn something new. i greenlight this if you will provide a patch :).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new privatemsg.improve_assemble.patch5.22 KB

The idea was to make it simpler, not more complicate :). And I'm not intending to rewrite DBTNG, either. (We'll need to do this soon enough...)

Patch is attached. Seems to work fine...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
StatusFileSize
new privatemsg.improve_assemble_2.patch5.24 KB

re-rolled patch.

The current hooks look like this:

- hook_privatemsg_queryid_sql

I would like to have it like this:

- hook_privatemsg_sql_querid

So that all sql hooks are grouped similar to

- hook_privatemsg_message_xy
- hook_privatemsg_thread_xy (not yet here)

However, it would be good to if the "privatemsg" part would be configurabe too, so that it can be used by submodules. But we can't really use an additional argument for that because we are already using them as options for the query hooks. The only way I currently see to make this possible is to optionally allow an array as query id:

_privatemsg_assemble_query('list');
// would be the same as
_privatemsg_assemble_query(array('list', 'privatemsg'));

// A sub-module could then call for example:
_privatemsg_assemble_query(array('authors', 'privatemsg_filter'));

Thoughts ?

naheemsays’s picture

naheemsays commented
Status: Needs work » Needs review
StatusFileSize
new privatemsg_querybuilder.patch7.91 KB

I have updated the above patch to do things as you suggested and it works for me.

I have also added a couple of redundancies to the querybuilder which may not be necessary.

As for the array idea, I have done it both ways. by default it will be 'privatemsg', but a module can feed in a different module name.

The variable names may need to be improved though. Setting to CNR as I assume from your three patches this needs to go in first.

naheemsays’s picture

naheemsays commented

Since we (and by we I mean Birder :)) are working on the querybuilder, is this a good patch to kill the privatemsgapi.inc file in?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Patch looks good to me. Some minor things..

- $query_group or so might be a better name as $query_name, what do you think ? $query_name and $query_id sounds very similiar to me...

- Not sure if we need to introduce backward compatibility (your 'privatemsg_' check) at this point as it is not a stable version and there is afaik no "external" module already using the querybuilder function. So we still have "full control" over the callers...

Not sure if we should kill the privatemsgapi.inc file in this patch. I personally don't really like it, as it is very hard to follow the changes if the code is moved. A separate patch with only the really necessary code changes would imho be better. But I agree that either this and/or the move-patch should go in next..

naheemsays’s picture

naheemsays commented
StatusFileSize
new privatemsg_querybuilder.patch7.74 KB

Rerolled the patch renaming to $query_group (I still do not think either one is especially well named, but I have nothing against the suggested name.)

Yeah It could be a good idea to do the refactoring in one go after the current patches are in.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Looks good to me, can't really set it to RTBC since the initial patch was my own ;)

naheemsays’s picture

naheemsays commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new privatemsg_querybuilder.patch8.06 KB

I had missed out replacing a "privatemsg_load" with just "load" in the privatemsgapi.inc file.

Fixed and I will also set to rtbc because IMO it is ready.

litwol’s picture

litwol commented
Status: Reviewed & tested by the community » Fixed

Committed :)

Status: Fixed » Closed (fixed)

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