Google V3 Glossary

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, enable GitLab CI for the project, and fix what reported from the phpcs job. This help to fix most of what reviewers would report.
• For the time this application is open, only your commits are allowed. No other people, including other maintainers/co-maintainers can make commits.
• 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.
• Nobody else will get the permission to opt projects into security advisory policy. If there are other maintainers/co-maintainers who will to get that permission, they need to apply with a different module.
• 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. The configuration used for these tools needs to be the same configuration used by GitLab CI, stored in the GitLab Templates repository.
• 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 → .

• 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/LanguageGlossaryForm.php

    catch (\Exception $e) {
      $this->logger('google_v3_glossary')->error($e->getMessage());
    }

The $message parameter passed to the LoggerInterface methods must be a literal string that uses placeholders. It's not a translatable string returned from t() or $this->t(), a string concatenation, nor the value returned from a function/method.

  /**
   * {@inheritdoc}
   */
  public function submitForm(array &$form, FormStateInterface $form_state): void {
    // Empty.
  }

If the submission handler is not necessary, that method can be removed, since the parent class already has an empty submitForm(). The purpose of forms is submitting data. What does this form exactly do, if it does not store the submitted data?

src/Controller/GlossaryController.php

Since that class does not use any 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.

    $build['config_info'] = [
      '#type' => 'item',
      '#markup' => $this->t(
        "Be sure to configure the Google V3 Glossary settings by visiting the <a href='@config_form'>configuration form</a>.",
        ['@config_form' => Url::fromRoute('google_v3_glossary.settings')->toString()]
      ),

The correct placeholder for URLs is :variable as shown in the documentation for FormattableMarkup::placeholderFormat(). That is the placeholder used also by Drupal core code for URLs that take to drupal.org.

$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>';

google_v3_glossary.install

There is no need to delete the configuration objects used by the module, when a module is uninstalled: That is already done by Drupal core.

google_v3_glossary.module

      $output .= '<dt>' . t('Configuration') . '</dt>';
      $output .= '<dd>' . t('Configure your Google Cloud Translation API settings and glossary management options.') . ' ';
      $output .= Link::fromTextAndUrl(t('Configure the module'), Url::fromRoute('google_v3_glossary.settings'))->toString() . '</dd>';
      $output .= '</dl>';

Translatable strings use placeholders, instead of concatenating strings.

1. FILE: google_v3_glossary.module

/**
 * @file
 * Primary module hooks for Google V3 Glossary module.
 */

Drupal does not have primary and secondary hooks. Instead of that, it is preferable to use the usual description: “Hook implementations for the [module name] module”, where [module name] is the name of the module given in its .info.yml file.

2. FILE: README.md

The README file is missing the required sections → - Configuration.

Thanks for the feedback. Below are the original comments and my comments follow.

/**
   * {@inheritdoc}
   */
  public function submitForm(array &$form, FormStateInterface $form_state): void {
    // Empty.
  }

If the submission handler is not necessary, that method can be removed, since the parent class already has an empty submitForm(). The purpose of forms is submitting data. What does this form exactly do, if it does not store the submitted data?

Yes, the submitForm() is required since it is extending FormBase which implements FormInterface which requires submitForm().

DynamicLanguageGlossaryForm extends LanguageGlossaryForm which simply holds some common functionality from a previous iteration of the module. Technically, for your info, this is valid code and submitForm() is required in this case although the entire LanguageGlossaryForm class is no longer necessary.
Since I'm now using the new DynamicLanguageGlossaryForm I can consolidate the functions and remove LanguageGlossaryForm, which I did.

src/Form/LanguageGlossaryForm.php

    catch (\Exception $e) {
      $this->logger('google_v3_glossary')->error($e->getMessage());
    }

The $message parameter passed to the LoggerInterface methods must be a literal string that uses placeholders. It's not a translatable string returned from t() or $this->t(), a string concatenation, nor the value returned from a function/method.

The method $e->getMessage() always is a string and I'm not translating a error message so there is nothing technically wrong with this. The only thing to change would be to assign $e->getMessage() to a variable and replace $e->getMessage() with that variable which is an unnecessary variable assignment.

$build['config_info'] = [
      '#type' => 'item',
      '#markup' => $this->t(
        "Be sure to configure the Google V3 Glossary settings by visiting the <a href='@config_form'>configuration form</a>.",
        ['@config_form' => Url::fromRoute('google_v3_glossary.settings')->toString()]
      ),

The correct placeholder for URLs is :variable as shown in the documentation for FormattableMarkup::placeholderFormat(). That is the placeholder used also by Drupal core code for URLs that take to drupal.org.

Updated, but the result is the same since the URL is NOT user input the filtering from :variable should not be needed.

google_v3_glossary.install
There is no need to delete the configuration objects used by the module, when a module is uninstalled: That is already done by Drupal core.

Maybe, but there's no harm in doing clean up ourselves.

The value returned by that method is not a literal string; it is the value of a property. Even if you are not translating that string, it is Drupal core that translates it, and it needs a literal string.

It could be valid code, but the purpose of a form is storing the submitted values, which is not something this module is doing with that form.

There would be no warn in creating a database table with hook_install() instead of using hook_schema() too, but that is a wrong use of the Drupal API, similar to deleting values Drupal core deletes itself.

:variable is used even when the URL is not user input. See what Drupal core does with drupal.org links.

Also, the controller class is still extending ControllerBase, but it does not use any of the parent class methods; hook_help() is still concatenating a translatable string with a dynamic string instead of using placeholders in the translatable string.

Also, the controller class is still extending ControllerBase, but it does not use any of the parent class methods;

Extending Controller is required for this to work and doesn't matter if I'm not currently using any methods from the parent Controller class. It simply does NOT work without extending ControllerBase.

hook_help() is still concatenating a translatable string with a dynamic string instead of using placeholders in the translatable string.

Updated

The value returned by that method is not a literal string; it is the value of a property.

$e->getMessage() is not a value of a property and is a method that returns a string if this is the code you are talking about and core code uses writes it this way as well $this->logger->error($e->getMessage());

Made changes

There would be no warn in creating a database table with hook_install() instead of using hook_schema() too, but that is a wrong use of the Drupal API, similar to deleting values Drupal core deletes itself.

Not sure what this is talking about, but if it has anything to do with the .install file then this is valid and is correctly using the API since we are creating configuration with `GoogleV3GlossaryConfigForm` and explicitly deleting that config when the module is uninstalled.

Extending Controller is required for this to work.

No, it is not required. Controller classes do not need to have a parent class at all; that is explained on
Routing API.

      $output .= '<dd>' . t('@link', ['@link' => Link::fromTextAndUrl('Configure the module', Url::fromRoute('google_v3_glossary.settings'))->toString()]) . '</dd>';
      $output .= '</dl>';

Translators cannot translate a string which just contains a placeholder. Furthermore, a single placeholder does not allow them to insert the placeholder in the right place.
'Configure the module' is not a translatable string. Strings shown in the user interface must be translated.

The correct code to use is not different from the following code, used by the module.

 $this->t(
        "Be sure to configure the Google V3 Glossary settings by visiting the <a href=':config_form'>configuration form</a>.",
        [':config_form' => Url::fromRoute('google_v3_glossary.settings')->toString()]
      ),

/**
 * Implements hook_uninstall().
 */
function google_v3_glossary_uninstall() {
  // Delete 'google_v3_glossary.settings' configuration.
  $config_factory = \Drupal::configFactory();
  // Get editable configuration for this module.
  $config = $config_factory->getEditable('google_v3_glossary.settings');
  // Delete the configuration.
  $config->delete();
}

That code is still not necessary, since Drupal core remove the configuration objects used from a module that is uninstalled.

Updated

Thank you for your contribution and for your patience with the review process!

I am going to update your account so you can opt into security advisory coverage any project you create, including the projects you already created.

These are some recommended readings to help you with maintainership:

• Dries → ' post on Responsible maintainers
• Maintainership →
• Git version control system →
• Issue procedures and etiquette →
• Maintaining and responding to issues for a project →
• Release naming conventions → .

You can find more contributors chatting on Slack → or IRC → in #drupal-contribute. So, come hang out and stay involved → !
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

I thank the dedicated reviewers as well.