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...
- GitHub Account
- A CiviCRM Development Environment (this might be optional, but good to have). One benefit is the ability to check out the PR in your environment.
- Comfort reading code and patches
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:successto 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
WIPwhich 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 the LTS. 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.
Reproduce the fix¶
Confirm that the PR works as advertised by observing the result in the build.
You can either test locally or on the test server.
Using the test server to review¶
Our test server automatically creates a dedicated CiviCRM installation for every PR so that (in most cases) it's easy to review the PR without needing to set up a local installation with the fix applied. To access the test build follow these steps:
- In the PR, find the section at the bottom of the page which says "All checks have passed"
- Go to: "Show all checks" > "Details" > "Console Output" > "Full Log"
- Search in page for
- The first result should bring you to a URL which points to an installation for the build of this PR.
- Click on the URL to go to the built site
- To get login credentials click on "test sites" link from Civibot's (Standard links) comment on the PR
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:
- Install Hub
hub checkout https://github.com/civicrm/civicrm-core/pull/1234where
1234is the PR number you're reviewing
If the PR contains database upgrades¶
(This situation is less common)
- Install a buildkit site for the latest publicly available release of CiviCRM (not
master). Pass the
--civi-veroption to civibuild for this.
- Update the
civicrmdirectory files so that the codebase has the changes in the PR (perhaps by using Hub as described above).
- From the
./bin/setup.sh -Dgto update the generated-code.
drush civicrm-upgrade-dbto perform database upgrades.
Write a review as a comment¶
- Evaluate the change against each of our review standards criteria.
- 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).
- In your comment, tag one of the active contributors (e.g.
@eileenmcnaughton) so they will see that the PR is ready for further action.