[3.0.x] SearchUnify Drupal Connector

Manual review of the 3.0.x branch, commit 5a03a9b.

The issue summary lists three claims that I could not confirm in the code. Since they are the security-relevant part of the summary, they should be corrected or implemented before this goes further.

1. Key module integration. The summary says "Key module integration for secure API token storage", but there is no reference to the Key module anywhere in the branch: no drupal/key entry in composer.json, no key entry in sudc.info.yml dependencies, and no use of key.repository in src/. The provision key and the access token are stored as plain strings in the sudc.configs config object, which means they are also written to any config export.
2. Access token exposed in the JWT payload. The summary says the module "prevents access tokens from being exposed", but SuResultController::getsearchjwt() puts the SearchUnify access token into the JWT payload, as access_token in buildJwtPayload(). A JWT payload is base64url, not encrypted. The route sudc.getsearchjwt only requires _user_is_logged_in, so any authenticated user, including a self-registered one, can call /search-unify/v1/search_jwt and base64-decode the site's SearchUnify access token out of the response. The token does not need to be in the payload for the signature to work, since it is already used as the HMAC key.
3. Provision key rendered in cleartext. In SuConfigForm::buildForm() the Provision Key element is a password element, but it also sets an #attributes array whose value key holds the stored provision key. That renders the secret in cleartext in the value attribute of the HTML source of /admin/config/sudc. The usual pattern is a password field left empty with a "leave blank to keep the current value" description, and a submit handler that keeps the stored value when the field is empty.
4. No config schema. There is no config/schema/ directory. config/install/sudc.configs.yml defines uid, epoint, cdn, token_expiry, provision_key and access_token, and repeater_data and num_fields are written at runtime, but none of them have a schema. Config schema is required.
5. Keys added by the packaging script. sudc.info.yml contains version: '3.0.1' and project: 'sudc'. Both keys are added by the drupal.org packaging script and must not be in the repository.
6. composer.json name. It declares "name": "grazitti/drupal-searchunify-connector". For a project hosted on drupal.org this has to be drupal/sudc.
7. GitLab CI. There is no .gitlab-ci.yml in the branch, so CI is not running and PHP_CodeSniffer results are not visible.
8. Library definition. sudc.libraries.yml declares css/all.min.css, which does not exist in the repository (css/ only contains style.css), and it uses version: VERSION, which is reserved for Drupal core modules. js/index.js exists but is not attached by any library, so it is currently dead code.
9. Committed junk. .DS_Store is committed twice, at the repository root and in src/.
10. Dependency injection. The summary says runtime service lookups were replaced by dependency injection, but three static lookups remain. SuResultController::getsearchjwt() line 257 uses \Drupal::service('private_key'). CommonCalls::grazittiHeader() line 66 uses \Drupal::service('extension.list.module') in a class that already receives its dependencies through the container. And SuHelpController::create() and SuResultController::create() both inject the result of getCurrentRequest() on the request stack. The Request object should not be stored on the controller: type-hint it on the page callback and Drupal passes it in, or inject request_stack itself.
11. Parent constructor not called. SuConfigForm::__construct() does not call parent::__construct(), so ConfigFormBase does not receive its ConfigFactoryInterface and, on Drupal 10.2 and later, its TypedConfigManagerInterface.
12. Undocumented debug parameter. buildForm() reads a df query parameter, and ?df=1 renders the access token in a form field. If this is debugging code it should be removed.
13. PHP coding standards. The PHP in src/ uses 4-space indentation with the opening brace on its own line, lowercase true, false and null, snake_case class properties such as $provision_key and $token_expiry, and PEAR-style docblock tags (@category, @package, @author, @link, @since). Drupal uses 2 spaces, the brace on the same line, uppercase TRUE, FALSE and NULL, lowerCamelCase properties, and none of those tags. Note also that SuHelpController.php and SuConfigForm.php still carry the template placeholder author name and example email address, and that several docblocks link to http://graztti.com, which is both a typo and plain http.
14. Missing cacheability metadata. SuResultController::content() builds a render array from sudc.configs but attaches no cacheability metadata. It needs at least the config:sudc.configs cache tag, otherwise the rendered search page will not be invalidated when the configuration changes.

I did not test the module against a live SearchUnify instance, so the first three points are based on reading the code only. Happy to re-check once the branch is updated.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that does not follow the coding standards, contains possible security issue, or does not correctly use the Drupal API
• The single review points are not ordered, not even by importance

src/Form/SuConfigForm.php

  /**
   * @param \Drupal\Core\Config\ConfigFactoryInterface      $configFactory      Config factory.
   * @param \Drupal\Core\Config\TypedConfigManagerInterface $typedConfigManager Typed config manager.
   * @param \Drupal\sudc\Services\CommonCalls               $ccall              CommonCalls service.
   * @param \Drupal\sudc\Services\RestCalls                 $rcall              RestCalls service.
   */
  public function __construct(
    ConfigFactoryInterface $configFactory,
    TypedConfigManagerInterface $typedConfigManager,
    CommonCalls $ccall,
    RestCalls $rcall
  ) {
    parent::__construct($configFactory, $typedConfigManager);
    $this->ccall = $ccall;
    $this->rcall = $rcall;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('config.factory'),
      $container->get('config.typed'),
      $container->get('sudc.commonCalls'),
      $container->get('sudc.restCalls')
    );
  }

That code cannot be used in Drupal 9.x and in Drupal 11.x because the constructor for \Drupal\Core\Form\ConfigFormBase accepts two parameters only on Drupal 10.2.x and forward. (The relative change record is New parameter added to \Drupal\Core\Form\ConfigFormBase::__construct.)
While there are other possible ways to rewrite that code, dropping the support for no longer supported Drupal releases, in the reviewed branch, is probably the quicker way to fix the code. If there is really the need to also support Drupal releases no longer supported by Drupal core maintainers, it is possible to have a branch for Drupal 9 and a branch for currently supported Drupal releases.

With Drupal 10 and Drupal 11, there is no longer need to use #default_value for each form element, when the parent class is ConfigFormBase: It is sufficient to use #config_target, as in the following code.

$form['image_toolkit'] = [
  '#type' => 'radios',
  '#title' => $this->t('Select an image processing toolkit'),
  '#config_target' => 'system.image:toolkit',
  '#options' => [],
];

Using that code, it is no longer needed to save the configuration values in the form submission handler: The parent class will take care of that.
For this change, it is necessary to require at least Drupal 10.3, but that is not an issue, considering which Drupal releases are currently supported by Drupal core maintainers.

That said, ConfigBaseForm is usually the base class used for simple configuration forms, not for complex forms like this one. For these cases, a different base class is probably preferable..

    $form['hidden'] = [
      '#type'       => 'hidden',
      '#attributes' => ['autocomplete' => 'off'],
    ];

An hidden form element does not use attributes like autocomplete because it is not an input form element.
Furthermore, that element is not used by the form validation handler nor the for submission handler; it can be removed.

    $form['body']['token_expiry'] = [
      '#type'          => 'number',
      '#title'         => $this->t('Enter expiration time'),
      '#description'   => $this->t('Authentication token for search in minutes'),
      '#size'          => 64,
      '#default_value' => $suConfigs->get('token_expiry') ?? '180',
      '#maxlength'     => 200,
      '#required'      => TRUE,
    ];

As per Drupal coding standards, before and after operators like => there needs to be a single space. Array values are not vertically aligned, simply to avoid changing the code when a longer key is added to the form.

    $num_fields = $this->config('sudc.configs')->get('num_fields');
    if ($num_fields === '' || $num_fields === NULL) {
      $this->configFactory()->getEditable('sudc.configs')->set('num_fields', 1)->save();
      $num_fields = 1;
    }

The configuration editable object is returned by $this->config(), when the parent class is ConfigFormBase. Clearly, when the parent class is different, that method could be not available.

