Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
OUGC Awards

Please note that this is pretty much a negative criticism post, rather than a balanced review as mentioned in this thread. Also be aware that stuff posted here may be highly subjective.
Please feel free to criticise this post, however.

Plugin Reviewed: OUGC Awards
Plugin Version: 1.0.2 (last updated 15th April 2012)
Plugin Author: Omar Gonzalez
Author Message
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,338
Joined: Jan 2008
Post: #1
OUGC Awards
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.

My Blog
04-19-2012 05:16 PM
Find all posts by this user Quote this message in a reply
Sama34 Offline
Senior Member
****
Posts: 476
Joined: May 2011
Post: #2
RE: OUGC Awards
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.

Support PM's will be ignored. Yipi
Plugins: Announcement Bars - Custom Reputation - Mark PM As Unread
04-22-2012 09:04 AM
Visit this user's website Find all posts by this user Quote this message in a reply
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,338
Joined: Jan 2008
Post: #3
RE: OUGC Awards
(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.


My Blog
(This post was last modified: 04-22-2012 08:35 PM by ZiNgA BuRgA.)
04-22-2012 08:34 PM
Find all posts by this user Quote this message in a reply
Sama34 Offline
Senior Member
****
Posts: 476
Joined: May 2011
Post: #4
RE: OUGC Awards
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.

Support PM's will be ignored. Yipi
Plugins: Announcement Bars - Custom Reputation - Mark PM As Unread
04-23-2012 01:30 PM
Visit this user's website Find all posts by this user Quote this message in a reply
ZiNgA BuRgA Offline
Fag
*******
Posts: 3,338
Joined: Jan 2008
Post: #5
RE: OUGC Awards
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 Tongue)

My Blog
04-23-2012 04:54 PM
Find all posts by this user Quote this message in a reply
Sama34 Offline
Senior Member
****
Posts: 476
Joined: May 2011
Post: #6
RE: OUGC Awards
Right, I actually don't know what static is, googling it right now...

Thank you very much.

Support PM's will be ignored. Yipi
Plugins: Announcement Bars - Custom Reputation - Mark PM As Unread
04-24-2012 01:59 PM
Visit this user's website Find all posts by this user Quote this message in a reply


Forum Jump: