Actions

Developer Area/How to Review Code

From Mahara Wiki

< Developer Area
Revision as of 19:15, 25 May 2011 by Francois (talk | contribs) (→‎Security: fix formatting)

Basic guidelines

  1. If you submit a change, you can mark as verified unless you specifically want others to test it.
  2. If you're the first reviewer, you can +1 or +2 a commit.
  3. If you're reviewing something that's already +1, you cannot just give it another +1. You must take the other review into consideration and give it a +2 or you directly invite another person to review and give a +1.

Misc stuff

  • Coding guidelines should be followed unless there's a good reason not to.
  • Curl: don't use directly, use htdocs/lib/web.php:mahara_http_request() instead
  • No magic numbers: use constants instead of hard-coded numbers as much as possible
  • No DIRECTORY_SEPARATOR: always use the UNIX path separator when opening files (it works on Windows NT too)

Langstrings / help files

  • Apart from messages to the logs, everything should be translatable
  • Check for grammar / typos

Scalability

  • Make sure there are no unnecessary DB queries being added
  • Very slow operations should ideally be done in cron

Security

  • Check for cross-site scripting problems:
    • watch out for |safe in templates
    • templates should be escaped by default
  • Check for cross-site request forgeries
    • forms should ideally use pieforms which has a built-in session key check
    • other operations that change the state of the application need to check the session key before doing anything
  • SQL injections
    • use placeholders in queries
    • ideally, do the validation of the parameters close to where they're used so that it's obvious that it's validated

New upstream components

  • our patches are maintained but not mixed in with upstream code
  • upstream doesn't add new dependencies
  • check if there are new copyright statements (which could lead to GPL-incompatible code being added to our codebase)