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.

[Imran]

Nice review of the plugin Yumi.

[flash.tato]

Thank you for your review.

I understood your plugin review and i think your points are right (i could've made a single query instead of n-queries and avoided some redudant code by writing a function

Quote:UserCP action should probably check/verify postkey like MyBB does

You're right i forgot to do, i think.

Quote: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.  [b]The merge post handler/b] may not always work (delayed moderation?)

Quote:$plugins->add_hook("moderation_start", "tagging_on_merge_posts");

Doesn't "tagging_on_merge_posts" do its job in merging posts? Maybe i don't understand what you mean that it is probably.

Ah and as last thing.

Thank you for your time, i think your points will be helpful for the future i like very much your analyzing abilities.

[Skiilz]

flash.tato is this working for MyBB 1.6?

[ZiNgA BuRgA]

Thanks

(11-23-2010 05:16 AM)flash.tato Wrote:  

Quote: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.  [b]The merge post handler/b] may not always work (delayed moderation?)

Quote:$plugins->add_hook("moderation_start", "tagging_on_merge_posts");

Doesn't "tagging_on_merge_posts" do its job in merging posts? Maybe i don't understand what you mean that it is probably.

It does, but there's a condition which forces it to only work on moderation.php.  So, for example, delayed moderation (done through tasks) won't work.  Moderation tools might have a bit of trouble too.
I blame the MyBB API for this >_>

[flash.tato]

Ah ok now i take the point.

Anyway i don't know if i will be gonna rewrite it but of course your analysing will make me adjusting my defects in coding (at least i hope ).

Yes i hope MyBB will rethink some of his "behind the scenes" for 2.0 i think it is a great occasion to solve some design problems (i mean MyBB was designed in an age where the needs were different, today those needs are changed and so it has to be re-designed).

[ZiNgA BuRgA]

I believe that was the reason why they're focusing on a rewrite, however, one big problem is their not wanting to change much once something has been developed.

[flash.tato]

(11-23-2010 06:06 PM)ZiNgA BuRgA Wrote:  I believe that was the reason why they're focusing on a rewrite, however, one big problem is their not wanting to change much once something has been developed.

If it is so, i don't understand why losing time in rewrite if you don't change what you developed.

[ZiNgA BuRgA]

Just their way of doing things.
If you go around their dev site, you'll see that they will often try to avoid changes.
They accept a lot of bad coding practices and generally don't fix them, such as

PHP Code:

1 2 3 4 5 6 7 8 9

switch($db->type) { case "pgsql": case "sqlite": $query = $db->simple_select("modtools", 'tid, name', "(','||forums||',' LIKE '%,$fid,%' OR ','||forums||',' LIKE '%,-1,%' OR forums='') AND type = 't'"); break; default: $query = $db->simple_select("modtools", 'tid, name', "(CONCAT(',',forums,',') LIKE '%,$fid,%' OR CONCAT(',',forums,',') LIKE '%,-1,%' OR forums='') AND type = 't'"); }

---

which you may find littered around the place.  The code goes against the whole purpose of a DB abstraction layer, but that's how things are.

Ultimately, it may not be a bad strategy.  The majority of forum admins don't understand good coding practices, and if you spend more time developing features and less on good code (as long as it is "good enough"), you get the attention.

[flash.tato]

Yeah! That depends mainly on the target of the product.

I think i learned that having a good product isn't enough, you've to find ways to manage it to its best.