iLangL module is a plugin for the Translation Management Tool module (tmgmt).
It uses iLangL (https://ilangl.com) service
for automated translation of the content.

Project link

https://www.drupal.org/project/tmgmt_ilangl

Git instructions

git clone --branch 8.x-1.0 https://git.drupalcode.org/project/tmgmt_ilangl.git

Comments

Bohdan Vasyliuk created an issue. See original summary.

podarok’s picture

Assigned: bohdan vasyliuk » Unassigned
Status: Needs review » Reviewed & tested by the community

I've checked code.
Looks good for me.

avpaderno’s picture

Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Needs review

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 Security advisory coverage application workflow.

Since the project is being used for this application, for the time this application is open, only the user who created the application can commit code.

(I will make a review once I return back to home, since I have seen something that should be fixed.)

avpaderno’s picture

Issue tags: +PAreview: security
  • 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
author: iLangL

author isn't a recognized key, in .info.yml files.

core: 8.x
core_version_requirement: ^8 || ^9

core_version_requirement requires Drupal 8.7.7, while semantic versioning requires Drupal 8.8.3, as previous Drupal versions don't handle them.
core should not be used with recent Drupal releases.

Since Drupal 8 isn't supported anymore, I would rather limit the required Drupal versions to Drupal 9, except in the case the module is already used in sites running on Drupal 8. In this case, the minimum Drupal 8 version should still be adjusted.

  /**
   * The HTTP client.
   *
   * @var \GuzzleHttp\ClientInterface
   */
  protected ClientInterface $client;

The property type can only be declared in PHP 8. Drupal 9 still requires PHP 7.3.

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

FormBase has already methods to retrieve configuration objects. That property and the code handling it aren't necessary.

  /**
   * The request stack.
   *
   * @var \Symfony\Component\HttpFoundation\RequestStack
   */
  protected $requestStack;

There are already FormBase::$requestStack and FormBase::getRequest().

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

FormBase uses MessengerTrait, which provides MessengerTrait::$messenger and MessengerTrait::messenger().
The code is duplicating properties already provided from the parent class.

$this->messenger->addStatus('Your message was sent. Thank you.');
    catch (\Exception $e) {
      $this->messenger()->addError($e->getMessage());
      return [];
    }

The first argument passed to MessengerInterface::addError(), MessengerInterface::addMessage(), MessengerInterface::addStatus(), and MessengerInterface::addWarning() must be a translatable string that uses placeholders.

      $this->t('@translator is not available. Make sure it is properly <a href=:configured>configured</a>.',
        [
          '@translator' => $translator->label(),
          ':configured' => $translator->toUrl()->toString(),
        ]
      )

The placeholder for the URL is correct, but it must be used between double quotes. Using it without double quotes is considered a security issue.

          $message = $this->t('Error:') . $e->getMessage();
          $this->logger->error($message);

The $message parameter passed to the LoggerInterface methods must be a literal string that uses placeholders. It's not a translatable string returned from t() or $this->t(), or a string concatenation.

The README.txt file doesn't follow the README template.

avpaderno’s picture

Assigned: avpaderno » Unassigned
Status: Needs review » Needs work
podarok’s picture

@apaderno
Thanks for the review

Just a couple of questions
- how is this related to security?
- wouldn't be good to have this as a post-approval stage?

The reason I'm asking is - I saw these in my review but decided to send them as a suggestion, not blocking security opt-in, which is a part of the current issue queue.

avpaderno’s picture

These applications don't verify only the project is using secure code, but also that the code follows the Drupal coding standards and correctly uses the Drupal API. That is stated in Apply for permission to opt into security advisory coverage / Purpose.

avpaderno’s picture

What these applications don't do is fixing all the bugs in the code. That's something that need to be reported in the project issue queue.

bohdan vasyliuk’s picture

Status: Needs work » Needs review

Thank you for your time and feedback.

The code was updated according to your comments. Please review and let me know your feedback.

avpaderno’s picture

Title: iLangL Translation Provider Plugin for Translation Management Tool (TMGMT) » [D9] iLangL Translation Provider Plugin for Translation Management Tool (TMGMT)
Status: Needs review » Reviewed & tested by the community
$this->messenger()->addError('Error: @message', ['@message' => $e->getMessage()]);

The messages passed to the messenger's methods must be translatable.

avpaderno’s picture

Status: Reviewed & tested by the community » Fixed

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

These are some recommended readings to help with excellent maintainership:

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 too.

podarok’s picture

Status: Fixed » Closed (fixed)

Thank you

avpaderno’s picture

Status: Closed (fixed) » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

avpaderno’s picture

Assigned: Unassigned » avpaderno