Developer Area/How to Review Code

Here are the things to look for when reviewing code on the Mahara Gerrit. These are not absolute rules, they are guidelines.

Also see our commit / review policy.

Basic guidelines

1. If you're the first reviewer, you can +1 or +2 a commit.
2. 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.

Verification

The rest of the guidelines apply to the "code review" part of Gerrit. These ones are for the testing or "Verified" part:

• If you submit a change, you can mark it as verified unless you specifically want others to test it.
• Only mark as verified a change that you have tested yourself by checking out the right review branch (use the command line given by Gerrit).
• When appropriate, briefly describe what tests you did in your review. For example "tested fresh install on Postgres and MySQL, works fine"

Backward compatible code

Backward compatibility is usually encouraged in Mahara. This mostly affects the parts of Mahara where different pieces of code interact with each other (the "interfaces").

• function definitions (names, parameters, return values)
• class definitions
• expected Dwoo template variables (for templates that are called by many scripts, such as header.tpl)

Changing these sorts of things, can cause bugs in code that still expects the old interface to be there. This may be code from third-party plugins, or code that was missed while refactoring (perhaps because it's in a Dwoo template, or the function name is pieced together from multiple strings).

Don'ts

Things that are likely to break backwards compatibility, and cause non-upgraded code to throw ugly errors:

• Change the names of functions, classes, constants, Dwoo template variables
  • Workaround: Provide a "passthrough" function under the old name, that simply invokes the function under its new name (and maybe logs a debug message).
• Change the filenames of PHP scripts that are not "INTERNAL".
  • For example, when we changed view/artefact.php to artefact/artefact.php, it broke old hard-coded URLs that people had made, to view/artefact.php
  • Workaround: If you must rename a non-internal PHP script, leave a script under its old name that simply redirects to the new file's name.
• Change the order of existing function parameters
• Remove existing function parameters
  • e.g. replacing function foo($a, $b, $c) with function foo($a, $c)
  • You may have to leave a deprecated, non-used parameter in the parameter list. That's okay! Just document that it is deprecated (and perhaps change its name to $deprecated)
• Add new parameters in the middle of the list
  • e.g. replacing function foo($a, $b) with function foo($a, $a1, $b)
  • New parameters must always be added at the end of the list, even if that makes the ordering of the params ugly: function foo($a, $b, $a1 = null)
• Add a new parameter with no default value
  • e.g. replacing function foo($a, $b) with function foo($a, $b, $c)
  • Always provide a default value for new params: function foo($a, $b, $c=false)
• Change the data type of existing parameters
  • Certainly for PHP7 code with explicit data types on parameters
  • But also for implied data types. For example, if a function's implementation changes so that a parameter $user, which used to be an integer, is now expected to be an object, and you write code that does $user->id. This will throw an ugly error if old code tries to send $user as an integer.
  • Workaround: Add code that detects the data type of the value passed to the parameter, and converts it to the new data type. e.g. if (is_int($user)) { $user = new User($user); }
• Add new methods to a PHP interface
• Add new abstract methods to an abstract class

Do's

It's okay to break backwards compatibility in these cases:

1. Still in development: A new feature/function/class still in code review can of course be changed as much as you want. Or, if it has been pushed to "master", but hasn't yet been included in a release candidate or major release.

2. Major version upgrades: If you must break backwards compatibility on an existing stable feature, then do it on a major version upgrade. And add a note about it in the release notes.

3. Urgent security issues: A high-priority security issue may be worth breaking backwards compatibility outside of the normal release schedule.

4. Private / "internal" code: If a piece of code is clearly marked as being an internal implementation detail, then it's less risky to break BC because third-party code should not be using it.

Database

• Make sure there are no unnecessary DB queries being added
• Watch out for queries inside loops (do it outside the loop all at once in a single query)
• Table names in queries should be surrounded by {braces} so that prefixes will be applied if needed.
• The name length of a table, column, index, constraint, view, function or stored program including the $cfg->dbprefix must be less than 64 characters to make it compatible to MySQL 5.x
• SQL injections
  • use placeholders in queries, no exceptions
  • ideally, do the validation of the parameters close to where they're used so that it's obvious that it's validated
• When the database schema is changed, the same changes should be made in the install.xml and in the upgrade.php
• Version bumps on the stable branch should not use the current date, they should just add 1 to the version number.

Misc stuff

A few things which don't fit in the other categories but are important to keep in mind:

• Make sure loop variables are properly initialized at the start and/or each iteration of the loop.
  • In general be careful of re-using variables. Make sure to wipe the variable completely so that, e.g. data from one database record doesn't get inadvertently written onto another one.
• Watch out for create_function() inside a loop. These functions go in the global namespace and are never garbage collected, so they can easily cause out-of-memory errors.
• 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)
• Too much inline HTML? Please move it to a Dwoo template.
• JSON: new code should always use json_reply() rather than json_headers(), etc.
• HTTP headers: when writing code that assumes the presence of HTTP headers, think about cron running in CLI mode
• Slow operations should ideally be done in cron

Commit message

The commit message is part of what we review. This article about great git commit messages is a good start. Other things to watch for:

• Unless the commit is trivial (e.g. typo fix, minor coding guideline cleanup), it should have a bug on the tracker and the bug number should be included somewhere (doesn't have to be the summary line) in the commit message like this: "bug #123456" or "Bug #123456"
• If the bug is generic and relates to a blueprint, put in a tag to link it to that instead "Blueprint: blueprint-name"
• Click on the bug/blueprint number. Is it the right bug/blueprint?
• Think of the reviewer: your "commit message + the diff" needs to tell a story: why you changed the code in that way. The reviewer may need to go back to the bug on the tracker to see how to reproduce exactly the original problem, but your solution should be clearly explained in the commit message if it's not obvious from the code.
• When you fix a bug something introduced in a previous (recent-ish) commit, mention the commit hash in your commit message.
• If adding a new feature, briefly describe the feature, don't assume the reviewer will know all about the fancy Web 2.0 service you're adding a plugin for (at the very least in this case, you should include the URL of the service).

Langstrings / help files

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

Templates

• HTML element "style=" attributes in the templates are discouraged because it makes it impossible to theme these elements. Move this css stuff to the raw stylesheet instead.
• Check for cross-site scripting problems:
  • watch out for |safe in templates, it should only be there when the HTML is outputs is not coming from the user (or has been cleaned through Purifier)
  • templates should be escaped by default
• HTML comments should be avoided. Instead use a Dwoo comment like this: {* this is a comment that won't be in the final HTML output *}

Forms

• Check for cross-site request forgeries
  • forms should ideally use pieforms which has a built-in session key check
  • non-pieforms forms should include the sesskey and check it on submission
  • other operations that change the state of the application need to check the session key before doing anything

Javascript / CSS

• Javascript and CSS is forward-compatible
• Javascript has semicolons at the end of each line (for maximum compatibility)

New external libraries

• our patches are maintained but not mixed in with upstream code
• check that the library doesn't add new dependencies
• check if there are new copyright statements (which could lead to GPL-incompatible code being added to our codebase)
• make sure a README.Mahara exists for that external component. look at htdocs/lib/htmlpurifier/README.Mahara for a sample one
• do not change upstream code at all in the commit where you add it to Mahara. if you need to patch it to fix a bug, do that in a separate commit and make a note in its README.Mahara
• we shouldn't fix coding-guidelines related problems in external code. Delete the Jenkins review and link to the relevant console output in your review (e.g. https://jenkins.mahara.org/job/mahara/414/console)

Useful links

• Commit and revision policy
• How Mahara code revision works - presentation on Mahara UK 11 technical workshop.