Template for reviewers

This is a collection of responses that can be used as the starting point from reviewers. They are intended to encourage the new contributor, explain why their hard work may not be acceptable and point them in the right direction to continue helping the Drupal community.
These responses are periodically changed, to reflect changes in the review process, or to reflect what reported in recent reviews. This page should be periodically checked.

Pre-review checks

These are responses given before starting to review the project for the correct usage of Drupal API, for code that correctly follow the Drupal coding standards or for security issues. 
It is important to give these responses before starting to effectively review the project used for the application.

Application from a maintainer who can already opt projects into security advisory coverage

<p>It seems you are already able to opt projects (for which you can edit the project node) into security advisory coverage. In this case, there is no need to apply for a second time, as these applications just give the permission to opt projects into security advisory coverage.</p>
<p>If you are not able to opt a project into security advisory coverage, and you are maintainer of that project, it means that either you do not have the permission to edit the project node, or the project has been created less than ten days ago. In both the cases, there is nothing a second application would fix.</p>
<p>In this case, the status of this application is either set to <em>Closed (duplicate)</em>, if the application that gave you the permission to opt projects into security coverage can be found, or to <em>Closed (won't fix)</em>, if that application cannot be found.</p>

Multiple applications

<p>It appears there are multiple project applications created using your account.</p>
<p>Since a successful completion of the project application process results in the applicant being granted the necessary role to be able to opt projects into security advisory coverage, there is no need to take multiple applications through the process. Once the first application has been successfully approved, the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers, which in turn results in longer wait times for everyone in the queue.</p>
<p>With this in mind, your secondary applications have been marked as <em>Closed (duplicate)</em>, with only one application left open. If, in the existing application you were asked to apply with a different project, please edit the issue summary of that application to change the project description and the project link.</p>

Commits are not from a single maintainer

<p>For these applications, we need a project where, in at least a branch, most of the commits (but preferably all the commits) have been done from the person who created the application.</p>
<p>The purpose of these applications is reviewing a project to understand what the person who applies understands about writing secure code which follows the Drupal coding standards and correctly uses the Drupal API, not what all the project maintainers collectively understand about those points.</p>
<p>Do you have a project for which most of the commits have been done by you in at least a branch? It also needs to contain enough Drupal-related-PHP code.</p>

Project is a duplicate

<p>It seems the project used for this application is duplicating the functionality provided from an existing project.<p>
<p>Rather than do the same work multiple times, we would encourage you to contribute patches to the existing project, or even become a co-maintainer.</p>
<p>If you feel your project is different, then please explain the differences on your project page, to allow users choose which of the modules is best suited for them.</p>

The project uses a main branch

<p><em>main</em> is acceptable as branch name, but it is not yet fully supported on drupal.org.</p>
<p>With contributed projects, it is still not possible to create an issue for the <em>main</em> branch. For projects used in these applications, for which is still suggested to work with release-specific branches, it should be better to avoid using a branch with that name.</p>

The project uses a master branch

<p>The project is using a <em>master</em> branch.</p>
<p>You should really be working in a release specific branch. The most direct documentation on this is <a href="https://www.drupal.org/node/1127732">Moving from a master branch to a version branch.</a> For additional resources please see the documentation about <a href="https://www.drupal.org/node/1015226">release naming conventions</a> and <a href="https://www.drupal.org/node/1066342">creating a branch in git</a>.</p>

 The project contains a LICENSE file for a license that is different from the license used from Drupal core

<p>Projects hosted on drupal.org are licensed under GPLv2+, the same license used from Drupal core.</p>
<p>If you are licensing a project under a different license, it cannot he hosted on drupal.org. More details are given in <a href="https://www.drupal.org/docs/develop/git/setting-up-git-for-drupal/drupal-git-usage-policies/drupal-git-contributor">Drupal Git Contributor Agreement &amp; Repository Usage Policy</a>.</p>

<blockquote>
<p>All code that is a derivative work of Drupal (typically PHP code, including but not limited to: core patches, modules, themes, etc) committed to Drupal.org's git repository is licensed as GPL version 2.0 and later (official short identifier: “GPL-2.0-or-later”). This means that the code is licensed under GPLv2, and there exists an option that allows downstream recipients to re-license the code to be under a later version of GPL.</p>
</blockquote>

<p>For code licensed under GPLv3, see See <a href="https://www.drupal.org/about/licensing#gpl2-only-gpl3-only">I want to release my work under GPL version 3 or under GPL version 2-only. Can I do so and host it on Drupal.org?</a></p>

<blockquote>
<p>No. You can release your work under any GPL version 2 or later compatible license. However, you may only check it into Drupal's Git repositories if you are releasing it under the same license as Drupal itself, that is GPL version 2 or later, allowing users to choose between the terms of the GPL version 2 or the terms in any new versions as updated by the FSF. If you are unable or unwilling to do so, do not check it into a Drupal Git repository.</p>
</blockquote>

<p>The content of the <em>LICENSE.txt</em> file is expected to be the same of the <a href="https://git.drupalcode.org/project/drupal/-/blob/main/core/LICENSE.txt?ref_type=heads">LICENSE.txt</a> file used by Drupal core.</p> 

Review responses

These are the responses that should be used when reviewing a project used in an application. The responses are not listed in any particular order, not even of importance. (The priority of different review aspects is given in a different documentation page.)

Dependency injection not used in a class

<p>Any dependency must be injected using the dependency injection container, except in the case the dependency is used only from static methods.</p>

Property already defined in the parent class

<p>Those properties are already defined from the parent class. There is no need to define them again.</p>

Controller classes do not need to extend ControllerBase

<p>Since that class does not use methods from the parent class, or it uses a single method from the parent class, it does not need to use <code>ControllerBase</code> as parent class.</p>
<p>Controllers do not need to have a parent class; as long as they implement <code>\Drupal\Core\DependencyInjection\ContainerInjectionInterface</code>, they are fine.</p>

 Dynamic content passed to t()

<p>The first parameter for <code>$this->t()</code> or <code>t()</code> must be a literal string, not a concatenation of literal strings, the output of a function/method, the value of a local variable, nor the value of a class property.</p>
<p>If then what passed as argument is user input, that is a security issue.</p>

Wrong placeholder for URL

<p>The correct placeholder for URLs is <em>:variable</em> as shown in the documentation for <a href="https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Render%21FormattableMarkup.php/function/FormattableMarkup%3A%3AplaceholderFormat/9.3.x"><code>FormattableMarkup::placeholderFormat()</code></a>. That is the placeholder used also by Drupal core code for URLs that take to drupal.org.</p>

<code>
$output = '';
$output .= '<h3>' . t('About') . '</h3>';
$output .= '<p>' . t('The Actions module provides tasks that can be executed by the site such as unpublishing content, sending email messages, or blocking a user. Other modules can trigger these actions when specific system events happen; for example, when new content is posted or when a user logs in. Modules can also provide additional actions. For more information, see the <a href=":documentation">online documentation for the Actions module</a>.', [
  ':documentation' => 'https://www.drupal.org/documentation/modules/action',
]) . '</p>';
</code>

Form classes do not need to extend FormBase

<p>The class does not use any method (or a single method) defined by the parent class.</p>
<p>A form does not need to have <code>FormBase</code> as parent class: as long as it implements the required interface, it is fine.</p>

The parent class is not part of the public API

<p>The class is not part of the public API and cannot be used as parent class for classes implemented by a contributed module.</p>
<p>Only base classes or classes that use the <code>@api</code> annotation can be used as parent class.</p>

Wrong arguments for the logger methods

<p>The <code>$message</code> parameter passed to the <a href="https://api.drupal.org/api/drupal/vendor%21psr%21log%21Psr%21Log%21LoggerInterface.php/interface/LoggerInterface/9.3.x"><code>LoggerInterface</code></a> methods must be a literal string that uses placeholders. It is not a translatable string returned from <code>t()</code>/<code>$this->t()</code>, a string concatenation, a value returned from a function/method, nor a variable containing an exception object.</p>

Wrong arguments for the messenger methods

<p>The first argument passed to <code>MessengerInterface::addError()</code>, <code>MessengerInterface::addMessage()</code>, <code>MessengerInterface::addStatus()</code>, and<code> MessengerInterface::addWarning()</code> must be a translatable string which uses placeholders.</p>

Hooks are still procedural

<p>A new module that aims to be compatible with latest Drupal releases is expected to implement hooks as class methods as described in <a href="https://www.drupal.org/node/3442349">Support for object oriented hook implementations using autowired services</a>.
It requires increasing the minimum required Drupal version, but that is not an issue, since not all the Drupal releases are supported.</p>

Annotations are used instead of attributes

<p>Projects that are compatible with Drupal 10 or higher versions should use attributes instead of annotations. This means requiring at least Drupal 10.3, but this is not an issue, considering which Drupal core versions are nowadays supported.</p>

A controller defines a property for the Request object

<p>There is no need to use that property, since the <code>Request</code> object is automatically passed to the page controller, if one of its parameters is correctly type-hinted. See <a href="https://www.drupal.org/docs/8/api/routing-system/parameters-in-routes/using-parameters-in-routes">Using parameters in routes</a>.</p>

<code>
public function content(AccountInterface $user, Request $request) {
  // …
}
</code>

Wrong argument for FormState::setErrorByName()

<p>The second argument for <code>setErrorByName()</code> must be a translatable string.</p>

Calls to \Drupal methods in OOP hooks

<p>A class that implements hooks does not need to use <code>\Drupal</code> for its dependencies.</p>
<p> As described in <a href="https://www.drupal.org/node/3442349">Support for object oriented hook implementations using autowired services</a>, those hook classes are handled as autowired services, without any need to define them as services.</p>

Status/priority responses

These are comments posted when the status or the priority is changed.

Priority is changed

<p>I am changing priority as per <a href="https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or-distribution-project/security-coverage/reviewing/issue-priorities">Issue priorities</a>.</p>

The status is changed from Active to Closed (won't fix)

<p>I am closing this application because its status was <em>Active</em> and it was not changed in the past 14 days.</p>
<p>Feel free to re-open this application, if you are the applicant and you are sure you can follow it the time necessary to get it closed as fixed.</p>

The status is changed from Needs work to Closed (won't fix)

<p>This thread has been idle, in the <em>Needs work</em> state with no activity for about six months or more; the application has been created about 11 months ago or more. Therefore, I marked it as <em>Closed (won't fix)</em>.</p>
<p>If this is incorrect, and you are still pursuing this application, please feel free to re-open it and set the issue status to <em>Needs work</em> or <em>Needs review</em>, depending on the current status of your code.</p>