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.