Community Reviews¶

Park-Manager is an open-source project driven by a large community. If you don’t feel ready to contribute code or patches, reviewing issues and pull requests (PRs) can be a great start to get involved and give back. In fact, people who “triage” issues are the backbone to Park-Manager’s success!

Why Reviewing Is Important¶

Community reviews are essential for the development of the Park-Manager framework, since there are many more pull requests and bug reports than there are members in the Park-Manager core team to review, fix and merge them.

On the Park-Manager issue tracker, you can find many items in a Needs Review status:

Bug Reports: Bug reports need to be checked for completeness. Is any important information missing? Can the bug be easily reproduced?
Pull Requests: Pull requests contain code that fixes a bug or implements new functionality. Reviews of pull requests ensure that they are implemented properly, are covered by test cases, don’t introduce new bugs and maintain backward compatibility.

Note that anyone who has some basic familiarity with Park-Manager and PHP can review bug reports and pull requests. You don’t need to be an expert to help.

Be Constructive¶

Before you begin, remember that you are looking at the result of someone else’s hard work. A good review comment thanks the contributor for their work, identifies what was done well, identifies what should be improved and suggests a next step.

Create a GitHub Account¶

Park-Manager uses GitHub to manage bug reports and pull requests. If you want to do reviews, you need to create a GitHub account and log in.

The Bug Report Review Process¶

A good way to get started with reviewing is to pick a bug report from the bug reports in need of review.

The steps for the review are:

Is the Report Complete?

Good bug reports contain a link to a fork of the Park-Manager Standard Edition (the “reproduction project”) that reproduces the bug. If it doesn’t, the report should at least contain enough information and code samples to reproduce the bug.
Reproduce the Bug

Download the reproduction project and test whether the bug can be reproduced on your system. If the reporter did not provide a reproduction project, create one by forking the Park-Manager Standard Edition.
Update the Issue Status

At last, add a comment to the bug report. Thank the reporter for reporting the bug. Include the line Status: <status> in your comment to trigger our Carson Bot which updates the status label of the issue. You can set the status to one of the following:

Needs Work If the bug does not contain enough information to be reproduced, explain what information is missing and move the report to this status.

Works for me If the bug report does contain enough information to be reproduced but works on your system, or if the reported bug is a feature and not a bug, provide a short explanation and move the report to this status.

Reviewed If you can reproduce the bug, move the report to this status. If you created a reproduction project, include the link to the project in your comment.

Example

Here is a sample comment for a bug report that could be reproduced:

Thank you @weaverryan for creating this bug report! This indeed looks
like a bug. I reproduced the bug in the "kernel-bug" branch of
https://github.com/webmozart/park-manager-standard.

Status: Reviewed

The Pull Request Review Process¶

The process for reviewing pull requests (PRs) is similar to the one for bug reports. Reviews of pull requests usually take a little longer since you need to understand the functionality that has been fixed or added and find out whether the implementation is complete.

It is okay to do partial reviews! If you do a partial review, comment how far you got and leave the PR in “Needs Review” state.

Pick a pull request from the PRs in need of review and follow these steps:

Is the PR Complete?

Every pull request must contain a header that gives some basic information about the PR. You can find the template for that header in the Contribution Guidelines.
Is the Base Branch Correct?

GitHub displays the branch that a PR is based on below the title of the pull request. Is that branch correct?
- Bug fixes and new features must always target the master branch. Only exceptional fixes should target a maintenance branch.
Reproduce the Problem

Read the issue that the pull request is supposed to fix. Reproduce the problem on a clean Park-Manager Standard Edition project and try to understand why it exists. If the linked issue already contains such a project, install it and run it on your system.
Review the Code

Read the code of the pull request and check it against some common criteria:
- Does the code address the issue the PR is intended to fix/implement?
- Does the PR stay within scope to address only that issue?
- Does the PR contain automated tests? Do those tests cover all relevant edge cases?
- Does the PR contain sufficient comments to easily understand its code?
- Does the code break backward compatibility? If yes, does the PR header say so?
- Does the PR contain deprecations? If yes, does the PR header say so? Does the code contain trigger_error() statements for all deprecated features?
- Are all deprecations and backward compatibility breaks documented in the latest UPGRADE-X.X.md file? Do those explanations contain “Before”/”After” examples with clear upgrade instructions?
Note

Eventually, some of these aspects will be checked automatically.
Test the Code

Take your project from step 3 and test whether the PR works properly. Replace the Park-Manager project in the vendor directory by the code in the PR by running the following Git commands. Insert the PR ID (that’s the number after the # in the PR title) for the <ID> placeholders:
```
$ cd vendor/park-manager/park-manager
$ git fetch origin pull/<ID>/head:pr<ID>
$ git checkout pr<ID>
```
For example:
```
$ git fetch origin pull/15723/head:pr15723
$ git checkout pr15723
```
Now you can test the project against the code in the PR.
Update the PR Status

At last, add a comment to the PR. Thank the contributor for working on the PR. Include the line Status: <status> in your comment to trigger our Carson Bot which updates the status label of the issue. You can set the status to one of the following:

Needs Work If the PR is not yet ready to be merged, explain the issues that you found and move it to this status.

Reviewed If the PR satisfies all the checks above, move it to this status. A core contributor will soon look at the PR and decide whether it can be merged or needs further work.

Example

Here is a sample comment for a PR that is not yet ready for merge:

Thank you @weaverryan for working on this! It seems that your test
cases don't cover the cases when the counter is zero or smaller.
Could you please add some tests for that?

Status: Needs Work