[2.0.x] U.S. National Weather Service

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.

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

I am changing priority as per Issue priorities.

The project as a whole: The project's default branch is master. This should be changed to the 2.0.1 branch or another branch. Also, Drupal doesn't allow "master" as a branch name anymore.

src/ForecastClient.php

Use constructor property promotion. Also, constructor doc block comments are optional and can be removed.

getPoints returns NULL on failure. That's procedural programming style. In object-oriented code, you should throw an exception so that it can be caught or handled by the caller or subclasses. Using an exception unique to the module is desirable if no core one fits, such as NwsWeatherException or something. An example is how the Webform module uses WebformException. Don't forget to add @throws to function comments that throw an exception.

These might also apply to other files.

src/Form/NwsWeatherAdminSettings.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.

src/Plugin/Block/MultiDayForecast.php

The indentation isn't quite right here:

#[Block(
   id: 'multidayforecast',
   admin_label: new TranslatableMarkup('Multi Day Forecast'),
  )]

Also, use constructor property promotion, etc. And if you use autowiring of services, you won't need the create function, I think?

A release branch name should end with the literal .x; names like 1.0.0 or 2.1.0-beta1 are for tags.

(e.g. 2.0.x or 2.x)

1. FILE: README.md

The README file is missing the required section - Requirements.

2. FILE: nws_weather.module

For a new module that aims to be compatible with Drupal 10 and Drupal 11, I would rather implement hooks as class methods as described in Support for object oriented hook implementations using autowired services.
It would require increasing the minimum Drupal 10 version supported, but Drupal 10.1 is no longer supported.

3. FILE: templates/nws-weather-forecast-block.html.twig

Strings used in template files must be translatable.

4. FILE: src/Form/NwsWeatherAdminSettings.php

Starting with Drupal 10.2, ConfigFormBase::__construct() requires two parameters and #config_target requires at least Drupal 10.3.

You need to update the drupal core requirement in nws_weather.info.yml file.

Rest seems fine to me.

Please wait for other reviewers and Project Moderator to take a look and if everything goes fine, you will get the role.

Fixed branch name in issue summary

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

public function buildForm(array $form, FormStateInterface $form_state): array {
  $form = [];

$form is the parameter passed to the method. It is not initialized to an empty array, or it loses all the passed values.

    // Check for empty directory.
    if (empty($value)) {
      $form_state->setError($element, $this->t('Image override directory cannot be empty.'));
      return;
    }

If a submitted value is required, it is sufficient to set the form element as required. It is correct to verify the submitted value does not contain just spaces, since in that case Drupal core would consider that as a valid string, but checking the submitted value is not empty is not necessary. Further more empty() returns TRUE for values you would not expect it to return TRUE.

Probably, there are more values which should not be accepted. For example, a string containing only slashes (////////////////) should probably be rejected. I imagine the absolute path for the submitted value should be checked too, or a value like ../../../../../../../../../../../../../../ would be accepted, although points to a directory that does not exist.
For the first case, an error message like Image override directory does not exist or is not writable. Please create it manually inside the public files folder. does not say exactly what is wrong with the submitted value. (It is not that the directory does not exist; that directory name is not valid.)

    $lat = $form_state->getValue('lat');
    if (!is_numeric($lat) || $lat > 90 || $lat < -90) {
      $form_state->setErrorByName('lat', $this->t('Latitude value must be numeric and between -90 and 90.'));
    }

For values that are expected to be integers, Drupal has a specific form element, which also allow to set a minimum and a maximum value.
Those lines and the next lines are not necessary.

src/Plugin/Block/MultiDayForecast.php

  public function __construct(
    array $configuration,
    $plugin_id,
    $plugin_definition,
    protected readonly ConfigFactoryInterface $configFactory,
    protected ForecastClientInterface $forecastClient,
    protected ForecastDataProcessorInterface $dataProcessor,
    protected ModuleExtensionList $moduleExtensionList,
    protected AccountProxyInterface $account,
    protected KillSwitch $killSwitch,
    LoggerChannelFactoryInterface $logger_factory,
  ) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
    $this->logger = $logger_factory->get('nws_weather');
  }

As with $configFactory, it is suggested to use a property for the logger factory instead of the logger instance used by the class.

      catch (NwsWeatherException $e) {
        $this->logger->warning($e->getMessage());
      }

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

For exceptions, it is probably better to use Error::logException(), which allows understanding exactly what caused the exception.

src/ForecastClient.php

  /**
   * Gets the daily forecast for a specific location.
   *
   * @param float $lat
   *   The latitude of the location.
   * @param float $lon
   *   The longitude of the location.
   *
   * @return array
   *   The processed forecast data.
   *
   * @throws NwsWeatherException
   */
  public function forecastByDay(float $lat, float $lon): array {

Methods defined in interfaces, or inherited by a parent class, use a simpler documentation comment, which eventually can use @throws tags, when those are not already used by the interface or the parent class.
@throws tags need to describe when the exception is thrown.

src/Form/NwsWeatherAdminSettings.php

  /**
   * Return a render array for the nws_weather block.
   */

Methods defined in a parent class or in an interface have a different documentation comment.

  /**
   * Validate image_map directory.
   *
   * @param array $element
   *   The element array.
   * @param \Drupal\Core\Form\FormStateInterface $form_state
   *   Provides an interface for an object containing
   *   the current state of a form.
   *
   * @return void
   *   Returns void.
   */

Documentation comments for form validation handlers just need the short description, which needs to describe which form or form element they validate.
@return void is never used when a function/method does not have any return value. It is omitted, together its description.

src/Plugin/Block/MultiDayForecast.php

  public function __construct(
    array $configuration,
    $plugin_id,
    $plugin_definition,
    protected readonly ConfigFactoryInterface $configFactory,
    protected ForecastClientInterface $forecastClient,
    protected ForecastDataProcessorInterface $dataProcessor,
    protected ModuleExtensionList $moduleExtensionList,
    protected AccountProxyInterface $account,
    protected KillSwitch $killSwitch,
    protected LoggerChannelFactoryInterface $loggerFactory,
  ) {
    parent::__construct($configuration, $plugin_id, $plugin_definition);
  }

Why only one parameter is defined protected readonly?

    // Disable page cache for anonymous users until Issue #2352009 is resolved.
    // See issue https://www.drupal.org/project/drupal/issues/2352009

There is no need to reference the same issue twice. It is sufficient to use until https://www.drupal.org/project/drupal/issues/2352009 is resolved. (keeping the line 80 characters).

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 also all the reviewers for helping with these applications.