To reproduce:

1. Enable the "who's online" block.
2. Open up browser #1 (say, Firefox) and login as some user.
3. Open up browser #2 (say, Safari) and login as the same user.

The user's name will be listed twice. Since this block is referring to users and not sessions, each username should only be listed once, even if multiple sessions are present.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Status: Active » Needs review
FileSize
1.11 KB

This patch adds a DISTINCT which fixes the problem.

webchick’s picture

By the way, I'm using MySQL 5.0.19. I know I've seen this on other sites as well, but I'm not sure of the db versions running there.

RobRoy’s picture

Is there a greater underlying problem since there is more than one session per user? What change caused this bad behavior all of a sudden?

RobRoy’s picture

Ahh, nevermind. Just re-read the top post and it makes sense with multiple browsers. I'm an idiot. Patch looks good to me.

Dries’s picture

I can reproduce the problem. We should check with Robert -- he removed that distinct, I believe.

jacauc’s picture

Having the same problem with D5 HEAD. The DISTINCT is not there.
I added it, and now it's fine.

Thanks

Gerhard Killesreiter’s picture

Status: Needs review » Needs work

The proper fix is to skip the join on the sessions table and use the access value in the users table fo rthe time info.

profix898’s picture

Status: Needs work » Needs review

@Gerhard: Take a look at http://drupal.org/node/93042 for why there is a join ... only using the access value is not sufficient, because users that log out are removed from the session table, but their access timestamp is still in the interval, so they were listed.

RobRoy’s picture

Marked http://drupal.org/node/115112 a dupe of this.

RobRoy’s picture

Marked http://drupal.org/node/116929 a dupe of this.

RobRoy’s picture

Version: 5.x-dev » 6.x-dev

Another dupe: http://drupal.org/node/117278.

Where does this stand. Is adding a DISTINCT really not the way to go? Any SQL gurus get a patch up for that JOIN then?

Dries’s picture

Version: 6.x-dev » 5.x-dev
Status: Needs review » Reviewed & tested by the community

Committed to CVS HEAD. Needs to be backported to Drupal 5.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 5.

Anonymous’s picture

Status: Fixed » Closed (fixed)
siaiweb’s picture

Worked fine for me. Thanks !!

JHeffner’s picture

Status: Closed (fixed) » Active

This one popped up again for 5.2

Someone added s.timestamp into the select query as such..

