[11.x-1.x] languagewire_translation_provider

Thank you for applying!

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

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications, Application workflow, What to cover in an application review, and Tools to use for reviews.

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues.

main is a wrong branch name, as branch names end with the literal .x. That branch needs to be removed.

Drupal core is going to switch to main as main branch. main is an acceptable branch name, but drupal.org is still not completely able to handle that branch name and it is preferable to still use a branch name like 1.x or 2.0.x as described in Release branches.

There is still a single branch, main, which needs to be deleted. Before that, a new branch must be created and the code committed on that branch. See Release branches for more details about the correct branch names to use.

Furthermore, the project page does not describe the module, its requirements, nor its features; it does not even give information useful to decide whenever the module is a good suit for somebody's purpose.

Fix PHPCS issues

See attached phpcs report.

It seems you have missed working on the coding standards. You can use the PHPCS tool for checking and resolving issues.

More importantly, the license for projects hosted on drupal.org cannot be MIT, as the project page says. Providing a project under that license is contrary to the Git access agreement you accepted.

Libraries that are available from other sites or repositories must not be committed in drupal.org repositories. If those libraries are necessary for the module to work, the project page should give instructions on how to install the library.
Furthermore, if that library has a license that is different from the one used by Drupal core, it cannot be committed in drupal.org repositories, for the same reason I gave in comment #15.

See my previous comment. The library code must be removed from the module repository.

Branch 10.x-1.x

1. file composer.json
"LanguageWire\\": "api/src/"

In the project structure, the 'api/src' folder seems to be missing. Can you check again?

2. Still have Coding Standard issues reported by PHPCS

vendor/bin/phpcs --standard=Drupal modules/languagewire_translation_provider/

FILE: /var/www/html/drupal-10.2.x/modules/languagewire_translation_provider/api/Callback/ResponseCallbackTrait.php
------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------
 18 | WARNING | Line exceeds 80 characters; contains 106 characters
------------------------------------------------------------------------------------------------------------------


FILE: /var/www/html/drupal-10.2.x/modules/languagewire_translation_provider/api/Content/Content.php
-----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------
 24 | WARNING | The class short comment should describe what the class does and not simply repeat the class name
 25 | WARNING | Interface names should always have the suffix "Interface"
-----------------------------------------------------------------------------------------------------------------


FILE: /var/www/html/drupal-10.2.x/modules/languagewire_translation_provider/api/Content/DehydrationInterface.php
----------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------------
 18 | WARNING | Line exceeds 80 characters; contains 106 characters
 19 | WARNING | Line exceeds 80 characters; contains 114 characters
----------------------------------------------------------------------------------------------------------------


FILE: /var/www/html/drupal-10.2.x/modules/languagewire_translation_provider/api/Content/JsonCodec.php
-----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
-----------------------------------------------------------------------------------------------------
 41 | WARNING | Line exceeds 80 characters; contains 114 characters
 42 | WARNING | Line exceeds 80 characters; contains 116 characters
 43 | WARNING | Line exceeds 80 characters; contains 115 characters
 57 | WARNING | Line exceeds 80 characters; contains 107 characters
-----------------------------------------------------------------------------------------------------


FILE: /var/www/html/drupal-10.2.x/modules/languagewire_translation_provider/api/Content/JsonSerializer.php
----------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------
 28 | WARNING | Line exceeds 80 characters; contains 82 characters
----------------------------------------------------------------------------------------------------------


FILE: /var/www/html/drupal-10.2.x/modules/languagewire_translation_provider/api/Content/Serializer.php
------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
------------------------------------------------------------------------------------------------------
  8 | WARNING | Line exceeds 80 characters; contains 99 characters
 15 | WARNING | Interface names should always have the suffix "Interface"
 18 | WARNING | Line exceeds 80 characters; contains 86 characters
------------------------------------------------------------------------------------------------------


