Hide Until Thanks Review

[ZiNgA BuRgA]

This "review" was requested by Walkman 5.0.


Firstly, sorry for the lateness of this - I have been very busy lately.


I have a few main concerns with this plugin:

• I'm rather worried about how this plugin loads quite a bit of stuff in the postbit hook - for every post on showthread which gets displayed to the user, it loads the language file twice, and performs a query; so if your default is set to 20 posts per page, every thread view will cause an additional 20 queries to be executed - note also that no optimisation is also done - that is, the queries will be executed even if a guest is viewing the thread (and the query won't get anything, since guests aren't allowed to "thank")
  A simple optimisation could be to check whether the user is a guest or the poster of the post, as well as determining whether the post itself has any hide tags, then perform a query.  The query can select for all pids which will be displayed and cache the result.  As for language loading, a simple check would be suffice, eg
  
  PHP Code:

  if(!$lang->thx_main) $lang->load('thx');

  ---

• I'm concerned that giving/deleting thanks doesn't check post keys.  Submitting through AJAX does check that the HTTP request method is POST, however, this is not the case when submitting without AJAX - in this case, a CSRF attack against the system is kinda trivial.
  

My other concerns and issues found are:

• A number of queries in the install/uninstall routine, and do_thank/del_thank routines should use $db->write_query instead of $db->query
• Although probably not an issue, it's probably a good idea to use escape_string on the template insertions in the _activate() function; same for the descriptions for settings
• The drop table statement in the uninstall is commented out - wanted to do something here?
• The plugin only hooks into postbit, so won't affect the preview post function
• A number of instances there's preg_replace("/\[hide](.*?)\[\/hide\]/si", - probably works, but the second "]" should be escaped
• In some instances, array indexes are not quoted, for example $mybb->usergroup[gid] - ideally, "gid" should have quotes around it to ensure that it doesn't refer to a defined constant (as opposed to a string constant)
• There's some potentially confusing code, for example, in the thx function, you have:
  
  PHP Code:

  $post['thanks'] = "<a id=\"a{$post['pid']}\" onclick=\"javascript: ThankYou.thx({$post['pid']}); return false; \" href=\"showthread.php?action=thank&tid={$post['tid']}&pid={$post['pid']}\"> <img src=\"{$theme['imgdir']}/postbit_thx.gif\" border=\"0\" alt=\"$lang->thx_main\" id=\"i{$post['pid']}\" /></a>";

  ---

  and then, later
  

  PHP Code:

  $post['thanks'] = "<a id=\"a{$post['pid']}\" onclick=\"javascript:return thx({$post['pid']});\" href=\"showthread.php?action=thank&tid={$post['tid']}&pid={$post['pid']}\"> <img src=\"{$mybb->settings['bburl']}/{$theme['imgdir']}/postbit_thx.gif\" border=\"0\" alt=\"$lang->thx_main\" title=\"$lang->thx_main\" id=\"i{$post['pid']}\" /></a>";

  ---

  The code is written in a way that if the first line is run, the second line will also run, overriding the previous line.  Actually, the second seems to be the accurate one, so the first one should perhaps be removed.

• Some function names, such as "do_recount" are perhaps a little too generic, and should be prefixed more, perhaps.  Although if it ends up conflicting with another plugin, is much a problem of the other plugin, ideally, one can avoid any such issues by not naming functions that way in the first place.
• Not entirely sure, but this query looks a little odd.
  
  PHP Code:

  1 2 3 4 5 6 7

  $query = $db->query(" SELECT uid, pid FROM ".TABLE_PREFIX."thx GROUP BY pid ORDER BY pid ASC LIMIT $start, $per_page ");

  ---

  I'm surprised MySQL allows it, as 'uid' isn't aggregated, neither is it being grouped by.  Regardless, what is returned in the "uid" field is unclear, if there are multiple uids attached to a pid.

• as the hide code is hooked through postbit, it won't work properly via Quick Edit
• Thanking the post, using the AJAX method, doesn't show the content immediately - the user needs to actually refresh the page to see the unhidden content (so the AJAX isn't terribly useful, and in fact, a bit confusing)
• Redirection without AJAX relies on $_SERVER['HTTP_REFERER'] to be set - which isn't always the case (using Opera's Open in new tab doesn't send it, for example)
• The image URL in the thanks_postbit_outline template is incorrect - should be "/images/postbit_thx.gif" not "/images/thx.gif"
  

I hope this post is useful to the plugin author and its users in some way.
Thanks for reading.

[Walkman 5.0]

wow thanskyou so much

i solved 9 problems

ammm any example of the unoptimisate path?


this is my firs advanced plugin

[ZiNgA BuRgA]

(06-29-2010 04:53 PM)Walkman 5.0 Wrote:  this is my firs advanced plugin

Wow, really?  Then congratulations.  My first advanced programs/scripts were horrible.  You did fairly well then

(06-29-2010 04:53 PM)Walkman 5.0 Wrote:  ammm any example of the unoptimisate path?

Do you mean my first point?
Try putting your call to build_thank after you've done the various checks there.
You have a nice $thx_cache variable actually, which you've used for caching username styles - perhaps, also try caching the results from the query as well.
If you look at showthread.php, MyBB uses the $pids variable to pull posts out, so you can change your query, replace WHERE th.pid='$pid' with WHERE th.$pids.
Do note that $pids is only set on linear display - threaded display doesn't use the variable, so you need to also double check that $pids is set before trying to build a cache.

Anyway, you can build the cache in the thx function, and then, in the build_thank function, check if the result has been cached.  If it has, use that instead of a query, if not, you'll have to fall back to a query.
That would be my suggestion on how to implement it.

Hope that helps.  If it's too confusing, feel free to ask, specifying which part you're having trouble with.

And thanks for your work in the plugin!

[arsham]

Hi Why AJAX, this plugin does not work?
http://mods.mybb.com/view/hide-until-thanks