NewPoints 1.4 Review

[ZiNgA BuRgA]

This is a plugin "review" requested by Pirata Nervo.


This is quite a large plugin, so I will not go over everything.  In general, the code looks quite good - there doesn't appear to be any outright issues.  But as these reviews focus on negative aspects, I'll put up my concerns here.

Concerns

• On newpoints.php, the menu should, ideally, highlight the user's current location so that the user is aware of where they are.  Alternatively, a breadcrumb showing the user's location (which isn't present either).
• A number of templates on newpoints.php are uncached.
• As NewPoints appears to support multiple languages, language phrases shouldn't be hardcoded, as in this example in newpoints.php:
  
  PHP Code:

  $value = $setting['value']." Chars";

  ---

• I'm a bit concerned with how user input is handled, mainly with the plugin reporting back to the user if invalid input is supplied.  It appears to actually validate and sanitise the input well, however it's not always reported back to the user.
  
  When the user tries to donate points to another user (newpoints.php, under the do_donate action), we have the following piece of code:
  
  PHP Code:

  1 2 3 4 5 6

  // make sure the amount of points is not negative $amount = abs(floatval($mybb->input['amount'])); // do we have enough points? if ($amount <= 0 || $amount > floatval($mybb->user['newpoints'])) error($lang->newpoints_invalid_amount);

  ---

  The use of the abs() function is a little questionable.  Basically, if a user enters a negative amount to donate, the plugin will make it positive, and allow the input through (as long as you have enough points and it's not "-0").  I would ideally let the check which follows handle this invalid input, so that the user is notified that they entered an invalid input.
  
  Then we have the code:
  

  PHP Code:

  1 2 3 4 5 6 7 8

  // send points newpoints_addpoints($mybb->user['uid'], -($amount)); if ($uid > 0) newpoints_addpoints($uid, $amount); elseif ($username != "") newpoints_addpoints($username, $amount, 1, 1, true); else error();

  ---

  This handles what happens when the user enters a UID or username to donate to.  Unfortunately, no validation of either is performed, so if you happen to mistype a username or UID, you'll lose points, but no-one will gain them.  Ideally the system should check whether the user in question exists before attempting to donate to them, and telling the user if the user doesn't exist.  Also, if you fail to enter either, you'll get a generic error message rather than one explaining your problem.

• A small bug in the code:
  
  PHP Code:

  redirect($mybb->settings['bburl']."/newpoints.php", $lang->sprintf($lang->newpoints_donated, newpoints_format_points($amount))); $plugins->run_hooks("newpoints_do_donate_end");

  ---

  The redirect() function calls exit, so the plugin hook will never actually be executed.

• A small "bug": in a number of places, $db->query() is used instead of the more correct $db->write_query().  (yeah, not really an issue for a large majority of cases, and difficult to debug and identify without actually looking through code)
• The main plugin file, inc/plugins/newpoints.php is rather large (nearly 2000 lines) and is loaded on every page.  Probably not a big issue, but perhaps it could be modularised a bit more, so less code is loaded on every page load.
• Some of the functions shouldn't be declaring arguments as optional for example:
  
  PHP Code:

  1 2 3 4 5 6

  function newpoints_get_usergroup($gid = 0) { global $db; if (!$gid) return;

  ---

  PHP Code:

  1 2 3 4 5 6

  function newpoints_getrules($type = '', $id = 0) { global $db; if (!$type || !$id) return false;

  ---

  Not supplying a GID will cause the function to return null - thus making the parameters optional seems a little odd.

• Instances of $_SERVER['PHP_SELF'] should be replaced with $current_page for consistency reasons.
  

Note that, as mentioned above, I have only gone through some code, primarily newpoints.php and some of inc/plugins/newpoints.php

Commendations
An interesting feature is that there are a lot of plugin hooks spread throughout newpoints.php, which can make developing plugins for NewPoints relatively easy, which is a nice feature.

[Pirata Nervo]

Thank you
I have a question though, I can't check MyBB's code right now but what's the difference between $db->query and $db->write_query?

Not sure how I missed that redirect call ahah, that's why it's always good to have someone else to review the code.
I'll follow your suggestions and release 1.5 once I've done all modifications.

[ZiNgA BuRgA]

No worries - hope my suggestions helped.

$db->write_query is for multi-server setups - it sends the query to the master MySQL server, as opposed to the slaves.

[Pirata Nervo]

Oh alright, thank you

[Pirata Nervo]

I changed the way the menu works. In 1.4 it loads the menu() function from each plugin.
In 1.5 I switched to a run_hook_by_ref which passes the default $menu as an argument and can then be altered by any plugin.

I believe this is better than loading all plugins again (one to load the plugins and another one to load the menu), right?

[ZiNgA BuRgA]

Can't remember what code you're using, but if it's require_once, then, obviously, the files will be loaded once anyway.

[Pirata Nervo]

Oh yeah it is, there is no different then apart from going through a loop again and browsing the directory.