Yotpo Drupal 10

Created on 30 May 2024, 8 months ago

Problem/Motivation

Yotpo API integration Drupal 10

Feature request
Status

Active

Version

1.0

Component

Code

Created by

🇪🇸Spain eduardo morales alberti Spain, 🇪🇺

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Status changed to Needs review 8 months ago
  • Assigned to tunic
  • 🇪🇸Spain eduardo morales alberti Spain, 🇪🇺

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

  • 🇪🇸Spain eduardo morales alberti Spain, 🇪🇺

    New release 2.0.0 compatible with Drupal 10.

  • 🇪🇸Spain tunic Madrid

    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.

  • 🇪🇸Spain eduardo morales alberti Spain, 🇪🇺

    Pending to review comments from @tunic

  • Issue was unassigned.
  • Status changed to Fixed 4 days ago
  • 🇪🇸Spain eduardo morales alberti Spain, 🇪🇺

    Drupal 7 is not supported by Drupal

  • 🇪🇸Spain eduardo morales alberti Spain, 🇪🇺

    Pending solve comments

  • 🇪🇸Spain eduardo morales alberti Spain, 🇪🇺

    @tunic could you review the MR?

  • 🇪🇸Spain eduardo morales alberti Spain, 🇪🇺

    File src/Commands/YotpoCommands.php

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

    Return after log.

        if (empty($options['external_id'])) {
          $this->logger->notice('The external_id param is mandatory');
          return
        }
    
    File src/YotpoClient.php

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

    Notice is the minimal level of the log that is shown without specifying the verbosity.
    To show info messages the command should use "-v".

    This comment is mismatched:

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

    Updated to:

      /**
       * Yotpo settings like the api credentials.
       *
       * @var \Drupal\Core\Config\ImmutableConfig
       */
      protected ImmutableConfig $config;
    
    Additionally, the comments are not explanatory. For example,
      /**
       * Default options.
       *
       * @var array
       */
      protected array $defaultOptions = [
    

    Changed to:

    * 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;
    

    The code overrides the default options if exists on param options if matches the key, if not merge options with default options.
    Refactored to:

        foreach ($options as $key => $value) {
          if (isset($this->defaultOptions[$key]) && is_array($value) && is_array($this->defaultOptions[$key])) {
            $options[$key] = array_merge($this->defaultOptions[$key], $value);
            unset($this->defaultOptions[$key]);
          }
        }
        $options += $this->defaultOptions;
    
    
    Some expressions can be simplified for readability:
     $headers = isset($options['headers']) ? array_merge($options['headers'], $additional_headers) : $additional_headers;
    

    Refactored to:

    $headers = array_merge($options['headers'] ?? [], $additional_headers);
    
    I would split callYotpoApi in two functions to lower the complexity

    Done on MR https://git.drupalcode.org/project/yotpo/-/merge_requests/3/diffs#2389f1...

    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.

    As you said, because it is called from drush it is not necessary to catch the exception.

Production build 0.71.5 2024