Skip to content

How to review a core pull request

When someone opens a pull request (aka "PR") on CiviCRM Core, it must be reviewed before we can merge it. Reviewing core PRs is a useful (and often much-needed) way of contributing to CiviCRM. You do not need any special access or merge rights. What you do need, is...

Video walk-through

Pick a PR

Look through the list of open PRs and choose one which meets all of the following criteria:

  • The automated tests should be completed and passed, as indicated by a green check-mark. When automated tests fail, a red X display instead and GitHub sends the PR submitter a notification of the failure. Don't review PRs that have failed tests. You can add status:success to your search to show only PRs which have passed. (Here is a direct link).

  • There should not be any git merge conflicts. GitHub displays this at the bottom of the page by saying either "This branch has no conflicts with the base branch" or "This branch has conflicts that must be resolved".

  • The title should not contain WIP which indicates "Work In Progress".

  • The comments should not contain any unresolved disagreements or unanswered questions.

If you're a beginning developer looking for easy PRs to review, you might have good luck by looking at ones with a low number of comments.

Claim the PR

Add a comment to the PR like "Reviewing this now" to let others know that you intend to submit a review.

Read about the issue

Many PRs will have an issue tracking ticket linked from the PR's page on GitHub. Read the original issue and understand how to reproduce the problem and what the solution looks like as well.

Read the code changes

On the PR, click over to “Files Changed” and understand what the code is doing.

  • Ensure the code is readable, and therefore maintainable by the next developer that has to work with it
  • Ensure it follows best practices. (TODO: what best practice?) (Note: basic code format standards are checked in the automated testing process.)
  • Consider whether any additional automated tests might be needed for this change. (TODO: how should I know?)

Reproduce the problem

Confirm which branch the PR was created against. This is probably either master or a Release Candidate branch. Setup an instance locally from that branch (e.g. with buildkit), or test on the public demo site if possible. Repeat the steps to reproduce described in the ticket or PR.

Confirm that the issue was a problem and a problem “worth solving”, generally worthy of being in core.

Validate the fix

Confirm that the PR works as advertised. Follow some mix of these steps:

  • Use the automatic demo-site
  • Review automatic test-results
  • Request additional test-results
  • Apply the patch locally

Use the automatic demo-site

Whenever a pull-request is submitted or updated, the test server will create a new demo-site. Civibot will post a link to the "Online demo of this PR".

What if there are no demo sites?

The demo may have expired. Demos are usually retained for a few days. But if there is a lot of activity, then it may only be a few hours.

Alternatively, the PR may have a bug that prevents installation.

In either case, proceed to the next step ("Review automatic test-results"). It will provide a console log and the option to retry.

Review automatic test-results

  1. In the PR timeline, find the latest commit.
  2. Find the status icon (orange dot, red X, or green checkmark) and click on it.
  3. Drill-down on the "Details".
  4. This page shows a list of automatic tasks -- such as building the demo, running low-level unit-tests, running end-to-end tests, and similar.
  5. Each task will have a few links available, such as:
    • Statistical summary: If the test-suite runs normally, it will provide a report of successes and failures.
    • Console output: Use this to monitor an active job or to identify fatal errors.
    • Retry: Run the task again. This is useful if the test-results have expired or if the test-server went offline.

Request additional test-results

By default, the automatic tests include one demo site and a variety of tests. However, deeper tests may be appropriate if:

  • The patch involves a CiviCRM integration (such as CiviCRM-Drupal or CiviCRM-WordPress).
  • The patch involves low-level code (such as the installer or the bootstrap process).

If you have permission to add labels, then you may request further tests with any of these labels:

Label Description
run-backdrop Create a demo-site with Backdrop. Run end-to-end tests on Backdrop.
run-drupal-7 Create a demo-site with Drupal 7. Run end-to-end tests on Drupal 7.
run-drupal-x Create a demo-site with Drupal 9/10. Run end-to-end tests on Drupal 9/10.
run-wordpress Create a demo-site with WordPress. Run end-to-end tests on WordPress.
run-extended-tests Run a comprehensive set of tests, including Backdrop, D7/D9/10, WordPress, and several versions of PHP-MySQL.
run-distmaker Create a set of zip-files/tar-files which include the patch. Use these to install locally.

Every label added will create more outputs and require additional time to complete.

Apply the patch locally

For more complicated PRs it is sometimes helpful or necessary to manually test them within a local development installation.

If the PR does not contain any database upgrades

(This is the most common situation)

Begin with a local development installation of CiviCRM master and apply the fix in the PR to your site.

An easy way to do this is:

  1. Install Hub
  2. cd to your civicrm root directory
  3. Run hub checkout https://github.com/civicrm/civicrm-core/pull/1234 where 1234 is the PR number you're reviewing

If the PR contains database upgrades

(This situation is less common)

  1. Install a buildkit site for the latest publicly available release of CiviCRM (not master). Pass the --civi-ver option to civibuild for this.
  2. Update the civicrm directory files so that the codebase has the changes in the PR (perhaps by using Hub as described above).
  3. From the civicrm directory, run ./bin/setup.sh -Dg to update the generated-code.
  4. Run drush civicrm-upgrade-db to perform database upgrades.

Write a review as a comment

  1. Evaluate the change against each of our review standards criteria.
  2. If you like, copy-paste one of the review templates into your comment and fill out the template.
    • If you choose not to use a template, then summarize your actions and findings, and recommend specific next steps (e.g. merging or otherwise).
  3. In your comment, tag one of the active contributors (e.g. @eileenmcnaughton) so they will see that the PR is ready for further action.