OUGC Awards

[ZiNgA BuRgA]

This is a plugin "review" requested by Sama34.

Quite a fair bit of code, so I won't bother going through everything.

Plugin allows users to be awarded through the ModCP or AdminCP.  Awards displayed in profile/postbit.  An AdminCP interface for managing awards is also provided.
I'm somewhat intrigued by the 'proxy' inc/plugins/ougc_awards.php file - can't quite grasp the idea behind it - maybe putting all code into one folder?

Anyway...

awards.php

• The check on whether the plugin is installed is a little pointless, due to the fact that the language load call will fail if that's the case
  
  PHP Code:

  1 2 3 4 5 6 7 8

  ougc_awards_lang_load(); $awards = $cache->read('ougc_awards'); // If plugin no active or user has no useid then stop. if(!$mybb->user['uid'] || $mybb->settings['ougc_awards_power'] != 1 || !function_exists('ougc_awards_is_installed') || !is_array($awards) || !$awards) { error_no_permission(); }

  ---

  I suggest putting both that and the cache load after the check.

• In-loop query used here - consider using a JOIN in the query
  
  PHP Code:

  $users = $db->simple_select('ougc_awards_users', '*', "aid IN ({$award['aid']})", array('order_by' => 'date', 'order_dir' => 'desc')); while($gived = $db->fetch_array($users)) { $trow = alt_trow(); $user = get_user($gived['uid']);

  ---

inc/plugins/ougc_awards/plugin.php

• PHP Code:

  1 2 3 4 5 6 7 8 9

  $db->write_query("CREATE TABLE `".TABLE_PREFIX."ougc_awards_users` ( `gid` bigint(30) UNSIGNED NOT NULL AUTO_INCREMENT, `uid` bigint(30) NOT NULL DEFAULT '0', `aid` bigint(30) NOT NULL DEFAULT '0', `reason` varchar(255) NOT NULL DEFAULT '', `date` int(10) NOT NULL DEFAULT '0', PRIMARY KEY (`gid`) ) ENGINE=MyISAM;" );

  ---

  I'd consider putting a unique key over (uid,aid) as it seems that it should be.  Perhaps also consider sticking indexes on uid as well as aid.

• ougc_awards_lang_load is sometimes called multiple times on a single page load.  Perhaps consider restricting this to one call per page.
• Over-sanitisation used here (and possibly in other places):
  
  PHP Code:

  1 2 3 4 5 6 7 8

  if($mybb->input['reason']) { $mybb->input['reason'] = htmlspecialchars_uni($mybb->input['reason']); } if($mybb->request_method == 'post') { ... ougc_awards_give_award($award, $user['uid'], $mybb->input['reason']);

  ---

  (a htmlspecialchar'd version of the reason is being saved to the DB)

• PHP Code:

  1 2 3 4 5 6 7 8 9 10 11 12

  while($award = $db->fetch_array($q1)) { $q2 = $db->simple_select('ougc_awards_users', 'uid', "aid='{$award['aid']}'"); $users = array(); while($user = $db->fetch_array($q2)) { $users[] = intval($user['uid']); } $db->free_result($q2); $update = implode(',', $users); ougc_awards_update_award($award['aid'], array('users' => $update)); }

  ---

  ...noting that the users column that is being saved to is defined as varchar(100) - ie, a large amount of users will easily overflow the column.  I recommend rethinking this behaviour altogether - do you really want to potentially cache such a huge amount of data?
  

Some files (language files, possible others) are using the short opening tag (<?) as opposed to the full tag (<?php) - I suggest ensuring all files use the latter as short tags may be disabled.

[Sama34]

You are right, the idea bihind the "proxy" was to keep all into one folder. It was the first plugin I did so, now I'm not very sure about it since the plugins folders starts to have many subdirectories (I wrote others plugins the same way).

About the <? short tag, I suppose I should google more before implementing such crazy ideas -.-

The JOIN DB thing seems to be very handy.

And do you really thing it would bring more problems if I cache the users? I did so to void postbit queries at at ll.

Everybody seems to be afraid of plugins that make any extra query by post at postbit.

I also though about adding a user column to save users awards there instead, but to me it looks worse since I use others plugins that do something similar (newpoints, myachievements, etc..).

Thanks for the review.

[ZiNgA BuRgA]

(04-22-2012 09:04 AM)Sama34 Wrote:  And do you really thing it would bring more problems if I cache the users? I did so to void postbit queries at at ll.

Everybody seems to be afraid of plugins that make any extra query by post at postbit.

I also though about adding a user column to save users awards there instead, but to me it looks worse since I use others plugins that do something similar (newpoints, myachievements, etc..).

It's generally a bad idea to query for each post build, eg if there's 20 posts per page, a single query on postbit would add an extra 20 queries to the page load.  2 queries per postbit would be 40 etc.
That doesn't mean you can't query on postbit, you just need to ensure it only executes once.

Eg:

PHP Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23

function postbit_hook_function($post) { static $awards_cache = null; if(!isset($awards_cache[$post['uid']])) { global $pids, $db; isset($awards_cache) or $awards_cache = array(); $awards_cache[$post['uid']] = array(); if(THIS_SCRIPT == 'showthread.php' && $pids) { // cache awards here (I'm unsure of this query, cbf checking) $query = $db->query('SELECT DISTINCT a.* FROM '.TABLE_PREFIX.'posts p LEFT JOIN '.TABLE_PREFIX.'awards a ON p.uid=a.uid WHERE p.'.$pids); while($award = $db->fetch_array($query)) { $awards_cache[$award['uid']][$award['aid']] = $award; } $db->free_result($query); } else { $query = $db->simple_select('awards', '*', 'uid='.$post['uid']); $award = $db->fetch_array($query); $awards_cache[$post['uid']] = array($award['aid'] => $award); } } // awards now can be referenced through $awards_cache[$post['uid']] }

---

Or just cache on the users table, as one could assume that there won't be too many awards.  If you're afraid of conflicting with another plugin, just ensure that your column name is unique.

[Sama34]

I see, and what about caching only necessary data from the awards column?
Like, awards id, name, and users.

I do not show data from the awards users column in postbit, so caching that table does´t seem necessary at all.

[ZiNgA BuRgA]

Of course, you should adjust the code to suit what you need.  The code above is just an example to show you a concept.
(I typically don't give working or even the best code as it forces people to try to interpret it )

[Sama34]

Right, I actually don't know what static is, googling it right now...

Thank you very much.