Peer reviewing
These are points to consider while peer-reviewing issues. They can also be applied when solving issues, to reduce the likelihood of problems during peer-review and integration.
The checklist
Syntax
To allow the community of Moodle developers to work together, conventions should be followed.
- the code is easy to understand and where it isn't, comments have been provided;
- variables are named correctly (all lower case, no camel-case, no underscores);
- functions are named correctly (all lower case, no camel-case, underscores allowed);
- PHP DocBlocks have been updated and adhere to coding style guide;
- the code doesn't use deprecated functions; and
- $_GET, $_POST, $_REQUEST, $_COOKIE, and $_SESSION are never used.
See the Coding style guide for details.
Whitespace
Unnecessary whitespace changes can cause conflicts when committing code.
Ensure that:
- there are no unnecessary blank lines in the new code;
- blank lines do not contain spaces;
- there are no unnecessary changes to whitespace in other areas on the file;
Output
Output needs to be controlled by renderers to achieve consistency and correct application of themes.
Ensure that:
- output renders are used to generate output strings, including HTML tags;
- HTML output is valid XHTML;
- no inline styles have been used in HTML output (everything has to be in CSS);
- CSS has been added to the appropriate CSS files (base, specific area, sometimes canvas); and
- the code doesn't use buffered output unless absolutely necessary.
feedback any notices (E_STRICT, etc) seen into the MDL.
Language
To achieve appropriate internationalisation of Moodle, language strings must be managed correctly.
Ensure that:
- new language strings are named correctly (all lower case, no camel-case, underscores are permissible in some cases);
- language strings are used instead of hard-coded strings for text output;
- language strings have not been removed or renamed in stable branches (permitted only in master); and
- AMOS commands have been specified when moving, copying, or deleting language strings.
Databases
DB calls are the greatest performance bottleneck in Moodle.
If there is SQL code you can test quickly then do so.
Ensure that:
- there are minimal DB calls (no excessive use of the DB); and
- the code uses SQL compatible with all the supported DB engines (check all selected fields appear in an 'order by' clause).
Testing
All code must be tested before integration. If testing instructions are insufficient, it's likely to cause more work down the track.
Ensure that:
- there are specific testing instructions that state how, as well as what, to test;
- new unit tests have been added when there is a change in functionality; and
- unit tests pass for related areas where changes have been made.
Security
The user community relies on Moodle being responsibly secure.
Ensure that:
- User login is checked where an identity is needed;
- Sesskey values are checked before all write actions where appropriate (some read actions as well); and
- Capabilities are checked where roles differ.
Documentation
Work does not stop when code is integrated.
Ensure that:
- Appropriate labels have been added when there has been a function change, particularly
- qa_test_required (significant functional change),
- docs_required (any functional change, usually paired with ui_change),
- dev_docs_required (any change to APIs, usually paired with api_chage),
- ui_change (any functional change, usually paired with docs_required, except ui_change remains permanetly), and
- api_change (any change to APIs that devs will need to know about, usually paired with dev_docs_required, except api_change remains permanetly).
Git
Ensure that:
- the commit message includes the tracker issue number and the component (ideal format is
MDL-xxxx Component Commit message); - the Git history is clean and the work has been rebased to minimal commits; and
- the original author of the work provided as a patch has been given credit within the commit (as author of in the commit message if changes were made).
Misc
Ensure that:
- the code seems to solve the described problem completely (or that further issues have been created to resolve remaining parts);
- the code makes sense in relation to the broader codebase (look at the whole function, not just the altered code); and
- the developer has searched for and fixed other areas that may also have been affected.
