[D10] [D11] [1.0.x] Webform OpenFisca

Thank you for applying!

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review → gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

1. main is a wrong name for a branch. Release branch names always end with the literal .x as described in Release branches → .

main will be a supported branch in future, but for the moment it is better not to use it. It is not wrong, but it is not completely supported on drupal.org.

2. FILE: webform_openfisca.module

/**
 * @file
 * Webform OpenFisca module.
 */

FILE: modules/webform_openfisca_key_auth/webform_openfisca_key_auth.module

/**
 * @file
 * Webform OpenFisca Key Authentication.
 */

The usual description for a .module file is Hook implementations for the [module name] module. where [module name] is the module name given in the .info.yml file.

3. FILE: src/RacContentHelper.php

  /**
   * Constructor.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   *   Entity Type Manager service.
   */
  public function __construct(

FILE: src/WebformFormAlterBase.php

  /**
   * Constructor.
   *
   * @param \Drupal\webform_openfisca\OpenFisca\ClientFactoryInterface $openFiscaClientFactory
   *   OpenFisca connector.
   * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
   *   String translation.
   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
   *   Messenger service.
   * @param \Drupal\Core\Cache\CacheTagsInvalidatorInterface $cacheTagsInvalidator
   *   Cache tags invalidator.
   */
  public function __construct(

FILE: src/WebformOpenFiscaSettings.php

  /**
   * Constructor.
   *
   * @param \Drupal\webform\WebformInterface $webform
   *   The webform.
   */
  protected function __construct(WebformInterface $webform) {

FILE: src/OpenFisca/Client.php

  /**
   * Constructor.
   *
   * @param string $baseApiUri
   *   The base API endpoint.
   * @param \GuzzleHttp\ClientInterface $httpClient
   *   The HTTP Client service.
   * @param \Psr\Log\LoggerInterface $logger
   *   The logger service.
   * @param array $httpClientOptions
   *   The options initialising the HTTP Client (used by the client factory).
   */
  public function __construct(

FILE: src/OpenFisca/ClientFactory.php

  /**
   * Constructor.
   *
   * @param \Drupal\Core\Http\ClientFactory $httpClientFactory
   *   HTTP Client Factory service.
   * @param \Psr\Log\LoggerInterface $logger
   *   Logger service.
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $moduleHandler
   *   Module handler.
   * @param \Drupal\Core\Theme\ThemeManagerInterface $themeManager
   *   Theme manager.
   */
  public function __construct(

FILE: src/OpenFisca/Payload.php

  /**
   * Constructor.
   */
  final public function __construct() {}

The documentation comment for constructors is not mandatory anymore, If it is given, the description must be Constructs a new [class name] object. where [class name] includes the class namespace.

Hi @vishal.kadam,

1. The main is a legacy branch and not in use. The main development branch is 1.x and the requested branch is 1.0.x. I have, however, deleted the main branch from Drupal git.
2. I have updated the code as per your suggestions in the latest commit https://git.drupalcode.org/project/webform_openfisca/-/commit/5aaada2867...

For the coding standard issues, are they enforced by any Drupal/DrupalPractice PHPCS rules? We would like to enable those checks in the current CI so that they can be caught during pipeline runs.

Thank you for your review.

It appears that PHPCS checks are already enabled in your current CI, and the pipeline is running successfully without any errors.

Rest looks fine to me.

Let’s wait for a Code Review Administrator to take a look and if everything goes fine, you will get the role.

I am changing priority as per Issue priorities → .

I will open a parenthesis, but this is exactly the reason why the first review should be done from a project moderator, which (to make it clearer) does not mean "an application reviewer." There are commits which have not been done from the application, but from the project owner.
These applications are not done to verify what the maintainers as group understand about writing secure code which follows the Drupal coding standards, and which correctly uses the Drupal API. They are done to verify what the applicant understands about those topics to give the applicant the permission to opt projects into security advisory coverage.

Starting from today, any review done before a project moderator posted the first comment in an issue will be ignored. I apologize for the parenthesis, but since my previous comments have been ignored, I had to make this clear.

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.
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.

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 PHP code.

Hi @avpaderno,

I'm not sure why my commit count is not considered to be enough to apply. The project has had 75 commits since inception and 32 were authored by me. Since September 2024 (6 months ago), the majority of the commits were solely authored by myself. About 3 months ago, most of the code was rewritten (from this commit a66c7e8b) and almost all PHP code was authored by me.

Are you able to check the commit history of the project to verify my point, please?

The code has been changed, but using a merge request created by the project owner, which could have approved the merge request.

I don't understand why that merge request matters as it was closed and it changes were not merged into the branch 1.0.x. Also, the majority of commits in that merge request were authored by me.

You could see many commits "Merge pull request..." from the project owner because the project was mainly developed on our GitHub repository. We have recently synced the code to Drupal Git when the project was ready as a public module.

If my commit count is not enough for this type of application, does the commit count of the project owner qualify her as an applicant?

It matters because the commits in the merge request have not been done by you. They have been done by the project owner before you applied.

does the commit count of the project owner qualify her as an applicant?

What applies for you applies for the project owner too.

Hi @avpaderno,

Thank you for your clarification. I am closing this application as both maintainer and project owners are not qualified to apply.