[D9] gText

Thank you for applying!
Remember to change status, as in this queue Active tells the reviewers not to review the code, yet.

I couldn't access the PAReview link with https. This worked - http://pareview.net/r/371. There are still a few pending items in there. Can you please take a look at those?

  public function t($string, array $args = [], $options = []) {
    $options['context'] = $this->context;
    return new TranslatableMarkup($string, $args, $options);
  }

The documentation for TranslatableMarkup::__construct() says:

$string should never contain a variable, such as:

new TranslatableMarkup($text);

There are several reasons for this:

• Using a variable for $string that is user input is a security risk.
• Using a variable for $string that has even guaranteed safe text (for example, user interface text provided literally in code), will not be picked up by the localization static text processor. (The parameter could be a variable if the entire string in $text has been passed into t() or new TranslatableMarkup() elsewhere as the first argument, but that strategy is not recommended.)

namespace Drupal\gtext;

/**
 * Provides a gText class.
 */
class gText {

  /**
   * The context the sourcen string belongs to.
   *
   * @var array
   */
  protected $contextTranslations = [];

namespace Drupal\gtext;

use Drupal\Core\StringTranslation\TranslatableMarkup;
use Drupal\Core\StringTranslation\PluralTranslatableMarkup;

/**
 * Provides a gTextTranslation class.
 */
class gTextTranslation {

  /**
   * Context translation string.
   *
   * @var array
   */
  protected $context;

  /**
   * Constructs a new gTextTranslation.
   *
   * {@inheritDoc}
   */
  public function __construct($context) {
    $this->context = $context;
  }

  /**
   * Translates a string to the current language or to a given language.
   *
   * {@inheritDoc}
   */
  public function t($string, array $args = [], $options = []) {
    $options['context'] = $this->context;
    return new TranslatableMarkup($string, $args, $options);
  }

  /**
   * Formats a string containing a count of items.
   *
   * {@inheritDoc}
   */
  public function plural($count, $singular, $plural, array $args = [], $options = []) {
    $options['context'] = $this->context;
    return new PluralTranslatableMarkup($count, $singular, $plural, $args, $options);
  }

}

  /**
   * Is invoked automatically when a non-existing method.
   *
   * {@inheritDoc}
   */
  public function __call($context, $args) {
    if (!isset($this->contextTranslations[$context])) {
      $this->contextTranslations[$context] = new gTextTranslation($context);
    }

    return $this->contextTranslations[$context]->t($args[0], !empty($args[1]) ? $args[1] : []);
  }

  /**
   * Is utilized for reading data from inaccessible.
   *
   * {@inheritDoc}
   */
  public function __get($context) {
    if (!isset($this->contextTranslations[$context])) {
      $this->contextTranslations[$context] = new gTextTranslation($context);
    }

    return $this->contextTranslations[$context];
  }

  /**
   * Is triggered by calling isset() or empty()
   *
   * {@inheritDoc}
   */
  public function __isset($context) {
    return \ctype_alnum($context) && !ctype_digit(mb_substr($context, 0, 1));
  }

}

It's not clear the purpose of that code, when a module would just use code similar to the following one.

$blog_title = new TranslatableMarkup("@name's blog", array(
  '@name' => $account
    ->getDisplayName(),
));

It's not also clear why the class would implement __call(), which is invoked for every not existing method, instead of implementing a normal method.

As for the class name, gText isn't more readable than Gtext or GText, which are the class names that follow the Drupal coding standards.

I checked the two instances of the concatenation warning

FILE: /var/www/pareviewd/pareview_temp/wshijwft/src/Form/TranslateForm.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
264 | WARNING | Do not concatenate strings to translatable strings, they
| | should be part of the t() argument and you should use
| | placeholders
--------------------------------------------------------------------------

This concatenation does not affect the whole idea of t() being able to also handle structural and word ordering implications of languages. All of what t is to handle is actually handled by t().

FILE: ...pareviewd/pareview_temp/wshijwft/src/Twig/TwigTranslateExtension.php
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
65 | WARNING | Only string literals should be passed to t() where
| | possible
--------------------------------------------------------------------------

This is a wrapper to allow the translation function to be called within the class. So this is a false positive as well.

I concur with @apaderno on class naming. Even when acronyms are part of class names it is recommended to follow the Drupal standard. There is a long discussion around this on drupal.org - https://www.drupal.org/project/drupal/issues/1627350. It is worth a read :)