$authenticated_users = db_query('SELECT DISTINCT u.uid, u.name, s.timestamp FROM {users}

removing s.timestamp solved the issue.

Ustas’s picture

Yes, JHeffner is right. Who fix this problem in CVS?

JHeffner’s picture

re-introduced by hunmonk at Revision 1.791 of user.module. CVS notes are

- Patch #148974 by hunmonk: fixed whois online block on PostgreSQL.

JHeffner’s picture

Introduced again on 5.3. Removing select timestamp fixed problem again. There must be multiple timestamps for users that aren't distinct. I noticed this only seems to happen with the admin user now though.

JirkaRybka’s picture

Possible duplicate: http://drupal.org/node/185202

drumm’s picture

Status: Active » Closed (duplicate)
drumm’s picture

Version: 5.x-dev » 6.x-dev
Status: Closed (duplicate) » Active

Actually, I will leave this one open since there is more information.

Selecting distinct on the session timestamp is not good since it will probably be different for different sessions with the same user. The timestamp used should be the one from the user column as Gerhard mentioned, but still do the join to make sure the session is still open.

Make sure to test Postgres before committing.

JHeffner’s picture

Affects new version 5.6. Any progress with getting this committed?

vladimir.dolgopolov’s picture

Yes, it would be nice to know.
I think removing select timestamp will work fine. Other solutions are not.

gdevlugt’s picture

As mentioned before, selecting the timestamp will result in different rows, thus rendering the added DISTINCT useless.

I tested removing the timestamp from the query agains both MySQL and PostgreSQL. Unfortunately PostgreSQL will give the following error :

* warning: pg_query(): Query failed: ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list in /var/www/httpdocs/drupal6-head/includes/database.pgsql.inc on line 138.

* user warning: query: SELECT DISTINCT u.uid, u.name FROM users u INNER JOIN sessions s ON u.uid = s.uid WHERE s.timestamp >= 1201459760 AND s.uid > 0 ORDER BY s.timestamp DESC in /var/www/httpdocs/drupal6-head/modules/user/user.module on line 758.

I made a patch for D6 head which solves the listing problem by keeping track of users added to the list in an array. It's definitely not an optimal solution imho because when the limit of number of users to display is set to a very large value, it will have some negative impact on performance. However, with or without this added code, having a large limit would result in negative performance anyway. I also removed the DISTINCT since it's not needed anymore.

If it could be solved by perhaps rewriting the query it would definitely be a better solution, but since I'm not that familiar with PostgreSQL, I don't know if it's possible at all.

catch’s picture

Status: Active » Needs work

gdevlugt, please provide patches in unified diff format, see http://drupal.org/patch

gdevlugt’s picture

Status: Needs work » Needs review
FileSize
1.49 KB

Hi,

Sorry, forgot the up switch. Here's an updated diff file attached.

vladimir.dolgopolov’s picture

I'd tested your patch - it works well.
I'd tried to find more fast solution, but in_array is the fastest one.

I am not familiar with PostgreSQL either, but maybe this "sql-rewriting" will work on it:

$authenticated_users = db_query(db_distinct_field('u', 'uid', 'SELECT u.uid, u.name FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.timestamp >= %d AND s.uid > 0 ORDER BY s.timestamp DESC', $interval));

This works for me on MySQL.
I really doubt it will work on PostgreSQL, but what if...

bfo’s picture

Happening with 5.6 running with postgres,

I'm not too bothered by it, but it would be nice to know that a fix for postgres is also being worked on.

vladimir.dolgopolov’s picture

No - db_distinct_field is not working here.
It adds "uid" in ORDER BY as first field.

vladimir.dolgopolov’s picture

@bfo:
What about patch from #27? It's not sqlify, but it clearly works.

I think there can be no suitable solutions:
http://www.tek-tips.com/viewthread.cfm?qid=1261181&page=1
"The best workaround is using sub-select"

Maybe it's time to write a distinct wrapper for Database abstraction layer like db_query_range() (called db_query_distinct).

vladimir.dolgopolov’s picture

The patch #27 has a authenticated_count mistake.
So this is a new patch.

catch’s picture

Is there a particular reason this couldn't be done with a subquery? D6 supports them now.

vladimir.dolgopolov’s picture

You are right - no reason at all:

http://drupal.org/requirements
Drupal requires MySQL 3.23.17 or higher. MySQL 4.1 or higher are strongly recommended. Drupal 6 will require MySQL 4.1 or higher.

But D5.x still hasn't subquery in MySQL before 4.1.

I'll try to make a patch with a subquery.
Is there any subqueries in core modues of Drupal6 ?

bfo’s picture

Please remember that drupal is also supposed to work with postgres it says 7.1 or higher so I would assume that a core fix should work for both DB's

catch’s picture

afaik (which isn't much) postgresql 7.1 supports subqueries without issues.

gdevlugt’s picture

I added a patch using a subquery. Please test since I only did a short test which worked on two D6 head installs using MySQL and PostgreSQL. In both cases, after patching the username was only listed once.

Alternatively, a subquery is 'too advanced', a group by on uid,name and aggregating (say using count) timestamp might also work, but the query itself will be less readable.

gdevlugt’s picture

Ignore last patch and use the one attached to this comment.... Forgot something in the query.

vladimir.dolgopolov’s picture

@gdevlugt
The patches #37 #38 are the same.
I think you forgot ORDER BY s.timestamp somewhere.

vladimir.dolgopolov’s picture

The attached patch is used db_distinct_field() function.
I'd tested it on PostgreSQL - works well. the last logged user appears on the top.

bfo’s picture

@bfo:
What about patch from #27? It's not sqlify, but it clearly works.

Will it work with drupal 5.6?

or is the problem only being fixed for drupal 6?

Thanks

gdevlugt’s picture

I couldn't get the patch at #40 to work under MySQL (5.0.41). I think it's because db_distinct_field() uses 'DISTINCT ON' which MySQL ignores. Perhaps someone else can verify this or has some more information on this?

As mentioned by vladimir in #39 i forgot to the ORDER BY s.timestamp DESC part in the subquery. I added it to the patch attached and hopefully it is now complete. Thanks for noticing it.

gdevlugt’s picture

#41 bfo: You're right, since this bug also exists under Drupal 5.x, it should of course also be fixed.

In a previous comment I mentioned that the problem could perhaps also be solved by a GROUP BY and use of an aggregrate function. Attached to this comment is a patch which uses an SQL query which seems to work, but naturally needs reviewing and testing. Basically, instead of selecting distinctly (which doesn't correctly work atm), the query selects the maximum timestamp, grouped by user (id and name). Perhaps the SQL seems a bit explicit (grouping on both id and name), it's because PostgreSQL, by the looks of it, is more strict than MySQL. I'm no SQL expert so again, review and test.

I also reworked the code from my previous patch for Drupal 6 head (using a php array). I will refrain from attaching it atm, since it might get confusing and handling it the SQL way is preferred. However, if the patch above doesn't work and won't work, I'll post the ported patch for Drupal 5 if you want.

Again the patch below is for Drupal 5 head only.

JHeffner’s picture

The SQL in Patch #42 works on Drupal 5.7 with MySQL 4.1 .. The SQL in patch #43 didn't work. It didn't return any results from the query.

cburschka’s picture

Can we review 6.x to get that in first, then move to 5.x?

I have successfully tested #42 on DRUPAL-6 (MySQL 5.0.18) by logging in with two browsers. Pre-patch shows two users, post-patch shows one. Perhaps we can do with another review, preferably with the minimum required version of MySQL 4.1.

JHeffner’s picture

We have been waiting over a year to get this patch into Drupal 5. The patch doesn't need to be the same for both, as long a patch does get into Drupal 5 for the version it was originally posted for.

cburschka’s picture

Yes, but still the issue is in the 6.x queue right now, and the usual policy is to commit to the newer branch first. I realize that the patch for D5 will be different and will look like #43, but I just wanted to suggest that we review #42 to get it into D6 before fixing #43 up for D5.

gdevlugt’s picture

Regarding the problem mentioned in #44 by JHefner, the patch attached to #43 only seems to work for later versions than 4.1. The problem is the 'ORDER BY max(s.timestamp)'. I tested it with 4.1 and indeed it didn't return any results and gave a warning. I found out that by aliasing max(s.timestamp), it does seem to work under at least MySQL 4.1 and 5.0.41 and PostgreSQL 7.4.19. At the moment I'm unable to test if it works under MySQL 3.23.17 but will test later tonight when I'm able to. Again, I added a patch for Drupal 5 head.

What would be interesting to know for Drupal 6 is which SQL query (GROUP BY using max() or the subquery) performs better and/or is more efficient. That is if both are equivalent in the results they give and correctly fix the problem of course. For Drupal 5 the subquery patch will work only if your MySQL version supports it, like 4.1 and 5.

Please test and review and post your results.

JHeffner’s picture

gdevlugt patch #48 seems to work on Drupal 5.7 with MySQL 4.1. Thank you so much for working on the patch.

zie86’s picture

I just fixed it!

JHeffner’s picture

Here is a perfect example of the problems with the Drupal community. Why does it take a year and a half to fix and still not merge a fix for a bug into the core for 5? Why do the technical staff have to keep a list of 30 different patches that need to be checked every time a new minor version is released? We need more developers to take the maintenance of the code seriously, and not just always looking forward to writing new code.

cburschka’s picture

The problem is that there are too few people testing patches. This could have been solved a year ago otherwise.

Edit: A 6.x patch existed already, but I didn't spot it among all the 5.x patches. Oops.

webchick’s picture

@JHeffner: report your testing results with postgres, and if the patch continues to work, mark it "reviewed and tested by the community" and it'll go in. Pretty simple stuff. While things are marked "code needs review", they still need, er, review. :P We're held up by someone testing in a pgsql environment (preferably both 7.x and 8.x). And looks like benchmarks wouldn't hurt either.

JHeffner’s picture

@webchick I don't run in a postgres environment so can only test the fix for multiple users under MySQL 4/5 which I reported above. This seems unfortunate that this bug is held up by such a small group of testers. It was a long morning of patches to walk through in the 5.8 release, so I apologize for the outburst. This bug has been present in almost every release of version 5. This probably is the wrong place to bring this up, but there is a serious need for better testers/developers to help maintain patches so they can get committed across all current releases. I wish I had the time to commit, but don't due to my day job.

webchick’s picture

there is a serious need for better testers/developers to help maintain patches so they can get committed across all current releases.

Hm. I don't think you understand.

The entire Drupal community (you, me, that other dude there) are the testers. It is our responsibility to test patches so that they can get to a committable state. It is not the core committers' job; they're there to commit patches once the community has reviewed them. And they're certainly not going to commit patches that have not been fully tested, since that's how this bug came about in the first place.

Since it's already costing you time to deal with this each release, it's a little illogical to me why you don't spend just a little bit more time to do what's necessary to get this into core, and instead are expecting others (who also have day jobs, and families they'd like to see once in awhile, etc.) to drop what they're doing and perform this testing for you. Scratch your own itch.

JHeffner’s picture

@webchick I don't think I was specific enough. The problem was originally introduced by by hunmonk at Revision 1.791 of user.module. The notes indicate this was a supposed fix for PostgreSQL. I took the time to track down all the information I could find to let the developers know the best detailed information I had. This patch was committed back to core without properly being tested. Shouldn't there be some way to either pull the change for further testing, or get other developers to test the change with postgres before commit? I understand this can be hard, look at this thread for instance.

I understand everyone has day jobs. Unfortunately we are now at this stand still. I tested the patch on MySQL 4.1, and MySQL 5 with success, but I do not run postgres nor have it installed on any servers. However the initial change introduced the problem in these environments and I have tested that the patch does work there.

I'm not trying to make any attacks, but rather trying to find a solution that would allow fixes not to get stalled completely. This may mean that I need to install a few development environments on postgres to test these patches for the community. I believed there were plenty of developers already running postgres that could test a small patch like this easily. I apologize, as I am wrong. I'll test this next week and get back to this thread. I'm familiar with MSSQL and MySQL 4/5, but not at all aware of the differences in PostgresSQL. Since the version number was changed to 6, do we need testing on 5, 6 and now 7 for every patch? or is it enough just to test patches on a single version, depending on it's complexity?

webchick’s picture

I believed there were plenty of developers already running postgres that could test a small patch like this easily. I apologize, as I am wrong.

Yeah, unfortunately that's been a big problem. :( The good news is that if you become one of those developers that does, you suddenly get a lot more leverage with your patch reviews. :D

Since the version number was changed to 6, do we need testing on 5, 6 and now 7 for every patch? or is it enough just to test patches on a single version, depending on it's complexity?

Generally, you test patches against the latest development version of Drupal (which would be 7.x-dev) and any supported version of an OS/DB/PHP/etc. I think it would be sufficient to test this against 7.x-dev and pgsql 8.x. If we can prove it works for the latest stuff, then the patch simply gets back-ported to previous versions of Drupal. Then someone who uses pgsql 7.x can come back and supply another patch if it doesn't work.

Ideally, this would be accompanied by a test in the Drupal 7 branch so that this bug never gets reintroduced again, but I'm not sure off the top of my head how to simulate two user sessions, other than manual DB manipulation. Which, actually, might not be the worst thing. Hmmm...

Anyway, for now, let's focus on making sure that it doesn't break pgsql. :) Thanks for your help!

cburschka’s picture

you suddenly get a lot more leverage with your patch reviews.

No kidding. You can go "behold, I can test your patches on PostgreSQL" and get developers to kiss your feet. ;)

catch’s picture

Version: 6.x-dev » 7.x-dev

I've added this issue to the List of issues which need a review on postgres.

#42 still applies to 7.x with a few lines offset, moving to there since we normally commit to the development branch first, then backport.

danielhonrade’s picture

This definitely works, thanks for this patch. I am using the latest drupal 6.4 and the bug still exists, then I tried this patch and boom, no more doubling of users.

danielhonrade’s picture

I mean #48 works for Drupal 6.4 release, thanks

obsidiandesign’s picture

Patch from #42 works correctly on d7/postgresql install. Online user is only displayed once, even when logged in through two browsers. I guess with a postgresql successful test, this issue is RTBC?

JHeffner’s picture

Tested on Drupal 5/postgres 7.4.19. Still haven't test on Drupal 6. To re-create the error log in on a different browser to the same site. Your username should appear twice. After applying the patch in #48 your username should appear only once.

errement’s picture

Hi all,

Have noticed same duplicate problem for the Who's Online Block, under Drupal 6.6.

User.module reads this on line 762:

$authenticated_users = db_query('SELECT DISTINCT u.uid, u.name, s.timestamp FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.timestamp >= %d AND s.uid > 0 ORDER BY s.timestamp DESC', $interval);

Anyone noticed same?

It has the SELECT DISTINCT

My system is:
Drupal 6.6
MySQL 5.0.51a
PHP 5.2.6
Apache 2.2.9 (Unix)

Thanks.

errement’s picture

To know for all too,

I am using "Drupal Administration Menu" module always, and it never duplicates any user session, at least for me. Have latest module version 6.x-1.1.

Cheers!

swentel’s picture

FileSize
1.36 KB

Updated for HEAD.

cburschka’s picture

Excuse me for asking again, but what was the concrete argument for grouping by name as well as uid?

uid is a primary key, so it is impossible for two records to have the same uid but different names, so there should not be any case where grouping by both uid and name can lead to different results than grouping only by uid. Enlighten me?

errement’s picture

@swentel,

Thanks for reply but is this for 5.x or 6.x? cos it says: whois-online-groupbymax2-5.x-dev.patch

Also checked in cvs head revision 1.932 and in cvs head revision 1.933 and on both, still says:
$authenticated_users = db_query('SELECT DISTINCT u.uid, u.name, s.timestamp FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.timestamp >= %d AND s.uid > 0 ORDER BY s.timestamp DESC', $interval);

Should it change? or maybe in near future?

Anyway i tried your fix, on my D6.6 and i confirm that this works for me:
$authenticated_users = db_query('SELECT u.uid, u.name, max(s.timestamp) AS max_timestamp FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.timestamp >= %d AND s.uid > 0 GROUP BY u.uid, u.name ORDER BY max_timestamp DESC', $interval);

But still allowing me to log-in under different browsers, same time. Shouldn't it say: Sorry, you are already looged in?!!

Thanks...
;)

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Needs review
Dave Reid’s picture

I would think we need to just group by uid. Tested and works as expected - no duplicates.

obsidiandesign’s picture

Patch in #71 does work for MySQL, but does NOT work for PGSQL. I too thought that the u.name isn't required, but here's what I get in PGSQL 8.2 when I enable the who's online block:

PDOException: SELECT u.uid, u.name, MAX(s.timestamp) AS max_timestamp FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.timestamp >= ? AND s.uid > 0 GROUP BY u.uid ORDER BY max_timestamp DESC - Array ( [0] => 1226959155 ) SQLSTATE[42803]: Grouping error: 7 ERROR: column "u.name" must appear in the GROUP BY clause or be used in an aggregate function in user_block() (line 823 of F:\Program Files\xampp\htdocs\drupal-7dev\modules\user\user.module).

Testing the patch in #66, both MySQL and PGSQL act appropriately. +1 for the patch in #66 being RTBC against the latest HEAD.

Dave Reid’s picture

Ah, thanks for testing that on PostgreSQL. I really need to get that up and running on my local machine. Quick re-roll of #66 just so I can feel cuddly about the testing results.

Dave Reid’s picture

Actually, why just "SELECT DISTINCT u.uid, u.name FROM users u INNER JOIN sessions s ON s.uid = u.uid WHERE s.timestamp > %d AND s.uid > 0 ORDER BY s.timestamp DESC" wouldn't work? The s.timestamp is only used in the SQL query and not elsewhere, so why do we need to select it? Tested on MySQL + HEAD and the block worked as expected with a duplicate session record. No duplicate results. Patches for D7-D5 since this bug should be backported.

cburschka’s picture

Thanks, so now I know that PgSQL requires non-aggregated columns to be grouped by even if they cannot by definition be ambiguous. Sounds less than sensible, but there you go...

Dave Reid’s picture

Ok disregard #74... there will be a problem with PostgreSQL (#148974: "who's online" block broken) since we can't do an order by on a field if it wasn't selected. Patch in #73 is the preferred patch.

Dave Reid’s picture

Ok...final patch revision I swear. I tested on both MySQL and PostgreSQL and confirmed that duplicates were removed from the who's online block. Also updated the D7 patch to use the proper DBTNG argument syntax.

obsidiandesign’s picture

Thumbs up here on both PostgreSQL and MySQL. RTBC?

Dave Reid’s picture

Maybe just get one more set of eyes to mark this as RTBC. I don't prefer to mark my own patches.

catch’s picture

Since this block isn't cached, it'd be nice to get some benchmarks with and without the patch. I've got a feeling MAX might be a bit quicker than distinct, but would be nice to verify. Looks very good though.

Dave Reid’s picture

Did a little performance testing. I generated 2000 users. Had 908 logged-in users records in {session} and 93 anonymous user records in {session}. There were 161 'duplicated' user sessions, for a total logged-in visitor count of 726.

Before patch in #77:
Who's online shows '908 users online and 93 guests' <-- BOO!

Requests per second:    6.55 [#/sec] (mean)
Time per request:       152.750 [ms] (mean)
Time per request:       152.750 [ms] (mean, across all concurrent requests)
Transfer rate:          43.14 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   142  152  12.3    149     258
Waiting:      142  151  12.1    149     258
Total:        142  152  12.3    149     258

After patch in #77:
Shows '726 users online and 93 guests' <-- YAY!

Requests per second:    6.65 [#/sec] (mean)
Time per request:       150.450 [ms] (mean)
Time per request:       150.450 [ms] (mean, across all concurrent requests)
Transfer rate:          43.80 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   141  149   8.3    148     215
Waiting:      141  149   8.1    147     215
Total:        141  149   8.3    148     215

Slight improvement after patch! :D

Dave Reid’s picture

I revised the performance testing results in #81. I had user sessions 'expiring' out of the threshold to being online. I put the user timestamps in the future and verified I got the same totals before and after each run. Still a slight improvement with the patch.

Dave Reid’s picture

And here's an even better and final revision.

Before patch:

Requests per second:    6.49 [#/sec] (mean)
Time per request:       153.992 [ms] (mean)
Time per request:       153.992 [ms] (mean, across all concurrent requests)
Transfer rate:          44.61 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   144  153  12.1    150     268
Waiting:      144  153  12.0    150     268
Total:        144  153  12.1    150     268

After patch:

Requests per second:    6.77 [#/sec] (mean)
Time per request:       147.813 [ms] (mean)
Time per request:       147.813 [ms] (mean, across all concurrent requests)
Transfer rate:          46.48 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   136  147  13.1    143     261
Waiting:      135  147  12.9    143     261
Total:        136  147  13.1    143     261
catch’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks lovely, tested the who's online block manually, benchmarks look good. RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

YAY!!! Committed to HEAD!!! Thank you SO much! :D

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Oops. Meant to mark it thus.

Damien Tournoud’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs work

I'm I dreaming or this went in D7 without a test?

webchick’s picture

Oh, SNAP! :)

You are exactly right; I was a bit over-excited to fix this issue from 2+ years ago. :P

But yes, let's get some tests in here since this has broken at least twice.

Dave Reid’s picture

The 6.x and 5.x ports are still valid and in #77. I'll get a test written for this, but I did manually test the crap out of this, so I'm very confident in it. :)

errement’s picture

I confirm what Dave Reid said for #77 on D6.x

Cheers!!
:)

catch’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Needs review

I've opened a new issue for the Who's Online block tests #336596: Tests for Who's Online block

So marking this as CNR for Drupal 6 so it can be reviewed and tested for fixing in there. Wouldn't want people on the development list complaining there's not enough attention given to stable release would we etc. etc.

hba’s picture

I can confirm that the patch in #77 works for Drupal 6.8.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Has a test for D7, that has been confirmed to pass on MySQL and PostgreSQL. I'm confident for D6.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Problem with the D6 patch is that you renamed "timestamp" to "max_timestamp" in an array which is passed for theming. This will break any theme which had something to do with "timestamp".

digemall’s picture

Do you refer to #77 patch ?

I've patched a D6.8 (mysql) and everything seems to work well...

Has anyone found theme problem with this patch ?

Damien Tournoud’s picture

Status: Needs work » Patch (to be ported)

I was under the impression that patch #77 and #83 were similar. This is clearly not the case. We need to backport #83.

jvieille’s picture

Version: 6.x-dev » 6.13

I am just wondering about that mulitple users showing on the "who's online" block

I found the solution, applying manually the #77 patch (because it was for a very ancient D6 version)

My question is:
This issue dates back January 2007, Drupal 5
We are September 2009, Drupla 6.13, not far from Drupal 7

Why the hell such an issue can perdure along years and versions?
Why was it strode over by a decades of Drupal core updates????

ambo’s picture

Hi there,

i have the same questions in mind as jvieille does. do we have to apply the patch or are my duplicated users in core's who's online block a sign for a new bug?

regards - andreas

tuffnatty’s picture

subscribing! The query with MAX(s.timestamp) AS max_timestamp works for me on D6, and I am patching every new version for more than a year. Do I have to?

jvieille’s picture

We missed yet another Drupal Core Release.
Look like this patch will have to be applied forever

jvieille’s picture

Now 6.15 - not yet committed!!!
Thius bug was already mentioned in D5.
What can be done for having that 2 years old bug removed before it contaminates the future D7?

tqvn2004’s picture

Subcribing...

Today (9/1/2010), this problem is still there in Drupal 6.15!!!

onehitter’s picture

This is still an issue on my drupal and i am running 6.15. Anybody know how to fix! ?

hba’s picture

onehitter: Just use the D6 patch in #77.

jvieille’s picture

This is >2 years revealed, 15 months solved bug
It dates from D5.1 apparently, before the official D6 release...
Can the patch be committed for the next Drupal core releases?

Damien Tournoud’s picture

Version: 6.13 » 6.x-dev

@jvieille: nothing happen magically. If you want to help, see the Contribute code handbook and port the patch from #83 to Drupal 6!

jvieille’s picture

I am not that expert...
Can somebody help get ride of this?

foxfabi’s picture

I'm using mysql and drupal 6.16. The online user's counter and user list block shows a user twice while they login from mobile to. What do you expect from a block saying "Who's online"? I'm expecting the amount of registred users on the site, not the amount of sessions opened by anonymous...

so i have changed this lines:

764c764
<           $authenticated_users = db_query('SELECT DISTINCT u.uid, u.name FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.timestamp >= %d AND s.uid > 0 ORDER BY s.timestamp DESC', $interval);
---
>           $authenticated_users = db_query('SELECT DISTINCT u.uid, u.name, s.timestamp FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.timestamp >= %d AND s.uid > 0 ORDER BY s.timestamp DESC', $interval);
770c770
<               $items[$account->id] = $account;
---
>               $items[] = $account;

removed s.timestamp in the query because it break DISTINCT: users can't login twice at the same time..
using $account->id as index in the userlist will remove duplicate usernames in the list of online users.

hope this help clean the bug.

tuffnatty’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.85 KB

Here's a port of #83 against Drupal 6.16. Please test and review.

jvieille’s picture

I already applied manually #77 patch on D6.16, replacing
$authenticated_users = db_query('SELECT DISTINCT u.uid, u.name, s.timestamp FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.timestamp >= %d AND s.uid > 0 ORDER BY s.timestamp DESC', $interval);
by
$authenticated_users = db_query('SELECT u.uid, u.name, MAX(s.timestamp) AS max_timestamp FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.timestamp >= %d AND s.uid > 0 GROUP BY u.uid, u.name ORDER BY max_timestamp DESC', $interval);

This seems to work well. #83 was for D7, so I didn"t look at it
What's more here?

tuffnatty’s picture

I guess #83 is better performance-wise (selecting only max_users and using a separate query to get authenticated user COUNT), that's why it's been applied to HEAD, and that's why we had to port it to D6.

foxfabi’s picture

#109 works for me..

davidbessler’s picture

#109 works for me too

jvieille’s picture

Is someone capable eventually considering submitting this for the next D6 core release!
(bug sumitted January 5, 2007 - 12:04)

tuffnatty’s picture

Attaching the appropriate tag.

jvieille’s picture

Any hope this will be fixed before the next D6 release?

tuffnatty’s picture

If anyone who cares changes status to RTBC - probably yes.

arhak’s picture

Issue tags: +Ancient

tag

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

#109 looks good on visual inspection.

Damien Tournoud’s picture

Issue tags: -Ancient

Removing useless tags.

arhak’s picture

@#120 I disapprove your attitude http://drupal.org/node/7881#comment-2983710

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

This seems to change the structure of the items passed to the theme. They'd have max_timestamp and not timestamp keys. That would break existing themes printing the last login dates.

zyxware’s picture

Status: Needs work » Needs review
FileSize
1.85 KB

I have changed the 'max_timestamp' alias to 'timestamp' and have tested it on a local installation and it seems to be working fine. The order by clause has been reverted to the original s.timestamp clause. Am attaching the modified patch here.

jvieille’s picture

Committed soon?

MaxF’s picture

Works fine on my drupal-6.19 on mysqli 5.1.49 with three og_domains.

jvieille’s picture

3 years and 8 months...
We probably have to abandon any hope to have this bug fixed in D6 without patching.
Already there in D5
Not sure about D7.
Maybe D8?

ss54’s picture

Hi, when I log in as admin, I can see Admin appearing twice. You say the patch works, I would be so grateful if you could kindly tell me how to install the patch? Do you install a patch just like installing a module? BTW, I use Drupal 6.17 many thanks

Sarmad

Status: Needs review » Needs work

The last submitted patch, user.module-whos-online-dupes-d6-2.patch, failed testing.

Anonymous’s picture

Which patch should i use for drupal 6.19 or what latest patch is working correctly?

crifi’s picture

Status: Needs work » Needs review
FileSize
2.16 KB

Creating D6 patch from #123 matching the drupal patch rules via cvs diff...

You can use this patch for drupal 6.19, it is for the lastest stable branch.

Status: Needs review » Needs work

The last submitted patch, user-multiple-user-listed-107051-130.patch, failed testing.

crifi’s picture

Status: Needs work » Needs review
crifi’s picture

I've queued the patch for re-testing, because it's still against the latest DRUPAL-6 branch and could correctly applied. Maybe the bot run into a problem...
Feel free to review the patch manually...

Status: Needs review » Needs work

The last submitted patch, user-multiple-user-listed-107051-130.patch, failed testing.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
crifi’s picture

Status: Needs review » Reviewed & tested by the community

I've tested this patch in my testsystem and production sites now. It's working and everything is fine. In #130 I only changed the format for DRUPAL-6 branch, so this patch was tested in #125. Please change the status back to "need review" if I'am wrong. Thanks!

dilanke.kumara’s picture

I tested the patch in my testing site.And it works fine.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Finally, thanks! Committed and pushed.

Status: Fixed » Closed (fixed)

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