Tagging Plugin v1.2.2 Review

[ZiNgA BuRgA]

This is a plugin "review" requested by flash.tato.

Overall, plugin is quite good (as somewhat expected by someone who cares about their coding).

Points I'd make:

• Write queries should use MyBB's $db->write_query() function.
• The plugin seems to support custom language files, but the author seems to have missed one part, and hard-coded it, eg
  
  HTML Code

  <legend><strong>Who can tag you?</strong></legend>

  ---

• PHP Code:

  1 2 3 4 5 6 7 8

  $setting_group = array( "gid" => "NULL", "title" => "Tagging Plugin! Settings", "name" => "tagging", "description" => "Manage every single aspect of the Tagging Plugin!", "disporder" => '4', "isdefault" => "0" );

  ---

  gid -> NULL is inherently incorrect as MyBB enquotes all data.  MySQL strict mode would generate a warning (but then again, MyBB won't run fine in strict mode).  That line can safely be removed.

• Possibly a bit of a design issue - the tagging plugin allows you to enter a username or uid.  Theoretically, it's possible to have a username which is also a valid uid, but they're both different users.  Probably not too likely, but nonetheless possible.  The issue here is that there is no way to determine what the referred user is.
• The function tagging_get_user queries the DB, and is called from within a loop (ie, an in-loop query).  It would be better to query for all users at once.  However, this is only done during post insert/update, so the effect of the issue is somewhat reduced.
• From above, the function tagging_can_tag_user is also called from within a loop; the function calls get_user, which potentially queries if user isn't cached before.
• Suggestion:
  
  PHP Code:

  preg_match_all("#@\[(.*?)\]#", $message, $matches, PREG_SET_ORDER);

  ---

  Usernames don't allow some charcters, eg "<", so it might be a bit smarter to exclude those from the regex

• Another suggestion:
  
  PHP Code:

  1 2 3 4 5 6

  $tags = array(); foreach($matches as $tag) { if(!in_array($tag[1], $tags)) $tags[] = $tag[1]; }

  ---

  It's probably quicker to use array indexes, since these are hashed, rather than use in_array(), which can be a bit slower, eg:
  

  PHP Code:

  1 2 3 4 5 6

  $tags = array(); foreach($matches as $tag) { $tags[$tag[1]] = 1; } $tags = array_keys($tags); // this line is only for compatibility with existing code

  ---

  Speed difference is negligible in most cases though.  There are some other places which could use a similar treatment.
  Actually, above code could be made more efficient simply by leaving preg_match_all() to use it's default ordering.  This is probably how I'd write the function:
  

  PHP Code:

  1 2 3 4 5 6 7 8

  function get_tags_from_message($message) { if(!preg_match_all("#@\[([^<>\"]*?)\]#", $message, $matches)) { return array(); } return array_unique($matches[1]); }

  ---

• The following type of code is used in a number of places - it might make more sense to export it into a separate function to reduce the amount of code duplication:
  
  PHP Code:

  $tags = unserialize($post['tags']); if(is_array($tags) && count($tags) > 0) foreach($tags as $tag) $prev = str_replace(array("@[{$tag['uid']}]", "@[{$tag['username']}]"), tagging_get_tag_text($tag['uid'], $tag['username']), $prev);

  ---

  Personally, I'd do something like this:
  

  PHP Code:

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

  function tagging_parse_tags(&$stags, &$msg) { if(!$stags) return; // I'd assume quite a number of posts don't have tags, so we can terminate early in most cases $tags = unserialize($stags); if(empty($tags) || !is_array($tags)) return; $replacements = array(); foreach($tags as $tag) { $replacements["@[{$tag['uid']}]"] = $tag['uid']; $replacements["@[{$tag['username']}]"] = $tag['username']; } return strtr($msg, $replacements); // single replacement call is probably faster than calling str_replace from within a loop }

  ---

  And then a number of functions can be simplified, eg:
  

  PHP Code:

  1 2 3 4 5 6 7 8 9 10

  function tagging_show_post($post) { $post['message'] = tagging_parse_tags($post['tags'], $post['message']); } function tagging_show_post_archive() { global $post; $post['message'] = tagging_parse_tags($post['tags'], $post['message']); }

  ---

  The function can probably be modified to handle some of the special cases like tagging_on_delete_post.

• I'm a bit concerned about the use of unserialize to reverse $db->escape_string:
  
  PHP Code:

  $tags = unserialize(stripslashes($posthandler->post_insert_data['tags']));

  ---

  It probably works well (assuming no Sybase style escaping) but it might be better to just assign the array to a global variable (if you don't like globals, you could assign to the posthandler) and use that, rather than go through this business of escaping and unescaping.

• Plugin doesn't seem to handle copy threads, split posts etc, but I'll say that handling all these would probably really be a pain.  The merge post handler may not always work (delayed moderation?)
• UserCP action should probably check/verify postkey like MyBB does
  

A lot of complexity of this plugin appears to be in handling the tag cache, which I'm glad is actually implemented (unlike the novice who just queries for everything on every page load).
One thing I really commend the author on - documentation!  For those of you who don't know, us developers hate documenting, but flash.tato has done a pretty nice job with his documentation there.