Last updated March 7, 2017.

  • Goals: Broad view on how and why to review.
  • Workflow: Specific details on how to review.

This guide supports Drupal developers and themers as they review project applications. It is the flip side of Apply for permission to opt into security advisory coverage.

Application reviews are open to everyone and a great way to support the Drupal community! Please start with issues that have a review bonus or with issues that did not get a review in a long time. We need your help! Even reviewing one application helps a lot!

Goals

The following are the goals and ideas behind the application review methodology:

Enabling new contributors is the ultimate goal of this process. The code contribution application is often a first step into the Drupal community and should be viewed as a mentoring moment for this person. A good experience here can lead to a much stronger contributor in the long run, and creating a painful process may turn them away from contributing at all. It is important for reviewers to help recommend avenues for new contributors to get involved, i.e. with similar projects, ongoing issues, active workgroups, etc. Good mentors help new contributors more with their knowledge of community practices than with code style changes. The following should be kept in mind when reviewing:

  • Be supportive. Obviously this is very subjective, but it is important that your comments and review are meant to encourage and foster good code. Positive comments are just as helpful as negative ones; feel free to say how awesome something is, as well as point out how things need to be improved.
  • It is extremely important to provide useful comments. This means not just saying that code is wrong, but explaining why and using links to documentation.
  • Even if the code is perfect, you can still offer your experience as a Drupal developer. This application is not just about making sure code is perfect, its about ensuring that we have quality contributors to Drupal. If the code is good enough to accept the application, mark as reviewed, but make suggestions on how the applicant can improve their code to make it better for the community. For instance, suggesting Views integration, looking at X module, or saying how adding a hook here would make this much more extensible, etc.
  • Keep your expectations at a minimum. Try to see the perspective of the applicant as they may have not ever contributed code to an open source project, may have not read all the Drupal documentation about contributing, may be a brand new developer, or may not even speak English well. This means you may have to do a little hand holding at first, which should be acceptable.

Security and code quality (high priority) is very important to Drupal's reputation. So, ensuring that all projects are written to Drupal's coding standards and best practices is a critical role for reviewers. Writing high quality, secure code empowers all Drupal users and developers to be able to interact with it, both with understanding and developing, and helps streamlines collaboration.

Licensing (high priority) requirements for the code that is hosted on drupal.org is fairly strict. It is important to maintain this so that the community of users benefits from all the code, and drupal.org does not have any serious legal problems. 3rd party libraries on Drupal.org have specific requirements which extends to Why drupal.org doesn't host GPL-"compatible" code.

Module duplication (lower priority) makes it hard to find the right module, which is a real problem on drupal.org, and continues to grow. Again, to ensure that the code on drupal.org is useful to the community of people using Drupal, it is important to ensure that module duplication is avoided. However, duplication is different from similarity. A similar project that explains how it is different is helpful to the growth of our project. The Similar Module Review Workgroup helps with finding duplicate modules, analyzing their similarities/differences, and seeing if there is room for collaboration between them.

The project application checklist is a good guide for reviewing an application and the PAReview: Review Template is a good template to utilize while reviewing applications.

Workflow

There are two main ways to get access to mark projects as covered by security advisories:

  1. Apply with new project
  2. Volunteer to maintain an unsupported project

Drupal.org project application workflow is important to keep tasks organized. Overall, you want to alternate status between:

  • needs review: the application needs to be reviewed.
  • needs work: the application should provide more information to be reviewed.

The project application workflow page describes when to change the status.

Application Priority

Application priorities may be implemented to ensure that applications that have been awaiting review for extended periods of time are given priority. Priorities may be set according to priority guidelines by either the applicant or the reviewer and generally apply only to applications with a status of needs review. Applications in which the applicant has failed to respond should not be prioritized. For applications with elevated priority, once a reviewer has continued the review of the application by responding to the applicant according to the project application workflow the application priority should return to normal.

The guidelines on elevating project application priority are as follows:

  • Normal: All applications must begin with a normal priority. Applications with elevated priorities should be returned to normal priority once a reviewer has continued the application review.
  • Major: Applications with a status of needs review that have been awaiting response from a reviewer for 2+ weeks.
  • Critical: Applications with a status of needs review that have been awaiting response from a reviewer for 4+ weeks.

New Project

This route is by far the most time consuming, but also the most important and rewarding. So be thorough, but do not arbitrarily create barriers. Throughout these steps, it may be best to have the applicant fix things before further review.

  1. Ensure that there is a detailed description of what the project does and a full description of how it differs from any other project.
    • Search for similar projects yourself.
    • Keep in mind that English may be difficult for who is applying.
  2. Search through the files uploaded for any possible non-GPL, or third-party library files. Ask the applicant to explain these and make sure to point to relevant documentation.
  3. Review for security.
    • See Writing secure code (and child pages). Applications that have security issues should be tagged with 'PAReview: security'.
  4. Review for best practices.
  5. Review for coding standards.
  6. For a list of important issues that need to be resolved, see Review process for security advisory coverage: what to expect.

Abandoned Project Applications

The unsupported project process does not actually give the user the Git Vetted permission. Sometimes, someone will want to apply for the Git vetted role with the project that they have taken over. Overall, this is fine, but keep the following in mind:

  • Get a detailed description of updates. Basically, there needs to be some proof that the user has made enough code changes to be able to review the project.
  • The main issue with this circumstance is that there is not really a feasible way to determine how much code was written by the user applying. The assumption that should be made is that if the code reviewed is good enough, and the user has written a significant part of it, then there can be a correlation between their knowledge and quality of code.

Abandoned Applications

After four weeks with a status of needs work: the PAReview robot will close the application with a note saying that it appears abandoned, but that the applicant may reopen the issue if they wish to continue the application process in the future.

Comments

Tommy Kaneko’s picture

There have been instances of reviewers applying newer coding standards (used by the D7 Coder module, for example), to older code (developed with Coder for D6, for eg.), where standards have changed since contributors wrote parts of the code. Although this is not strictly bad practice, it can cause delays in the application, which it shouldn't.

The documentation should clarify that reviewers in certain cases (like above), should not block project applications on account of contributors using older coding standards, while encouraging consistency.

For more discussion:
http://groups.drupal.org/node/179939

Design is my game http://tomkaneko.com

bailey86’s picture

This issue of applying different coding standards was a real pain about six months ago.

I helped to resolve the issue by basically pointing the documentation at http://ventral.org/pareview which is (sort-of) the standard now for code review.

joachim’s picture

What's the tag for an application whose code has security issues? The review template mentions it but doesn't say what it is.