FILE: /var/www/html/drupal-10.2.x/modules/languagewire_translation_provider/api/Content/Udf/Frame.php
-----------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
-----------------------------------------------------------------------------------------------------
 18 | WARNING | Line exceeds 80 characters; contains 115 characters
 20 | WARNING | Line exceeds 80 characters; contains 112 characters
 21 | WARNING | Line exceeds 80 characters; contains 113 characters
 23 | WARNING | Line exceeds 80 characters; contains 118 characters
 24 | WARNING | Line exceeds 80 characters; contains 118 characters
-----------------------------------------------------------------------------------------------------
.....

Truly, as Drupal module, the only namespaces it can define start with \Drupal\languagewire_translation_provider\. The directory associated to those namespaces must be the src directory in the module repository.

All the files/directories contained in the api directory must be moved in the src directory.
The following documentation comment added to all those files must be removed, since those files are not part of the LanguageWire PHP Client Library package, as comment #20 said they are part of the module.

/*
 * This file is part of the LanguageWire PHP Client Library package.
 *
 * (c) LanguageWire <contact@languagewire.com>
 *
 * For the full copyright and license information,
 * please view the LICENSE file that was distributed with this source code.
 */

/**
 * Response Callback Trait.
 *
 * ResponseCallbackTrait trait provides
 * methods to execute a callback function
 * on a PSR-7 Response object.
 *
 * @category LanguageWire
 * @package LanguageWire\Callback
 * @copyright 2020 LanguageWire
 */

@category, @package, and @copyright are not used in drupal.org repositories. The last one would not probably hold true, as in GPL-2.0-or-later code, the copyright is hold between all the users who contributed code with commits.

src/Adapter/Content/DrupalVariablePlaceholderPrefixingConvention.php

As per Object-oriented code (part of Drupal coding standards), class names should not include Drupal.

src/Adapter/Database/DrupalConfigurationItemRepository.php

  /**
   * Constructor.
   */
  public function __construct(Connection $connection) {
    $this->connection = $connection;
  }

The documentation comment for constructors is not mandatory anymore, If it is given, the description must be Constructs a new [class name] object. where [class name] includes the class namespace.

  /**
   * Remove all.
   */
  public function removeAll(TmgmtTranslatorInterface $translator, string $itemType): void {
    $this->connection->delete(TablesInterface::CONFIGURATION_ITEM)
      ->condition('type', $itemType)
      ->condition('translator_id', $translator->id())
      ->execute();
  }

  /**
   * Save all.
   *
   * @param \stdClass[] $items
   *   Items.
   * @param \Drupal\languagewire_translation_provider\Adapter\TMGMT\TmgmtTranslatorInterface $translator
   *   Translator.
   * @param string $itemType
   *   Item type.
   *
   * @throws \Exception
   */

The short description for a method must not repeat the method name, but be more descriptive about its purpose. It should also be longer than one or two words. The documentation comment must describe the method parameters and the return value, if the method returns any value. If then the method is inherited from a parent class or defined in an interface, the documentation comment can simply be {@inheritdoc}.
@throws must also say when the exception is thrown.

src/Adapter/Database/LanguageWireConfigurationItemRepositoryInterface.php

  /**
   * Remove all.
   */
  public function removeAll(TmgmtTranslatorInterface $translator, string $itemType): void;

The short description for a method must not repeat the method name, but be more descriptive about its purpose. The documentation comment must describe the method parameters and the return value, if the method returns any value.

src/Adapter/Entity/DrupalEntityRepository.php

  /**
   * System.
   *
   * @var \Psr\Log\LoggerInterface
   */
  private $logger;

That is not the correct description for a property containing an object that implements \Psr\Log\LoggerInterface.

    catch (\Exception $exception) {
      $this->logger->warning("Exception occurred when trying to load \"$entityId\" (ID $entityId) entity. \n $exception");
      return NULL;
    }

The first argument passed to the logger methods that log a message must be a literal string, not translatable strings nor string concatenations.

