Actions

Difference between revisions of "Developer Area/How to Review Code"

From Mahara Wiki

< Developer Area
(dump my thoughts)
 
m (Security: fix formatting)
Line 25: Line 25:
  
 
* Check for cross-site scripting problems:
 
* Check for cross-site scripting problems:
* watch out for <tt>|safe</tt> in templates
+
** watch out for <tt>|safe</tt> in templates
* templates should be escaped by default
+
** templates should be escaped by default
  
 
* Check for cross-site request forgeries
 
* Check for cross-site request forgeries
* forms should ideally use pieforms which has a built-in session key check
+
** 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
+
** other operations that change the state of the application need to check the session key before doing anything
  
 
* SQL injections
 
* SQL injections
* use placeholders in queries
+
** 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
+
** 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 =
 
= New upstream components =

Revision as of 18:15, 25 May 2011

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)