[1.0.x] Media Keepeek DAM

I am crediting the reviewers of the past applications.

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

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 smother review.

To reviewers: Please read How to review security advisory coverage applications, What to cover in an application review, and Drupal.org security advisory coverage application workflow.

For the time this application is open, commits on the project used for the application are only allowed from the user who created the application.

Please resolve the following issues at first:

Review of the 1.0.x branch (commit 5566905):

• Your README.md does not follow best practices (headings need to be uppercase). See https://www.drupal.org/node/2181737 .
  • The INTRODUCTION section is missing.
  • The REQUIREMENTS section is missing.
  • The INSTALLATION section is missing.
  • The CONFIGURATION section is missing.
• The media_keepeekdam.module does not implement hook_help(). See https://www.drupal.org/docs/develop/documenting-your-project/module-docu... .
• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: .../modules/custom/media_keepeekdam/README.md
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------
   3 | WARNING | Line exceeds 80 characters; contains 190 characters
  --------------------------------------------------------------------------
  

  Then set the issue back to Needs review.

• What follows is a quick review of the project; it doesn't mean to be complete
• For each point, the review usually shows some lines that should be fixed (except in the case the point is about the full content of a file); it doesn't show all the lines that need to be changed for the same reason
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

      $url = Url::fromRoute('media_keepeekdam.overview', ['media_library_selected_type' => $selected_type_id], ['attributes' => ['target' => '_blank']]);
      $link = Link::fromTextAndUrl(t('@name Import', ['@name' => $global_name]), $url);
      $exposed['#prefix'] = t('%link (After adding new Keepeek media, you should refresh this interface by clicking on the active tab).', ['%link' => $link->toString()]);

The correct way to add dynamic or static links to a translatable string is to add the <a> markup directly in the translatable string, as explained also in Dynamic or static links and HTML in translatable strings, which is for Drupal 7 but still applies to Drupal 8 and Drupal 9.

  $config = Drupal::configFactory()
    ->get('media_keepeekdam.settings');

For obtaining an immutable configuration object, for example when the configuration is only read, the method to call is Drupal::config().

  /** @var \Drupal\Core\Logger\LoggerChannelInterface $logger */
  $logger = \Drupal::service('logger.factory')->get('media_keepeekdam');

The Drupal class has an helper method for this case: Drupal::logger().

      return $this->t('@name @suffix', [
        '@name' => $this->config->get('global_name'),
        '@suffix' => $suffix,
      ]);

That code won't translate the strings contained in the PAGES_TITLES constant nor the value contained in $this->config->get('global_name'). What is translated is the literal string passed as first argument to $this->t(), which can only be translated as '@name @suffix' or '@suffix @name' since it only contains two plaholders and for placeholders only their order can be changed.

class KeepeekSearchForm extends FormBase {

  /**
   * The user data.
   *
   * @var \Drupal\user\UserData
   */
  protected $userData;

  /**
   * The current logged user.
   *
   * @var \Drupal\Core\Session\AccountProxyInterface
   */
  protected $currentUser;

  /**
   * Keepeek search service.
   *
   * @var \Drupal\media_keepeekdam\Service\SearchService
   */
  protected $searchService;

  /**
   * Core messenger.
   *
   * @var \Drupal\Core\Messenger\Messenger
   */
  protected $messenger;

The parent class already has methods to get the current user and and the logger.

  public function __construct(Messenger $messenger, UserData $user_data, AccountProxyInterface $current_user = NULL, SearchService $searchService) {
    $this->messenger = $messenger;
    $this->userData = $user_data;
    $this->currentUser = $current_user;
    $this->searchService = $searchService;
  }

Parameters with default values cannot be added before parameters without default values. The default value isn't even necessary, since create() passes that value.

    $form['keepeek_media_table']['#prefix'] = $result['totalCount'] . ' Result(s)';

Strings visible in the user interface need to be translatable.

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('controller_resolver'),
      $container->get('access_manager'),
      $container->get('current_user'),
      $container->get('request_stack'),
    );
  }

Services don't implement that method.

      $this->loggerFactory->error('API error in @source: %error', [
        '@soucre' => $source,
        '%error' => $errors,
      ]);

There is a typo in the placeholder name.

I will review the project between an hour, less or more.

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('config.factory'),
      $container->get('logger.factory')
    );
  }

Service classes don't use that method. Drupal creates the service calling the class constructor and passing the arguments defined in the media_keepeekdam.services.yml file for that class.

$exposed['#prefix'] = t('<a href="@url">@name Import</a> (After adding new Keepeek media, you should refresh this interface by clicking on the active tab).',

The correct placeholder for URLs starts with a colon.

      $form_state->setErrorByName('authentication', $e->getMessage());

Error messages shown in the user interface must be translatable. $e->getMessage() doesn't return a message that is translated in the language selected for the currently logged-in user or selected for the site using the module.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the Slack #contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
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 all the reviewers.