src/Adapter/DrupalSystem.php

  /**
   * Constructor.
   */
  public function __construct() {
    $this->configItemRepository = \Drupal::service('languagewire.configuration_item_repository');
    $this->projectRepository = \Drupal::service('languagewire.project_repository');
    $this->projectTemplateRepository = \Drupal::service('languagewire.project_template_repository');
    $this->documentRepository = \Drupal::service('languagewire.document_repository');
  }

In a service class, the parameters passed to the class constructor are the ones defined for the service, in the .services.yml file.

    try {
      $moduleInfo = \Drupal::service('extension.list.module')->getExtensionInfo($moduleName);

      return empty($moduleInfo) || !isset($moduleInfo['version'])
                ? '0.x-dev'
                : $moduleInfo['version'];
    }
    catch (UnknownExtensionException $e) {
      return NULL;
    }
  }

In a service class, all the dependencies are injected using Dependency Injection, not using Drupal::service().

/*
 * This file is part of the LanguageWire PHP Client Library package.
 *
 * (c) LanguageWire <contact@languagewire.com>
 *
 * For the full copyright and license information,
 * please view the LICENSE file that was distributed with this source code.
 */

Those files are part of the languagewire_translation_provider module, not the LanguageWire PHP Client Library package. Those comments must be removed.

Furthermore, the project title (and module names) are different from the project machine name.

I am changing the issue priority as per issue priorities.

Please update the module page to have more readable formatting. Use headings and subheadings, and make the module title a readable name, rather than the "machine name" containing underscores. Look at Coder as an example.

Also, in the future, please consider using semantic versioning for branch names and releases.

I'm not able to view or clone the project repository for some reason right now, as it says "Deploy in progress"...

Your README.md does not follow best practices.

INSTALL.md contains outdated instructions. Remove info about Drupal 7. Also, when viewing the file, the images don't load.

The files in ./config/optional have CRLF line terminators. These should be changed to Unix-style LF line terminators.

languagewire_translation_provider.info.yml: All dependencies must be prefixed with the project name. Also, remove version and timestamp info. Those will be added automatically. See a good example in the Webform module.

There are multiple classes that use \Drupal calls. Use dependency injection instead, if possible.

The .module and .install files aren't even close to following Drupal coding standards. Sort your use statements alphabetically, have functions' opening braces on the same line as the function declaration. Include function comments for every function. TRUE, FALSE and NULL must be uppercase. Functions not in a class/trait/enum should be snake_case and prefixed with the module name.

Many of the issues can be automatically found by PHPCS and fixed by PHPCBF if you run those. Please run them, because there are many more issues that I have not listed, as there are too many. You should also configure your IDE to integrate with PHPCS, so that it will highlight coding standards violations as you type. If you use Visual Studio Code, you can use PHP Sniffer & Beautifier.

There is no need to create new branches because of this application.
Changing branch to review make it harder for reviewers to understand what has been changed and verify the changes are exactly what the review asked for.

• The following points are just a start and don't necessarily encompass all the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that does not follow the coding standards, contains possible security issues, or does not correctly use the Drupal API; the single points are not ordered, not even by importance

src/Adapter/System.php

src/Adapter/TMGMT/TmgmtTranslatorRepository.php

src/Ui/CheckoutUi.php

Classes used for services must not call any \Drupal method. Instead, they need to inject their dependencies.

src/Adapter/Logger/NineLogger.php

This class is merely returning a Drupal service. Remove this class and use directly that Drupal service.
Furthermore, if the code changes basing on the Drupal version, you need to have two different branches.

languagewire_translation_provider.services.yml

  languagewire.drupal.system:
    class: Drupal\languagewire_translation_provider\Adapter\System

  languagewire.project_template_mapper:
    class: Drupal\languagewire_translation_provider\Database\Mapper\ProjectTemplateMapper

  languagewire.document_mapper:
    class: Drupal\languagewire_translation_provider\Database\Mapper\DocumentMapper