buildForm() does not call the method defined by the parent class.

    $num_fields = $this->config('sudc.configs')->get('num_fields');
    $this->configFactory()->getEditable('sudc.configs')
      ->set('num_fields', $num_fields + 1)->save();
    $form_state->setRebuild();
  }

The object returned by $this->config() can be used to set values for the configuration objects whose names are returned by getEditableConfigNames(). (That method name should give an hint about what it is supposed to return.)

    $form['body']['provision_key'] = [
      '#type'        => 'password',
      '#title'       => $this->t('Provision Key'),
      '#description' => $this->t('Provision Key from SearchUnify app. Leave blank to keep the current value.'),
      '#size'        => 64,
      '#required'    => empty($suConfigs->get('provision_key')),
      '#maxlength'   => 200,
      '#attributes'  => ['autocomplete' => 'off'],
    ];

I understand that a password form element is used to avoid the provision key is visible to somebody who gained access to an account with the permission to access the form, but that is bad UX for people who legitimately need to change that value. As it is, they can only rely on an error message which tells the provision key is wrong, but they need to re-enter the full provision key, instead of correcting the entered value.
To protect against those cases, there are better solutions, including using the Key module, or doing like Drupal core does with public files folder, for example.

      if (empty($provisionKey) || empty($epoint) || empty($uid)) {
        $form_state->setErrorByName('message', $this->t('All required fields are mandatory.'));
        continue;
      }

That code is not necessary: It is sufficient to set #required to TRUE for all the required values. Drupal core will not validate the form when values for required form elements are not entered.

src/Controller/SuHelpController.php

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 ControllerBase as parent class.

Controllers do not need to have a parent class; as long as they implement \Drupal\Core\DependencyInjection\ContainerInjectionInterface, they are fine.

  /**
   * @param \Drupal\Core\Extension\ModuleExtensionList     $moduleExtensionList Module list service.
   * @param \Symfony\Component\HttpFoundation\RequestStack $requestStack        Request stack.
   */
  public function __construct(ModuleExtensionList $moduleExtensionList, RequestStack $requestStack) {
    $this->moduleExtensionList = $moduleExtensionList;
    $this->requestStack        = $requestStack;
  }

There is no need to use a \Symfony\Component\HttpFoundation\RequestStack or a \Symfony\Component\HttpFoundation\Request as class properties, since the \Symfony\Component\HttpFoundation\Request object is automatically passed to the page controller, if one of its parameters is correctly type-hinted. See Typehinted parameters.

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

The documentation comment for class constructors is no longer mandatory; if it is provided, it need to give the same information given in the following example code, formatted in the same way. I would rather avoid adding documentation comments for class constructors, though.

  /**
   * Constructs a new AccountForm object.
   *
   * @param \Drupal\Core\Entity\EntityRepositoryInterface $entity_repository
   *   The entity repository.
   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
   *   The language manager.
   * @param \Drupal\Core\Entity\EntityTypeBundleInfoInterface $entity_type_bundle_info
   *   The entity type bundle service.
   * @param \Drupal\Component\Datetime\TimeInterface $time
   *   The time service.
   */
  public function __construct(EntityRepositoryInterface $entity_repository, LanguageManagerInterface $language_manager, ?EntityTypeBundleInfoInterface $entity_type_bundle_info = NULL, ?TimeInterface $time = NULL) {
    parent::__construct($entity_repository, $entity_type_bundle_info, $time);
    $this->languageManager = $language_manager;
  }

  /**
   * Renders the help page.
   *
   * @return array
   *   Render array for the helpcontent theme hook.
   */
  public function content() {
    $request    = $this->requestStack->getCurrentRequest();
    $baseUrl    = $request->getSchemeAndHttpHost() . $request->getBasePath();
    $modulePath = $baseUrl . '/' . $this->moduleExtensionList->getPath('sudc');
    return [
      '#theme' => 'helpcontent',
      '#mpath' => $modulePath,
    ];
  }

To show a help page, Drupal core provides at least two ways: The easier is implementing hook_help(); the more complex is using help topics. Both are documented in Help and documentation.
In both the cases, a contributed project should not need a specific route/controller just to show its own help.

