Developer Area/How to Review Code

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)