Those services are not autowired. Their definitions are missing the service arguments.

  languagewire.tmgmt_job_repository:
    class: Drupal\languagewire_translation_provider\Adapter\TMGMT\TmgmtJobRepository

  languagewire.tmgmt_job_item_repository:
    class: Drupal\languagewire_translation_provider\Adapter\TMGMT\TmgmtJobItemRepository

  languagewire.preview_site_index_file_mover:
    class: Drupal\languagewire_translation_provider\Adapter\PreviewSite\CleanupAction\PreviewSiteIndexFileMover

  languagewire.preview_site_url_processor:
    class: Drupal\languagewire_translation_provider\Content\HtmlPreview\HtmlPreviewUrlProcessor

  languagewire.preview_site_builder:
    class: Drupal\languagewire_translation_provider\Adapter\PreviewSite\PreviewSiteBuilder

  languagewire.preview_site_build_repository:
    class: Drupal\languagewire_translation_provider\Adapter\PreviewSite\PreviewSiteBuildRepository

  languagewire.preview_strategy_repository:
    class: Drupal\languagewire_translation_provider\Adapter\PreviewSite\PreviewStrategyRepository

A class without dependencies does not need to implement a service.

src/Ui/JobInfoUi.php

src/Ui/ConfigurationFormUi.php

src/Ui/CheckoutUi.php

src/Ui/CheckoutUi/TemplateOrServiceForm.php

Those classes build a form, but no submission button is added. See what that means in Form generation.
Those forms do not have validation handlers. Adding a method whose name starts with validate does not automatically make it a form validation handler.
Form classes are also not used for services.

src/Service/HtmlPreviewService.php

    if (!$this->htmlPreviewSettingsChecker->canGenerateHtmlPreview()) {
      $this->logger->warning("Attempted to create HTML Previews without having valid settings. " . "Reason(s): " . implode(", ", $this->htmlPreviewSettingsChecker->getFailedRequirements()));
      return [];
    }

The first argument of warning() and similar logger methods, must be a literal string, not a concatenation of strings, nor a translatable string.

src/Content/HtmlPreview/HtmlPreviewSettingsChecker.php

    $privateFilePath = $this->system->realPath("private://");
    if (empty($privateFilePath)) {
      $errors[] = self::ERROR_PRIVATE_FILE_PATH_NOT_SET;
    }

Why isn't the code using the file_system service?

    $previewSiteVersionMatch = version_compare($previewSiteVersion, self::PREVIEW_SITE_MINIMUM_VERSION) >= 0;
    if (!$previewSiteVersionMatch) {
      $errors[] = sprintf(self::ERROR_PREVIEW_SITE_VERSION_MISMATCH, $previewSiteVersion, self::PREVIEW_SITE_MINIMUM_VERSION);
    }

    return $errors;

If those errors are shown in the user interface to users, they must be translatable.

src/api

All the classes contained in that directory and its sub-directories do not have any dependency on Drupal core. They need to be hosted outside of drupal.org.

In general, check also the code formatting, which must follow the Drupal coding standards. The Drupal coding standards, among other things, do not say the indentation is four spaces or more.
Check also the documentation comments which, as reported in a previous comment, should not repeat the class/method name; they should not also repeat what already said in the documentation comment used in the parent class or interfaces the class implements.
I would also check the code with phpcs --standard=Drupal,DrupalPractice, as already suggested, because it still reports warnings and errors.
It feels like the module is implementing own classes when it should use classes Drupal already provides.

I did check only the classes that are referenced in the .services.yml file. I will check the other ones too, once I find a way to get the list of the classes effectively instantiated from services or the module file.

I am changing priority as per Issue priorities.

Thank you for your contribution and for your patience with the review process!

I am going to update your account so you can opt into security advisory coverage any project you create, including the projects you already created.

These are some recommended readings to help you with maintainership:

• Dries' post on Responsible maintainers
• Maintainership
• Git version control system
• Issue procedures and etiquette
• Maintaining and responding to issues for a project
• Release naming conventions.

You can find more contributors chatting on Slack or IRC in #drupal-contribute. So, come hang out and stay involved!
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 the dedicated reviewers as well.