src/Controller/SuResultController.php

    if ($configVals['status']
      && isset($configVals['data'])
      && !empty($configVals['data'])
    ) {

Drupal coding standards say that control structures should be formatted like the following examples.

if (condition1 || condition2) {
  action1;
}
elseif (condition3 && condition4) {
  action2;
}
else {
  default_action;
}

switch (condition) {
  case 1:
    action1;
    break;

  case 2:
    action2;
    break;

  default:
    default_action;
}

do {
  actions;
} while ($condition);

The opening curly should be on the same line as the opening statement, preceded by one space. The closing curly should be on a line by itself and indented to the same level as the opening statement.

I did not check everything santerref reported, but it seems that not all his points have been taken in consideration.

Remember to change status, when files have been changed basing on the last review done.

FILE: src/Controller/SuHelpController.php

  /**
   * @var \Drupal\Core\Extension\ModuleExtensionList
   */
  protected $moduleExtensionList;

  public function __construct(ModuleExtensionList $moduleExtensionList) {
    $this->moduleExtensionList = $moduleExtensionList;
  }

FILE: src/Controller/SuResultController.php

  /**
   * @var \Drupal\sudc\Services\CommonCalls
   */
  protected $ccall;

  /**
   * @var \Drupal\sudc\Services\RestCalls
   */
  protected $rcall;

  /**
   * @var \Drupal\Core\Config\ImmutableConfig
   */
  protected $cfactory;

  /**
   * @var \Symfony\Component\HttpFoundation\RequestStack
   */
  protected $requestStack;

  /**
   * @var \Drupal\Core\PrivateKey
   */
  protected $privateKey;

  public function __construct(
    CommonCalls $ccall,
    RestCalls $rcall,
    ConfigFactoryInterface $cfactory,
    RequestStack $requestStack,
    PrivateKey $privateKey
  ) {
    $this->ccall        = $ccall;
    $this->rcall        = $rcall;
    $this->cfactory     = $cfactory->get('sudc.configs');
    $this->requestStack = $requestStack;
    $this->privateKey   = $privateKey;
  }

FILE: src/Form/SuConfigForm.php

  /**
   * @var \Drupal\sudc\Services\CommonCalls
   */
  protected $ccall;

  /**
   * @var \Drupal\sudc\Services\RestCalls
   */
  protected $rcall;

  public function __construct(
    ConfigFactoryInterface $configFactory,
    TypedConfigManagerInterface $typedConfigManager,
    CommonCalls $ccall,
    RestCalls $rcall
  ) {
    parent::__construct($configFactory, $typedConfigManager);
    $this->ccall = $ccall;
    $this->rcall = $rcall;
  }  

FILE: src/Services/CommonCalls.php

  /**
   * @var \Drupal\Core\Render\RendererInterface
   */
  protected $rendertemp;

  /**
   * @var \Symfony\Component\HttpFoundation\RequestStack
   */
  protected $requestStack;

  /**
   * @var \Drupal\Core\Extension\ModuleExtensionList
   */
  protected $moduleExtensionList;

  /**
   * @param \Drupal\Core\Render\RendererInterface          $rendertemp          The renderer.
   * @param \Symfony\Component\HttpFoundation\RequestStack $requestStack        The request stack.
   * @param \Drupal\Core\Extension\ModuleExtensionList     $moduleExtensionList Module list service.
   */
  public function __construct(
    RendererInterface $rendertemp,
    RequestStack $requestStack,
    ModuleExtensionList $moduleExtensionList
  ) {
    $this->rendertemp          = $rendertemp;
    $this->requestStack        = $requestStack;
    $this->moduleExtensionList = $moduleExtensionList;
  }

Modules which are compatible with Drupal 10 and higher versions are expected to use constructor property promotion.

Remember to change status, when the project is ready to be reviewed. In this queue, projects are only reviewed when the status is Needs review.

@aman.kumar2 When the status is Needs work like now, no reviewer will check the project. For the few reviewers who already commented, the status is still Needs work because you are not yet done with changes.

As a side note, to change status you need to click on the Save button, or changes will be lost.

As a further note, reviewers report a change only once, but you need to check whether the change applies to other files too. If, for example, it has been reported that before and after operators that accept two arguments there must be a single space, not multiple spaces, you need to verify that no other file contains lines like $this->rendertemp = $rendertemp;.

src/Controller/SuHelpController.php

@file documentation tags are not used for .php files.

<?php

/**
 * @file
 * Controller for the SearchUnify Drupal Connector help page.
 */

namespace Drupal\sudc\Controller;

use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\Core\Extension\ModuleExtensionList;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\Request;

/**
 * Renders the module help page.
 */
class SuHelpController implements ContainerInjectionInterface {

  public function __construct(
    protected ModuleExtensionList $moduleExtensionList,
  ) {}

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('extension.list.module')
    );
  }

  /**
   * Renders the help page.
   *
   * @return array
   *   Render array for the helpcontent theme hook.
   */
  public function content(Request $request) {
    $baseUrl = $request->getSchemeAndHttpHost() . $request->getBasePath();
    $modulePath = $baseUrl . '/' . $this->moduleExtensionList->getPath('sudc');
    return [
      '#theme' => 'helpcontent',
      '#mpath' => $modulePath,
    ];
  }

}

