If you use the node access subsystem then you insert a DISTINCT in every node listing query. The project page should emphasize this!

Comments

chx’s picture

chx commented

I tried to review the hook_db_rewrite_sql implementation but that's nowhere to be found?

On second thought, it's only the node grants system in node_db_rewrite_sql that forces distinct on. I need to research this more :(

chx’s picture

chx commented

Oh, found it , it's not in HEAD , now that's very interesting... is that all needed? This implemenation would mean that you can't show the list of private messages with Views.

chx’s picture

chx commented

So I am trying to be a bit more coherent here: If you have a module that controls node access then the only way it can control other module's node types is the node_access table and if you do that then node_db_rewrite_sql will add that acursed DISTINCT. If you only control your own nodes then you can use hook_access and hook_db_rewrite_sql as this module does and then node_db_rewrite_sql won't even fire because you do not use node_access table.

chx’s picture

chx commented
Title: The module kills site performance » Node access issues?
Category: bug » task
Priority: Critical » Normal

I have checked up with Moshe and Merlin both and seems like this is going to be OK though you won't be able to list these nodes, ever w/ a standard node query.

I think privatemsg is going to be an extension of this lite module which adds folders, thread view and so on. For that to work, this rewrite_sql needs some way to switch itself off...

rszrama’s picture

rszrama commented

Thanks a lot for looking into this, chx. I was indeed wondering about unexpected issues from that _db_rewrite_sql, and preserving the possibility of Views integration was something I was trying to do. Failure. : P

I'll give it some thought, and I'll hold off on any sort of advanced features in this module as my goal really was just to keep it small and simple - a quick way for folks to get PMs without the full (and quite impressive nowadays) feature set of Privatemsg. I think I had a quick thought about a PM Plus module that could provide extra features on top of this, but then I remembered I don't have that kind of time. So if there's anything I need to change in this module to facilitate someone else doing that, let me know.

Also, sorry about the trouble w/ the latest code... I'm a little CVS ignorant, so that should've been posted in HEAD. A little miffed that is wasn't, and I'll try to figure it out at home. >_<

andypost’s picture

andypost
he/him
Primary language Russian
commented

hm... if dont use db_rewrite_sql pm-nodes can be found by search system. Is this really good idea to put messages in nodes? views 2 can works with any data...

rszrama’s picture

rszrama commented

My thought on making PMs nodes is that many other systems don't integrate w/ any random data type... but they do integrate with nodes. I'll have to test this more fully, but one example is adding an image field to your PM content type to facilitate sending images privately. This will be much easier to do with CCK + Image Field than with a custom data structure. I figure it won't take any more code to nail down the access issues than it would to handle a custom data structure and API, and it might take less. I'm happy to work through those issues to keep the doors open for using node based modules on PMs... like taxonomy for categorizing messages in your Inbox. : )

So... as chx mentioned, I think we'll need to tweak the implementation of hook_db_rewrite_sql() so that it restricts PMs from most places but perhaps allows a Views query to access the data.

gavri’s picture

gavri commented

maybe we can add some kind of global argument to pass to hook_db_rewrite_sql() that will act like a key/password that will let specific queries - like views queries to ignore the rewrite.

rszrama’s picture

rszrama commented
Status: Active » Fixed

I've reviewed the implementation of db_rewrite_sql() and don't see any issues, just limitations that would be good to work around in future versions. I don't see any show stoppers, though, so I'm going to move this to fixed for now and open up a related issue to address improvements in this function. See: #358016: Add Views support for recipient checking and modify hook_db_rewrite_sql() accordingly

Status: Fixed » Closed (fixed)

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