Yotpo Drupal 10

Created new branch 2.x https://git.drupalcode.org/issue/yotpo-3450844/-/tree/2.x?ref_type=heads

Assigned to @tunic to review the changes before the new release.

New release 2.0.0 compatible with Drupal 10.

Some comments on the source code:

File src/Commands/YotpoCommands.php

    if (empty($options['external_id'])) {
      $this->logger->notice('The external_id param is mandatory');
    }

A warning is triggered but the function execution continues, if it is mandatory would make sense to throw an exception or exiting the method? Or if it is really not mandatory then remove the notice.

File src/YotpoClient.php

There several calls to $this->logger->notice that I would say they should be info. For example:

  public function getYotpoProducts() {
    $products = $this->yotpoClient->getYotpoProducts();
    $this->logger->notice('Products yotpo: {products}', ['products' => print_r($products, TRUE)]);
  }

This is a normal operation with normal execution, no special conditions. However, notice logs are for "Normal but significant conditions".

Info are just "informational messages" and I think it is a better fir for those logs that inform on actions taken by the module.

This comment is mismatched:

  /**
   * Logger.
   *
   * @var \Drupal\Core\Config\ImmutableConfig
   */
  protected ImmutableConfig $config;

Additionally, the comments are not explanatory. For example,

  /**
   * Default options.
   *
   * @var array
   */
  protected array $defaultOptions = [

The comment says the same thing as the variable name, so no additional information is provided. I would be nice to say something like "Default options for the Guzzle's HTTP client"

I'm not sure about addDefaultOptions. The code is:

    foreach ($options as $key => $value) {
      if (isset($this->defaultOptions[$key]) && is_array($options[$key]) && is_array($this->defaultOptions[$key])) {
        $options[$key] = array_merge($this->defaultOptions[$key], $value);
      }
    }
    $options += $this->defaultOptions;

So it seems $this->defaultOptions is merged into $options twice.

Some expressions can be simplified for readability:

 $headers = isset($options['headers']) ? array_merge($options['headers'], $additional_headers) : $additional_headers;

to

$headers = ($options['headers'] ?? []) + $additional_headers;

This operator is available since PHP 7 so it should be safe to use.

I would split callYotpoApi in two functions to lower the complexity, putting the code that actually requests the info in another function (the code under if (empty($cached_response) || empty($cached_response->data)) {).

In several places the method callYotpoApi is called. This method can throw an exception but the calls are not wrapped in a try-catrch, is this intended? If this is only used inside drush commands I think drush catches the exception and nicely ends the command, so it would be ok.

Pending to review comments from @tunic