Closed (fixed)
Project:
Privatemsg
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
20 Jan 2011 at 20:29 UTC
Updated:
7 Mar 2011 at 17:41 UTC
Jump to comment: Most recent file
Comments
Comment #1
te-brian commentedAlright, this is definitely just the start. This should give you an idea what I'm going for.
There are probably other places we want to mimic this. Also, the list sql itself can probably have the "type IN ('user', 'hidden')" removed, but I haven't tested if that is true or not.
Also, poor pgsql needs to be accounted for.
Note: not marking 'needs review' yet because it will definitely not pass tests.
Comment #2
BenK commentedSubscribing
Comment #3
berdir(Tests) Maybe not, but let's try anyway.
We don't verify in the tests that roles and so on don't show up, so the tests will even pass. We should however update them now to verify that the roles show up :)
Comment #5
berdir*slaps head*
Yeah, the other issue, sure :)
Comment #6
te-brian commentedYep :)
That's a boatload of calls to privatemsg_user_load() (good thing it has static caching :)
Comment #7
berdir#1: 1034378_list_sql_concat.patch queued for re-testing.
Comment #8
berdirOk, here is a patch that works for PostgreSQL too.
Comment #9
te-brian commentedIt is a testament to the solid use of API functions that such a huge change to the output of a query passes all tests!
Comment #10
berdirWell, there was one change necessary, but that already happened in another issue :)
Commited, will need to be ported to 7.x.
Comment #11
berdirComment #12
berdirOk, attaching a patch for 7.x. This also includes the necessary changes for #1033338: privatemsg_user_load should handle array of user ids.
There was some cleaning up to do. Our own load wrapper function now only accepts an array in D7, all other calls for author or other user objects that do not require type have been switched to user_load(), as that already has a static cache in 7.x
Comment #14
berdir#12: 1034378_list_sql_concat_for_7.patch queued for re-testing.
Comment #16
berdir#12: 1034378_list_sql_concat_for_7.patch queued for re-testing.
Comment #17
berdirCommited.
Comment #18
andypostSuppose this code should be checked for compatibility with sqlite & pgsql
Comment #19
berdirPostgreSQL should work, sqlite not so much :)
The way it is currently solved is crap and slow anyway, it should be replaced with/in #647212: Load participants in thread list in a single but separate query but that is not trivial to implement.
Comment #20
berdirTurned out that it did not :)
I've run the tests on PostgreSQL and fixed two bugs, one because of this patch and another that already existed but was discovered by new tests.
Comment #21
berdirAnd now commited a fix for sqlite compatibility. That was easier than I thought, I just had to remove the "SEPARATOR ','" part, because although sqlite supports group_concat(), that part of the syntax is different. Both mysql and sqlite default to "," though, so that's fine.
Interestingly, the tests are rather fast with sqlite on tmpfs than compared to Postgresql (5 vs. 15min...)
Comment #22
andypostusing tmpfs for mysql testing env is preffered too but probably could lead to db inconsistency on rsync