To show a help page, Drupal core provides at least two ways: The easier is implementing hook_help(); the more complex is using help topics. Both are documented in Help and documentation.
In both the cases, a contributed project should not need a specific route/controller just to show its own help.

src/Controller/SuResultController.php

  /**
   * @var \Drupal\Core\Config\ImmutableConfig
   */
  protected $cfactory;

That is a Drupal\Core\Config\ConfigFactoryInterface property.

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('sudc.commonCalls'),
      $container->get('sudc.restCalls'),
      $container->get('config.factory'),
      $container->get('request_stack'),
      $container->get('private_key')
    );
  }

With Drupal 10.2.x and higher versions, a controller class does not need to implement create(); using AutowireTrait, it just needs to implement the class constructor.

    if ($configVals['status']
      && isset($configVals['data'])
      && !empty($configVals['data'])) {
      $uid = $configVals['data']['uid'];
      $token = $configVals['data']['token'];
      $epoint = $configVals['data']['epoint'];
      $reqUrl = $epoint . self::SEARCH_RESULT_API_PATH;
      $postData['uid'] = $uid;
      $postData['accessToken'] = $token;

Drupal coding standards say that control structures should be formatted like the following examples.

if (condition1 || condition2) {
  action1;
}
elseif (condition3 && condition4) {
  action2;
}
else {
  default_action;
}

switch (condition) {
  case 1:
    action1;
    break;

  case 2:
    action2;
    break;

  default:
    default_action;
}

do {
  actions;
} while ($condition);

The opening curly should be on the same line as the opening statement, preceded by one space. The closing curly should be on a line by itself and indented to the same level as the opening statement.

src/Form/SuConfigForm.php

With Drupal 10 and Drupal 11, there is no longer need to use #default_value for each form element, when the parent class is ConfigFormBase: It is sufficient to use #config_target, as in the following code.

$form['image_toolkit'] = [
  '#type' => 'radios',
  '#title' => $this->t('Select an image processing toolkit'),
  '#config_target' => 'system.image:toolkit',
  '#options' => [],
];

Using that code, it is no longer needed to save the configuration values in the form submission handler: The parent class will take care of that.
For this change, it is necessary to require at least Drupal 10.3, but that is not an issue, considering which Drupal releases are currently supported by Drupal core maintainers.

    $form['body']['provision_key'] = [
      '#type' => 'password',
      '#title' => $this->t('Provision Key'),
      '#description' => $this->t('Provision Key from SearchUnify app. Leave blank to keep the current value.'),
      '#size' => 64,
      '#required' => empty($suConfigs->get('provision_key')),
      '#maxlength' => 200,
      '#attributes' => ['autocomplete' => 'off'],
    ];

I understand that a password form element is used to avoid the provision key is visible to somebody who gained access to an account with the permission to access the form, but that is bad UX for people who legitimately need to change that value. As it is, they can only rely on an error message which tells the provision key is wrong, but they need to re-enter the full provision key, instead of correcting the entered value.
To protect against those cases, there are better solutions, including using the Key module, or doing like Drupal core does with public files folder, for example.

src/Services/RestCalls.php

  protected $provisionKey = NULL;
  protected $uid = NULL;
  protected $epoint = NULL;
  protected $accessToken = '';
  protected $tokenExpiry = '';
  protected $cdn = '';
  private $sslVerify = TRUE;
  protected $cfactory;

Every class property needs a documentation comment.

  /**
   * Fetches an OAuth token from the SearchUnify provisioning endpoint.
   *
   * @param string $authUrl  Endpoint URL.
   * @param array  $authbody Form body parameters.
   *
   * @return array
   *   Array with 'status' and 'body' keys.
   */

@param lines are not correctly formatted.

    $reqParam = [
      'verify'      => $this->sslVerify,
      'headers'     => [
        'Content-Type'  => 'application/x-www-form-urlencoded',
        'cache-control' => 'no-cache',
      ],
      'form_params' => $authbody,
    ];

As per Drupal coding standards, operators that require two arguments need to have a single space before and after them. In => , there are seven spaces before the operator.

DRUPAL_COMMUNITY_CHECKLIST.md

That file is not necessary for the module to work. It is rather a checklist for this application, which is no longer relevant when this application will be done.

composer.json

"drupal/core": "^10.2 || ^11 || ^12",

The Drupal.org Composer Façade automatically adds the core requirements for the project basing on what entered in the .info.yml file. As noted in Add a composer.json file:

• The core requirements in the composer.json file are not generated by the façade for Git based checkouts that do not use the façade. For those cases, an entry for drupal/core may be required in the composer.json file.
• When the composer.json file contains the core requirements, those must be compatible with the same version set in the .info.yml file. A mismatch in compatibility may create issues.

This point is not suggesting a change is necessary; its purpose is to make sure you know what the Drupal.org Composer façade does, and that, when adding core requirements also in the composer.json file, it is necessary to double check they are compatible with the core requirements in the .info.yml file.

I cannot think of any workflow where Composer is not used to gather the projects necessary for a site, and their dependencies, considering that Composer is required by Drupal core to gather its dependencies, but that does not mean that having the core requirements also in the composer.json file is not necessary in specific cases.

"php": "^8.1",

Drupal core 10.2.x already requires PHP 8.1. That line is not necessary.

sudc.services.yml

Services can be autowired starting with Drupal 9.3.x. This means that it is no longer necessary to give a list of service arguments in the .services.yml file; they will be retrieved from the constructor definition.

Re-checked 3.0.x at 6b8b268 against my review in #4. The JWT payload, the signing key, the Referer based UID and the route permissions are all fixed. Four things left, plus a naming suggestion.

Why does the ClientException handler read the whole response body?

In RestCalls::exeHttpClient() and exeHttpClientSuGpt() it fills a body key that neither resultsByPost() nor getAPIParams() reads on that path, since both only return status and message when the call fails.

The uid is not validated against repeater_data

resultsByPost() forwards a client-supplied uid to SearchUnify with the site's access_token without validating it against the configured repeater_data. Other paths (content(), getsearchjwt()) read the UID from config; this one trusts the request. A caller can target an unconfigured search instance, and a missing uid triggers an undefined-array-key notice with a NULL UID sent upstream.

num_fields and repeater_data can disagree

In SuResultController::content() the loop runs on the num_fields config value but reads $repeater[$i], while SuConfigForm::addMore() saves num_fields on the Add click, before the form is submitted. Click Add, leave the page without saving, and /searchunify/{dynamic_path} emits "Undefined array key" on PHP 8. Looping on repeater_data covers both cases and makes num_fields unnecessary.

getAPIParams() throws a TypeError

property_exists() gets the return of json_decode($requestData). An empty body, null, an array or a number gives NULL, array or int, which property_exists() rejects on PHP 8. That is a 500 for any logged in user.

SudcHooks and SudcHooks1 say nothing about what they hold

You have to open both to find out that one implements hook_theme() and the other hook_help(), and the 1 suffix gives no clue at all. Naming them after what they implement would help, and since a single class can carry several #[Hook] attributes, merging them is also an option.

The summary still mentions the Key module

It says "Key module integration for secure API token storage". There is no drupal/key or key.repository anywhere in the branch, and provision_key and access_token are plain type: string in the schema. Implement it or fix the summary.

Your pipeline never runs

GitLab answers "Unable to run pipeline. Project project/gitlab_templates file .gitlab-ci/drupal_project.yml does not exist". Worth fixing, so you get that feedback before publishing a release.

Coding standards

Still off, for example: the @var blocks added in #21 have no short description, the concatenation in SEARCH_RESULT_API_PATH is pointless, and getAPIParams() is not lowerCamel. Also sudc.links.task.yml still declares sudc.help, whose route was removed in the last commit.

Re-checked 3.0.x at 33bd2482. Five of the seven points in #25 are fixed, and fixed well. Good job. Two small things remain, nothing that blocks the security review itself, but they matter.

The pipeline still does not run

Pipeline #920046, created on 33bd2482 shortly before #26, failed with 0 jobs, like every pipeline on this project. So "the pipeline is now running successfully" does not match what the pipelines page shows. The error message on that pipeline tells you where to look, I will let you dig into it.

The summary still mentions the Key module

Same as #4 and #25: "Key module integration for secure API token storage", with no drupal/key or key.repository anywhere in the branch. Implement it or fix the summary.

Two leftovers from the merge: SudcHooks1.php is now a file with no class in it and can simply be deleted, and the num_fields key in the config schema and config/install is dead now that the form tracks the counter in form state, no code reads or writes that config key anymore.

One friendly tip, since you are clearly moving fast: take a moment to test and double check each change before replying to the ticket. This process evaluates the code, but also how rigorously you work as a future maintainer.

I am going to ask some questions about the used code. No change is expected in project files; only answers are expected by this review.

    $form['body']['provision_key'] = [
      '#type' => 'password',
      '#title' => $this->t('Provision Key'),
      // phpcs:ignore Generic.Files.LineLength
      '#description' => $this->t('Provision Key from SearchUnify app. Leave blank to keep the current value.'),
      '#size' => 64,
      '#required' => empty($suConfigs->get('provision_key')),
      '#maxlength' => 200,
      '#attributes' => ['autocomplete' => 'off'],
    ];

Do you see any difference between using a password form element for an account password and using it for a provision key?

    $userInput = $form_state->getUserInput();
    if (isset($userInput['body']['items_fieldset'])) {
      array_splice($userInput['body']['items_fieldset'], $removeIndex, 1);
      $userInput['body']['items_fieldset'] = array_values(
        $userInput['body']['items_fieldset']
      );
      $form_state->setUserInput($userInput);
    }

Since $form_state->getUserInput() returns an array reference, is calling $form_state->setUserInput($userInput) necessary?

    $values = $form_state->cleanValues()->getValues();
    if (!isset($values['body'])) {
      return;
    }

Why does the code call cleanValues()?

  public function __construct(
    protected CommonCalls $ccall,
    protected RestCalls $rcall,
    protected RequestStack $requestStack,
    protected PrivateKey $privateKey,
  ) {}

In a controller, is using a property for RequestStack necessary?

$jwtSecretKey = hash_hmac('sha256', $this->privateKey->get(), $accessToken);

In that code, is $jwtSecretKey set to a secret key?

/**
 * @file
 * Hook implementations for the SearchUnify Drupal Connector module.
 *
 * All hooks are implemented via attribute-based hooks in
 * \Drupal\sudc\Hook\SudcHooks.
 */

Does Drupal 10.3, the minimum Drupal version required by this project, support OOP hooks?

/**
 * Provides common utility calls such as rendering the module footer.
 */

Why does the project use a service for rendering a footer which is rendered by a controller?

    if (is_object($payloadData) &&
      property_exists($payloadData, 'streaming') &&
      $payloadData->streaming) {
      return new JsonResponse($resAry['body']);
    }

Following Drupal coding standards, what is the correct way to format code for control structures?

Why does the project use a template file for what returned by hook_help()?

The differences with an account password are:

• An account password is supposedly changed by a single person (the person who created an account) while the setting for a module could be potentially changed by more people, when the site is not a personal site.
• To change an account password is necessary to first enter the current password. Instead, the setting used by this project could be changed without knowing its current value.

So, what is using a password form element trying to achieve, in this case? Was the user experience considered?

FormState::getUserInput() returns the internal $input array by value, not by reference.

Is that what FormState::getUserInput() code says?
Most importantly, what does the following note in that documentation page tell us? (Emphasis is mine.)

These are raw and non validated, so should not be used without a thorough understanding of security implications. In almost all cases, code should use self::getValues() and self::getValue() exclusively.

5. Is $jwtSecretKey set to a correct secret key?
No — the arguments to hash_hmac() are inverted.

The question was In that code, is $jwtSecretKey set to a secret key? It was not asking whether $jwtSecretKey is set to a correct secret key. Furthermore, the answer to my question does not depend on the arguments passed to hash_hmac().

Rephrasing the question, in the following code is $jwtSecretKey, a variable whose name makes think the variable stores a JWT secret key, set to a secret key?

$jwtSecretKey = hash_hmac('sha256', $this->privateKey->get(), $accessToken);

I could ask the same question for the following code, which is the code the module is currently using.

$jwtSecretKey = hash_hmac('sha256', $accessToken, $this->privateKey->get());

FormState::cleanValues() removes Drupal's internal form-tracking fields from the values array before getValues() is called. These internal fields include form_build_id, form_token, form_id, submit button labels (op), and values from structural elements such as #type => 'actions'. Without this call, those keys would be present in $values alongside the actual field data and could be accidentally stored to configuration. Calling cleanValues() at the start of submitForm() is standard Drupal form practice.

The project code expressly accesses specific values from $values, and those values are not the ones FormState::cleanValues() would remove. So, what is the purpose of calling FormState::cleanValues() which removes values the code is already ignoring?

This is an over-engineered abstraction for a task performed in a single location. The same result could be achieved with a few lines inside the form class itself, which already has access to the service container.

That is correct. So, why is the project using a service which is used only by the form class, and which is adding more code than necessary?

Correct Drupal coding standard formatting for multi-line control structures
Drupal coding standards require that when a control structure condition spans multiple lines, each condition is indented by two spaces, and the closing parenthesis and opening brace appear together on their own line.

Can you point out which part of the Drupal coding standards say that?

There is no compelling reason to do so, and it introduces several problems.

About that code, I said:

To show a help page, Drupal core provides at least two ways: The easier is implementing hook_help(); the more complex is using help topics. Both are documented in Help and documentation.
In both the cases, a contributed project should not need a specific route/controller just to show its own help.

The answer to that implied that the code was changed to fix what I pointed out, but the code was never changed. Your last comment says there is no compelling reason for not changing it.

The #[Hook] attribute-based OOP hook system was introduced in Drupal 10.2 as experimental. It did not become a stable, supported API until Drupal 11.0. This module declares core_version_requirement: ^10.3 || ^11 || ^12.

No, #[Hook] is an attribute that can only be used with Drupal 11.1.x. Drupal 10.3.x will ignore it. In fact, the documentation page for Drupal\Core\Hook\Attribute\Hook says:

This class will not have an effect until Drupal 11.1.0.

Given that, how can the project work in Drupal 10.3.x and any Drupal version before Drupal 11.1.x?

The most important questions: Did you use AI to answer my questions? Are you using AI to write code for this project?