[1.x] Video Toolbox

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.

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.

• 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

src/Controller/VideoAutoCompleteController.php

  /**
   * {@inheritdoc}
   */
  public function __construct(

{@inheritdoc} isn't used for the class constructor.

src/Controller/VideoToolBoxReportController.php

  /**
   * Constructor to initialize Services.
   *
   * @param \Drupal\video_toolbox\VideoHandlerInterface $videoHandler
   *   Video Handler Service.
   */
  public function __construct(VideoHandlerInterface $videoHandler) {
    $this->videoHandler = $videoHandler;
  }

For methods, like functions, the first word in the short description and in the long description is a verb.

        new TranslatableMarkup('<a class="button button--primary" href=":link">@download</a>',
          [
            ':link' => $urlDownload,
            '@download' => $this->t('Download'),
          ]
        ),
      ];
    }

In a string containing a placeholder and HTML markup, the only part that is translatable is the HTML markup. If users who translate aren't supposed to change the HTML markup (which translators would do only if it's necessary to adapt it to the translation language), that string should not be made translatable.

src/Form/VideoToolBoxConfigForm.php

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

That property is already defined by the parent class, which uses MesssengerTrait.

    $form['allowed_extensions'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Allowed extensions'),
      '#description' => $this->t('This is for the video extensions that you can allowed, separate them witha comma'),
      '#default_value' => $config->get('allowed_extensions') ?? 'mp4',
    ];

    return parent::buildForm($form, $form_state);
  }

  /**
   * {@inheritDoc}
   */
  public function validateForm(array &$form, FormStateInterface $form_state) {

    // Check for extensions.
    $ext = explode(',', preg_replace('/[ ]+/', '', $form_state->getValue('allowed_extensions')));

    if (!$ext) {
      $form_state->setErrorByName('allowed_extensions', $this->t('Empty please fill out'));
    }
    if (count(array_intersect($ext, self::EXT)) != count($ext)) {
      $form_state->setErrorByName('allowed_extensions', $this->t('Invalid extension!'));
    }
  }

If users need to enter a value for a form field, that field should be set as required. The validation handler would then check if what entered is valid.

  public function submitForm(array &$form, FormStateInterface $form_state) {
    // Save configuration on submit.
    $config = $this->config(self::VIDEO_TOOLBOX_SETTINGS);
    $config
      ->set('allowed_extensions', $form_state->getValue('allowed_extensions'));
    $config->save();
    // PLACEHOLDER MESSAGE.
    $this->messenger()->addMessage($this->t('SUBMITTED'));

    parent::submitForm($form, $form_state);
  }

A message is already shown by the parent class. To override that message, the parent method should not be called.

video_toolbox.install

/**
 * Uninstall function.
 */
function video_getter_uninstall() {

}

That isn't the correct documentation comment.
If the module doesn't need to do anything when it's uninstalled, it's not necessary to implement that hook.

video_toolbox.module

      $output = '';
      $output .= '<h3>' . t('About') . '</h3>';
      $output .= '<p>' . t('This module aims to improve video management and offered useful tools, like upload, watch, edit, download and convert videos.') . '</p>';
      $output .= '<p>' . t('For more information about the module check the README files and module page:');
      $output .= ' <a href="https://www.drupal.org/project/video_toolbox">https://www.drupal.org/project/video_toolbox</a>' . '</p>';
      $output .= '<h3>' . t('Important Routes') . '</h3>';
      $output .= '<dl>';
      $output .= '<dt>' . t('Upload videos') . '</dt>';
      $output .= '<dd>' . '<a href="/upload_video">/upload_video</a>' . '</dd>';
      $output .= '<dt>' . t('Video edit (video_id dependant for each video)') . '</dt>';
      $output .= '<dd>';
      $output .= '/video/{video_id}/edit';
      $output .= '</dd>';
      $output .= '<dt>' . t('List of all videos') . '</dt>';
      $output .= '<dd>' . '<a href="/admin/reports/uploaded_videos">/admin/reports/uploaded_videos</a>' . '</dd>';
      $output .= '</dl>';
      $output .= '<h3>' . t('Configuring Video Toolbox') . '</h3>';
      $output .= '<p>' . t('Configure module options in the <a href="/admin/config/media/video_toolbox">/admin/config/media/video_toolbox</a> admin route') . '</p>';
      $output .= '<h3>' . t('Maintainers') . '</h3>';
      $output .= '<ul>';
      $output .= '<li>' . '<a href="https://www.drupal.org/u/leoalcci">LeoAlcci</a>' . '</li>';
      $output .= '<li>' . '<a href="https://www.drupal.org/u/jacoboa">Jacoboa</a>' . '</li>';
      $output .= '<li>' . '<a href="https://www.drupal.org/u/juancec">juancec</a>' . '</li>';
      $output .= '<li>' . '<a href="https://www.drupal.org/u/arturo1007">Arturo1007</a>' . '</li>';
      $output .= '<li>' . '<a href="https://www.drupal.org/u/cinarb">cinarb</a>' . '</li>';
      $output .= '<li>' . '<a href="https://www.drupal.org/u/erikaagp">erikaagp</a>' . '</li>';
      $output .= '</ul>';

Links are included in the translatable text. See what Drupal core does.

It's not necessary to have a README.txt and a README.md file. Choose one, but keep in mind that the current Drupal.org coding standard says that documentation files should use a .txt extension.

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.