Skip to content

Review Standards

These review standards provide a name and description for common review tasks.

Usage

When reviewing a pull-request, you may consult this list for ideas/inspiration on things to check. If you find a problem or feel that some QA task remains to be done, then it can help to post a link to the relevant guideline. This practice allows newcomers to understand the critique, but it doesn't require you to write a long, bespoke blurb.

Standard codes

Each standard has a code name (e.g. r-explain). These make it easier to reference the standards when chatting with others about PR review.

Templates

You may conduct a structured review, checking each standard in turn. Doing this will be easier if you copy a template and paste it into your Github comment.

General standards

Explanation

Standard code: r-explain

Ensure the PR has an adequate explanation.

If you were a site-builder reading the PR-log/release-notes and drilled into this PR, would you understand the description? If you were debugging a problem and traced the change back to this PR, would you understand why the change was made?

It is strongly encouraged that PR's include URLs/hyperlinks for any explanatory material (when available) -- such as a Gitlab issue, JIRA issue, StackExchange question, related PR, or Mattermost chat. However, hyperlinks are not a substitute for a description. The PR should still have a description.

PR descriptions should generally follow the pull-request template, although this could be waived if another structure is more expressive.

Exception:

  • WIP PRs do not need a detailed explanation until they're ready for earnest review.
  • Genuine NFC PRs do not need a detailed explanation.

User impact

Standard code: r-user

If a user was comfortable using the old revision, would they upgrade and assimilate naturally and unthinkingly to the new revision? If not, has there been commensurate effort to provide a fair transition-path and communication?

Documentation

Standard code: r-doc

Some changes require adding or updating documentation. Consider the impact of this change on users, system administrators, and developers. Do they need additional instructions in order to reap the benefits of this change? If so, update documentation as necessary by making a corresponding PR on one of the guides.

Run it

Standard code: r-run

Use the code somehow. You don’t need to attack every imaginable scenario in every PR, but you should do something to try it out. Be proportionate.

Developer standards

Technical impact

Standard code: r-tech

  • Would the patch materially change the contract (signature/pre-condition/post-condition) for APIv3, a hook, a PHP function, a PHP class, a JS widget, or a CSS class?
  • Would you consider the changed element to be an officially supported contract? A de-facto important contract? An obscure internal detail?
  • How might the change affect other parts of civicrm-core? extensions? third-party integrations?
  • If it's hard to answer, look for inspiration:
    • Grep civicrm-core or universe to find out where the API/hook/function/etc is called. Consider how these might be affected.
    • Look at the Gotchas for a list of issues that have been mistakenly overlooked in past reviews.
  • If there is a foreseeable problem:
    • Is there a simple alternative?
    • Has there been commensurate effort to communicate change and provide a fair transition path?

Code quality

Standard code: r-code

Is it understandable? Does it follow common conventions? Does it fit in context? If it changes a difficult section of code -- does it tend to make that section better or worse?

Maintainability

Standard code: r-maint

Many changes should introduce some kind of automated test or protective measure to ensure maintainability. However, there can be tricky cost/benefit issues, and the author and reviewer must exercise balanced judgment.

Test results

Standard code: r-test

If the automated tests come back with any failures, look into why. Hopefully, Jenkins provides an itemized list of failures. If not, dig further into the "Console" output for PHP-fatals or build-failures.

Gotchas

Packaging

Standard code: rg-pkg

If the PR adds a new top-level file, new top-level folder, or novel file-type, consider whether "distmaker" will properly convey the file in *.zip/*.tar.gz builds.

If the PR removes a dangerous file, then common package handling may not be enough to remove the file. (This is particularly for Joomla users, but also true for with manual file management on other platforms.) Consider updating CRM_Utils_Check_Component_Security::checkFilesAreNotPresent.

Permissions

Standard code: rg-perm

If the PR changes the permissions model (by adding, removing, or repurposing a permission), are we sure that demo/test builds and existing installations will continue to work as expected?

Security

Standard code: rg-sec

If the PR passes data between different tiers (such as SQL/PHP/HTML/CLI), is this data escaped and validated correctly? Or would it be vulnerable to SQL-injections, cross-site scripting, or similar?

Settings

Standard code: rg-setting

If the PR adds or removes a setting, will existing deployments or build-scripts which reference the setting continue to work as expected?

Upgrades

Standard code: rg-upgrades

Some PRs change the database by modifying schema or inserting new data. Ensure new installations and upgraded installations will end up with consistent schema. (Extra: If it's a backport, take extra care to consider all upgrade paths.)

If the upgrade adds a column or index, use a helper function to insert it. (This makes the upgrade more robust against alternative upgrade paths, such as backports/cherry-picks.)

If the upgrade needs to populate or recompute data on a large table (such as civicrm_contact, civicrm_activity, or civicrm_mailing_event_queue), the upgrade screen could timeout. Consider applying the updates in batches.

If the upgrade inserts new strings, should they be generated in a multilingual-friendly way?

See also: Upgrade Reference

Hook signature

Standard code: rg-hook

Adding a new parameter to an existing hook may be syntactically safe, but is it semantically safe?