Issue status update for http://drupal.org/node/21566 Project: Drupal Version: cvs Component: user system Category: bug reports Priority: minor Assigned to: deekayen Reported by: gjf Updated by: deekayen Status: patch Attachment: http://drupal.org/files/issues/user_access.returnbooleans_1.diff (763 bytes) I haven't found a case where trim() would be necessary if you explode on ', ' (with the space). I made a patch so things are hopefully less broken. I'm working on the benchmark people keep asking me for in #drupal. deekayen Previous comments: ------------------------------------------------------------------------ April 29, 2005 - 00:53 : gjf user_access() function returns a string not a boolean. This caused memory consumption problems for us due to a bug elsewhere in our code. i.e. return strstr($perm[$account->uid], "$string, "); should probably be if ( strstr($perm[$account->uid], "$string, ")) { return true; } else { return false; } ------------------------------------------------------------------------ June 20, 2005 - 02:35 : deekayen Attachment: http://drupal.org/files/issues/user_access.returnbooleans.diff (1.28 KB) Attached patch has a spelling correction and makes user_access() return only booleans. Diff'ed against HEAD. ------------------------------------------------------------------------ June 20, 2005 - 02:55 : deekayen Attachment: http://drupal.org/files/issues/user_access.returnbooleans_0.diff (1.59 KB) It bothered me to have a string search instead of an array search for permissions. This patch does what the previous patch does, but it uses in_array() on the static $perm instead of strstr() on a string. ------------------------------------------------------------------------ June 20, 2005 - 03:02 : killes@www.drop.org Can you please provide a benchmark test of the strstr approach vs the in_array one? I am concerned that it might be slower. user_access gets called quite often. ------------------------------------------------------------------------ June 20, 2005 - 03:47 : deekayen That's a fair request. The output of the following code on my WinXP, PHP5.1-dev, 2.133 Ghz Athlon is: 0.29749894142151 0.29712684249878 1.0804200172424 0.56814384460449 0.10924220085144 0.12113380432129 0.13284420967102 <?php function utime($a = '') { static $utime; $time = explode(' ', microtime()); $usec = (double)$time[0]; $sec = (double)$time[1]; switch($a) { case 'bm': $to_return = $usec + $sec - $utime; break; default: $utime = $usec + $sec; return $utime; break; } $utime = $usec + $sec; return $to_return; } $account->uid = 1; utime(); for($i=0; $i<500000; $i+=1) { if ($account->uid == 1) { // blah } } echo utime('bm') .'<br />'; for($i=0; $i<500000; $i+=1) { if ((int)$account->uid === 1) { // blah } } echo utime('bm') .'<br />'; for($i=0; $i<50; $i+=1) { $result[$i] = ''; for($j=0; $j>10; $j+=1) { $result[$i] .= chr(rand(65-126)); } } $perm[$account->uid] = ''; utime(); for($i=0; $i<2000; $i+=1) { foreach($result as $row) { $perm[$account->uid] .= "$row, "; } isset($perm[$account->uid]) && strstr($perm[$account->uid], "{$result['45']}, "); } echo utime('bm') .'<br />'; $perm[$account->uid] = array(); utime(); for($i=0; $i<2000; $i+=1) { foreach($result as $row) { $perm[$account->uid][] = $row; } isset($perm[$account->uid]) && in_array($result['45'], $perm[$account->uid]); } echo utime('bm') .'<br />'; for($i=0; $i<500000; $i+=1) { // blah } echo utime('bm') .'<br />'; for($i=0; $i<500000; $i++) { // blah } echo utime('bm') .'<br />'; for($i=0; $i<500000; $i = $i+1) { // blah } echo utime('bm') .'<br />'; ?> ------------------------------------------------------------------------ June 20, 2005 - 04:01 : deekayen Actually, there's a bug in the benchmark code that causes $result to not populate. Switch the following: for($j=0; $j>10; $j+=1) { to for($j=0; $j<10; $j+=1) { Then the string search is more like 8.1727991104126 seconds verses like 0.46953892707825 for the array search, an even bigger difference. ------------------------------------------------------------------------ June 21, 2005 - 12:45 : Dries Committed to HEAD. ------------------------------------------------------------------------ June 22, 2005 - 14:31 : killes@www.drop.org Please revert this patch, it isn't working at all. Drupal HEad is non-workign for all users which aren't No. 1. ------------------------------------------------------------------------ June 22, 2005 - 14:37 : moshe weitzman yeah - this breaks HEAD completely. here is a possible fix, if we still want to use in_array() for checking. this is in user_access() where we build the permission array: <?php while ($row = db_fetch_object($result)) { foreach (explode(',', $row->perm) as $p) { $perm[$account->uid][] = trim($p); } } ?>