[1.0.x] Oorly Accessibility Widget

Thank you for applying!

Before giving links helpful to understand how the review process works, what to expect from a review, and what to do to avoid a review takes more time than needed, I would like to thank all the reviewers for the work they do.
These applications are volunters-driven, which also means it is not possible to predict when an application will be marked fixed and the applicant will get the permission to opt projects into security advisory policy. While we aim to make an application as quick as possible, it is also important for us that more people review the project used for an application. In this way, we make sure applications do not miss some important points that should be instead reported.
Applications are not meant to be complete debugging sessions that eliminate every existing bug, though. I apologize if sometimes applications seem to go into too-detailed reviews.

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 enable GitLab CI for the project and fix the PHP_CodeSniffer errors/warnings it reports.
• 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 will not be changed by this application; once this application is closed, you will be able to change the project status from Not covered to Opt into security advisory coverage. This is possible only 14 days after the project is created.
  
  Keep in mind that once the project is opted into security advisory coverage, only Security Team members may change coverage.
• Only the person who created the application will get the permission to opt projects into security advisory coverage. No other person will get the same permission from the same application; that applies also to co-maintainers/maintainers of the project used for the application.
• 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 project moderator before posting the first comment on newly created applications. Project moderators 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.

GitLab CI jobs should be enabled. In that way, most of what is reported by reviewers would be fixed before starting reviews.

FILE: oorly.info.yml

package: 'Custom'

Custom is not a package value used for projects hosted on drupal.org. That is not a mandatory value, and it can be omitted.

• 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

phpcs.xml.dist

<?xml version="1.0" encoding="UTF-8"?>
<ruleset name="oorly">
  <description>PHP CodeSniffer configuration for the Oorly module.</description>

  <file>.</file>

  <arg name="extensions" value="php,module,inc,install,test,profile,theme,css,info,txt,md,yml"/>
  <arg name="colors"/>
  <arg value="sp"/>

  <exclude-pattern>vendor/</exclude-pattern>
  <!-- Written by the GitLab CI template at job runtime, not part of the module. -->
  <exclude-pattern>gitlab_templates_version.txt</exclude-pattern>

  <rule ref="Drupal"/>
  <rule ref="DrupalPractice"/>
</ruleset>

Since the project is using a config file for PHP_CodeSniffer, that file should be tailored for the project. The project does not have any .inc, .profile, nor .theme file, so those extensions can be removed from <arg name="extensions">. Furthermore, PHP_CodeSniffer no longer check for plain text files, so css, info, txt, md, and yml can be removed as well.

<file>.</file> is not necessary for PHP_CodeSniffer to work. In fact, the default config file GitLab CI would use on git.drupalcode.org does not have that line.

src/Form/OorlySettingsForm.php

/**
 * Settings form for the Oorly widget.
 *
 * The elements are bound to the config object with '#config_target', so
 * ConfigFormBase reads the stored values and writes them back on submit; the
 * form declares no submitForm() of its own. '#config_target' is available from
 * Drupal 10.2, which is the oldest version this module supports.
 *
 * User facing strings are written in English as the source language; the
 * Turkish and German translations come from the translations/ directory.
 */

The long description is not necessary. It seems a explanation an AI would produce.
Is the code produced with the assistance of an AI?

    if ($code === '') {
      $form['preview']['value'] = [
        '#type' => 'html_tag',
        '#tag' => 'em',
        '#value' => $this->t('No site code entered, the script is not added.'),
      ];
    }

The translatable string is an example of comma-split sentence: A correct sentence uses either a period after the first sentence, or a semicolon.

  public function validateForm(array &$form, FormStateInterface $form_state): void {
    parent::validateForm($form, $form_state);

    // Only the free text field is validated here; the checkbox is constrained
    // by the form API and by the config schema already.
    $code = trim((string) $form_state->getValue('code'));
    if ($code !== '' && !preg_match('/^[A-Za-z0-9_-]+$/', $code)) {
      $form_state->setErrorByName('code', $this->t('The site code may only contain letters, digits, hyphens and underscores.'));
    }
  }

      '#config_target' => new ConfigTarget(
        OorlyWidget::SETTINGS,
        'code',
        toConfig: static fn (string $value): string => trim($value),
      ),

There is no need to use a validator handler to check the submitted value with a regular expression: A textfield form element can use #pattern. Since a space is not allowed by the regular expression, there is no need to strip the spaces before saving the value in the config file.

src/Hook/OorlyHooks.php

/**
 * Hook implementations for the Oorly module.
 *
 * Drupal 11.1 and later call these methods through the #[Hook] attributes.
 * Drupal 10 ignores the attributes and reaches the same object through the
 * procedural wrappers in oorly.module, which are marked #[LegacyHook] so that
 * newer versions do not run them a second time.
 */

That seems another explanation an AI would add.
That long description holds true for every hook class, so it does not need to be added. A comment is for what is specific for the used code, which would eventually remind the maintainers why the code needs to be written the way it is written.

    // Rebuild cached pages whenever the settings change. The
    // config:oorly.settings tag is added before the 'enabled' check; otherwise
    // changes made while the widget is turned off would not invalidate the
    // page cache.
    $attachments['#cache']['tags'] = Cache::mergeTags(
      $attachments['#cache']['tags'] ?? [],
      $config->getCacheTags()
    );

For that code, I am going to just ask a question; no change is required. Is that comment really necessary?

tests/src/Kernel/OorlySettingsFormTest.php

That class is testing what Drupal core does. Tests for a project should test what that project does, not what Drupal core does.

The cache tag is added before the 'enabled' check seems pretty obvious. Nobody would rewrite the code to the following one.

if (!$config->get('enabled')) {
  return;
}

$attachments['#cache']['tags'] = Cache::mergeTags($attachments['#cache']['tags'] ?? [],$config->getCacheTags());

Every line after a return would not run.

I would rather say Cache tags are added to invalidate the widget markup when the widget is turned back on.
That still does not hold true, since no tag is invalidated when the widget is disabled or enabled. The project should add a cache tag, which is then explicitly invalidated when the widget is turned off or on. (There is no need to use a custom cache tag, since Drupal core should have a cache tag that is invalidated when a configuration object is changed.)

As per Policy on the use of AI when contributing to Drupal, using an AI tool to generate code must be disclosed. In the case of a project, that must be done in the project page.

A list of cache tags used by Drupal core is listed in Cache tags.