When someone submits a pull request to the project it is reviewed by a minimum of two developers.
What follows is the guidance developers should following when reviewing code.
When performing a review the following things should be considered:
To make it easier for us all to work together we need to ensure that we are working to a common set of standards, and that the way we are coding is as easy to follow as possible.
The projects coding standards are being met.
The code should be easy to understand, comments can and should be used to assist with this.
PHP docs have been updated and help with the understanding of the code
The commits are split logically (i.e. a single short line should be able to summarise the change in each commit)
Each commit message starts with a ticket number followed by a short 1 line summary of the change
Where changes are complicated there is a blank line followed by longer description
PHPdocs and comments are useful (Make sure that value is added, i.e. the function name is not just repeated)
Significant changes have had the appropriate specifications and developer documentation updated
Commit messages help explain changes
Where possible output is not mixed in with programme logic (this makes things easier to maintain) some projects will make this easier than others.
Output conforms to the project HTML standards.
Inline styles should not be used (external stylesheet files are better for maintainability)
Output should be in a form that can be localised
Language used will be understandable by end users
Minimal database calls (i.e. calls in loops are generally bad for performance)
SQL is written in a way that will work on all MySQL database engines supported by the project: innodb/ndb (and MyISAM for help tables)
User input is not directly added to SQL without any form of cleaning
Appropriate logins and capability checks are performed
No unnecessary personal data is collected
Personal data is stored and used in a way that is compliant with the General Data Protection Regulation (GDPR)
Database queries look efficient.
Expensive code is not on a critical path in the system and is not used on every page load (i.e would cause a page to be slow)
There is not excessive looping
Does the code look like it solves the problem it set out to do, without creating additional side effects (i.e. it does not make changes outside of the scope of the ticket)
The code makes sense when considering the whole system (for example if a single line in a method was changes look at the whole method to understand how it fits in)
Are there other parts of the code base that could have been affected by the issue?
The change is not mixing a re-factor and functional change (this is much more complicated to review)
That appropriate unit and functional tests have been added
That there are notes on the ticket to help the test team determine what the scope of testing should be