Last updated March 11, 2013.
This guide supports Drupal developers and themers as they review applications to create full projects. It is the flip side of Applying for permission to create full projects.
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, ie with similar projects, ongoing issues, active workgroups, etc 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 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 modules 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.
Workflow
There are two main ways to get full project access:
- Apply with new module
- Volunteer to maintain an unsupported module
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 the 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 Module
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.
- Ensure that there is a detailed description of what the module does and a full description of how it differs from any other project.
- Search for similar modules yourself.
- Keep in mind that English may be difficult for who is applying.
- 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.
- Review for security.
- See Writing secure code (and child pages).
- Review for best practices.
- See Programming best practices (and all sub pages).
- Review for coding standards.
- See Coding standards (and all sub pages).
- See also Module documentation guidelines.
- For a list of important issues that need to be resolved, see Full project applications review, what to expect.
Abandoned Module Applications
The unsupported module process does not actually give the user the Git Vetted permission. Sometimes, someone will want to apply for Full Project access (Git Vetted permission) with the module 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 module.
- 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
For project applications that appear abandoned:
- After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.
- After two weeks following attempted contact: if the applicant has failed to respond the application status may be set to closed (won't fix). The action should be documented and applicant notified that they may reopen the issue if they wish to continue the application process in the future.
Comments
Applying newer coding standards to older code
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
This issue of applying
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.