t() is allowed to use that code because that is Drupal core, but none of the modules are allowed to call t() or new TranslatableMarkup() using a variable as first argument.

The translation context is allowed to be a string containing spaces, such as in t('December', [], ['context' => 'Long month name']) used by Drupal to get the translated month names. In those cases, that code won't work.

Another security vulnerability is a translation form on a route "gtext.translate"
Form on this route doesn't have a html validation and allows to save translations like "<script src="https://somescript"></script>"
The core interface translation form doesn't allow to save tags like this to prevent xss.

  /**
   * Returns the language manager service.
   *
   * @var \Drupal\Core\Language\LanguageManagerInterface
   */
  protected $languageManager;

  /**
   * Class that manages modules in a Drupal installation.
   *
   * @var \Drupal\Core\Extension\ModuleHandler
   */
  protected $moduleHandler;

Those properties are already defined from the parent class. There is no need to re-define them.

    _locale_refresh_translations(array_keys($languages), [$lid]);
    _locale_refresh_configuration(array_keys($languages), [$lid]);

Functions in Drupal core or Drupal core module whose names start with an underscore should not be called, since they aren't part of the Drupal public API.

    if (!empty($_POST['target']) && !empty($_POST['lang']) && !empty($_POST['text'])) {
      $translate = GoogleTranslate::translate(!empty($_POST['source']) ? $_POST['source'] : 'auto', $_POST['lang'], $_POST['text']);

      if (!empty($translate)) {
        $translate = str_replace(trim($_POST['text']), $translate, $_POST['text']);
        $response->addCommand(new InvokeCommand('#' . $_POST['target'], 'val', [$translate]));
      }
    }

Modules should not access the values in $_POST, as that is considered a security issue. There is also no need to access that global variable, since Symfony has methods to get those values.

    $filename = str_replace('www.', '', $_SERVER['HTTP_HOST']) . '-' . ($group == '_all' ? 'all' : $group) . '-' . $langcode . '-' . date('Y-m-d_His') . '.po';

Symfony has also a method to get the value of $_SERVER['HTTP_HOST'], which should not be accessed directly.

class SettingsForm extends FormBase {

Since the class is used for a configuration form, it should extend ConfigFormBase, not FormBase.

    switch ($langcode) {
      case 'ru':
      case 'uk':
      case 'be':
        return 'nplurals=3; plural=(n%10==1 && n%100!=11 ? 0 : n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20) ? 1 : 2);';

      default:
        return 'nplurals=2; plural=(n != 1);';
    }

That code is returning the wrong plural formula for many languages, including Tibetan, Mapudungun, Chiga, Czech, Welsh, and French.

        $message = @json_decode($e->getMessage(), TRUE);
        if ($message !== FALSE && isset($message['error']['code']) && isset($message['error']['message'])) {
          $message = '[' . $message['error']['code'] . '] ' . $message['error']['message'];
        }
        else {
          $message = $e->getMessage();
        }
        $form_state->setError($form['api_key'], $message);

The error message isn't translatable.

    if (isset($_GET['context']) && strpos($_GET['context'], '_ntrans:') !== FALSE) {
      unset($form['actions']['export']);
    }

Instead of deleting a form submission button, the code should set its #access property to FALSE.

Please fix the error and the warning in the attached file.

Using the following code, the title changes from First plural form for the first text area to 2. plural form for the second text area. That is probably not the desired effect. Rather then changing the title, it would be easier to use the same title for every plural form text areas; Drupal doesn't require that each form element have a unique title and Plural form is correct for every plural form text area.

            $form['strings'][$string->lid][$langcode][$i] = [
              '#type'          => 'textarea',
              '#title'         => ($i == 0 ? $this->t('Singular form') : $this->formatPlural($i, 'First plural form', '@count. plural form')),
              '#rows'          => 2,
              '#cols'          => 40,
              '#default_value' => isset($translation_array[$langcode][$i]) ? $translation_array[$langcode][$i] : '',
              '#attributes'    => [
                'lang'           => $langcode,
                'class'          => ['gtext-translatable-field'],
                'data-lang'      => $langcode,
                'data-text'      => $source_array[$i == 0 ? 0 : 1],
                'data-url'       => Url::fromRoute('gtext.translate.google')->toString(),
              ],
              '#prefix'        => $i == 0 ? ('<span class="visually-hidden">' . $this->t('Translated string (@language)', ['@language' => $langcode]) . '</span>') : '',
              '#weight'        => $i * 2,
            ];
          }

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 IRC #drupal-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 dedicated reviewers as well.