[1.0.x] WordsOnline Connector

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

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

1. Fix phpcs issues. You can use the PHPCS tool for checking and resolving issues.

2. FILE: tmgmt_wordsonline.info.yml

version: '1.0'
project: 'wordsonline_translation_provider'
datestamp: 1653927462

Remove these lines. It will be added automatically by the packaging script.

core_version_requirement: ^8 || ^9 || ^10

Drupal Core versions before 8.7.7 do not recognize the core_version_requirement: key.

3. main and 1.0.0 are wrong branch names, as branch names end with the literal .x. That branch needs to be removed.

4. README.md file is empty. Please take a moment to make your README.md follow the guidelines.

php -d memory_limit=4G vendor/bin/phpstan analyse modules/contrib/wordsonline_connector

PHPSTAN issues

(The branch name is 1.0.x, not 1.0.0.)

There are PHPCS issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml wordsonline_connector/

FILE: wordsonline_connector/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
----------------------------------------------------------------------
  3 | WARNING | Line exceeds 80 characters; contains 102 characters
  4 | WARNING | Line exceeds 80 characters; contains 111 characters
  6 | WARNING | Line exceeds 80 characters; contains 118 characters
 23 | WARNING | Line exceeds 80 characters; contains 82 characters
----------------------------------------------------------------------


FILE: wordsonline_connector/src/Controller/OrderController.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 13 WARNINGS AFFECTING 13 LINES
--------------------------------------------------------------------------------
  129 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
  369 | WARNING | Only string literals should be passed to t() where possible
  405 | WARNING | Only string literals should be passed to t() where possible
  430 | WARNING | Only string literals should be passed to t() where possible
  455 | WARNING | Only string literals should be passed to t() where possible
  493 | WARNING | Only string literals should be passed to t() where possible
  500 | WARNING | Only string literals should be passed to t() where possible
  508 | WARNING | Only string literals should be passed to t() where possible
  538 | WARNING | Only string literals should be passed to t() where possible
  546 | WARNING | Only string literals should be passed to t() where possible
  582 | WARNING | Only string literals should be passed to t() where possible
  950 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 1113 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------


FILE: wordsonline_connector/src/Form/OrderConfirmForm.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
--------------------------------------------------------------------------------
 108 | WARNING | Unused variable $status.
 109 | WARNING | Unused variable $rGuid.
 112 | WARNING | Unused variable $status.
 113 | WARNING | Unused variable $rGuid.
--------------------------------------------------------------------------------


FILE: wordsonline_connector/src/Form/UpdateForm.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
--------------------------------------------------------------------------------
 156 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 172 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 197 | WARNING | Unused variable $files.
 212 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------


FILE: wordsonline_connector/src/Form/ViewXlfForm.php
--------------------------------------------------------------------------------
FOUND 3 ERRORS AND 2 WARNINGS AFFECTING 4 LINES
--------------------------------------------------------------------------------
  42 | ERROR   | Doc comment for parameter $configuration does not match actual variable name <undefined>
  44 | ERROR   | Superfluous parameter comment
  46 | ERROR   | Superfluous parameter comment
 105 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
 105 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------


FILE: wordsonline_connector/src/Plugin/tmgmt/Translator/WordsOnlineTranslator.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 298 | WARNING | Unused variable $files.
 428 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------


FILE: wordsonline_connector/src/WordsOnlineTranslatorUi.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 7 WARNINGS AFFECTING 7 LINES
--------------------------------------------------------------------------------
  77 | WARNING | Unused variable $translator.
 162 | WARNING | Only string literals should be passed to t() where possible
 187 | WARNING | Only string literals should be passed to t() where possible
 213 | WARNING | Unused variable $triggering_element.
 214 | WARNING | Unused variable $translator_settings.
 271 | WARNING | Unused variable $servicesarray.
 287 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------


FILE: wordsonline_connector/tmgmt_wordsonline.info.yml
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------------
 6 | WARNING | [ ] All dependencies must be prefixed with the project name, for example "drupal:"
 8 | ERROR   | [x] Expected 1 newline at end of file; 2 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: wordsonline_connector/tmgmt_wordsonline.module
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 426 | WARNING | Hook implementations should not duplicate @param documentation
 438 | WARNING | Hook implementations should not duplicate @param documentation
 440 | WARNING | Hook implementations should not duplicate @param documentation
--------------------------------------------------------------------------------

Yes, PHPCS warnings also need to fix.

@luannguyen

I have reviewed the changes, and they look fine to me.

Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.

Thanks

@luannguyen : I have tested on 1.0.x branch. Please find the text file for PHPSTAN issues.

Command run : php -d memory_limit=4G ../../vendor/bin/phpstan analyse wordsonline_connector

• The following points are just a start and don't necessarily encompass all of 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 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

Since the project machine name (its short name as indicated in https://www.drupal.org/node/3345120/edit) is wordsonline_connector the module must contain the wordsonline_connector.module, wordsonline_connector.info.yml, wordsonline_connector.install, wordsonline_connector.libraries, wordsonline_connector.links.menu.yml, wordsonline_connector.routing.yml, and wordsonline_connector.services files. All the functions (not the methods) implemented by the module must have a name starting with wordsonline_connector_. The namespaces used by the modules nees to start with Drupal\wordsonline_connector\.
The project machine name cannot be changed anymore; those files, all the functions, and the namespaces must be renamed.

src/Controller/OrderController.php

/**
 * Controller for handle order.
 */
class OrderController extends ControllerBase implements ContainerInjectionInterface {

The parent class already implements that interface; there is not need to implement it again.

  /**
   * {@inheritdoc}
   *
   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
   *   Container.
   */
  public static function create(ContainerInterface $container) {

There is no need to document the parameters, since the documentation comment is already using {@inheritdoc}.

      // Then fetch all active or continuous jobs using those translators.
      $job_ids = \Drupal::entityQuery("tmgmt_job")
        ->condition("translator", $wordsonline_translator_ids, "IN")
        ->condition(
        "state",
        [
          JobInterface::STATE_ACTIVE,
          JobInterface::STATE_CONTINUOUS,
        ],
        "IN"
        )
        ->execute();

Controllers need to use injected dependencies, even for dependencies that are used from a single line in all the controller code.

                $download_link = $this->t(
                  '<a href=":url" class="use-ajax btn"  
                   data-dialog-type="modal" 
                   title="download">Download</a>',
                  [
                    ":url" => $down,
                  ]
                );
                $upload_link = $this->t(
                  '<a  class="btn" onclick="reimport(@job_id
                  )"  title="Import">Import </a>',
                  [
                    "@job_id" => $job_id,
                  ]
                  );
                $operation = $this->t(
                  "@linkApprove  @linkView  @linkUpload",
                  [
                    "@linkApprove" => $edit_link,
                    "@linkView" => $download_link,
                    "@linkUpload" => $upload_link,
                  ]
                );
              }

That is not how links are added to a translatable string. Links are added directly to the literal strings passed to t() or $this->t().
With the current code, links are sanitized twice, which means that <a href="link">Link title</a> would be changed to <a href="link">Link title<a>.

        $this->t('Cannot find WordsOnline provider settings.
          Please contact <a href="mailto:helpdesk@wordsonline.com">
           helpdesk@wordsonline.com</a> to obtain WordsOnline provider settings.'
        )

Any link added to translatable strings must be added through placeholders. that is also valid for email links.

src/Entity/WOFile.php

/**
 * WordsOnline File.
 */
class WOFile {

The short description says does not describe the purpose of the class. It is also wrong as a class is not a file.
A Drupal entity implements at least an entity interface; differently, it is not a Drupal entity, and it should not use the namespace entities use. Entity classes also use annotations, which does not happen in this and other classes contained in the src/Entity directory.

src/Form/OrderConfirmForm.php

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

That property is already defined from the parent class.

    $markup = new FormattableMarkup("<div class='wol-order-info'>", []);
    $form['order_info'] = [
      '#markup' => $markup,
    ];

There is no need to use a FormattableMarkup instance, since the string passed to the class constructor does not contain placeholders. Then, #markup accepts also a string.

    if ($req == NULL) {
      $this->messenger->addMessage("Can not find job");
      return;
    }

Strings passed to the messenger must be translatable.

@luannguyen Yes, I have not passed any level on command the default level is 0.

If you passing level 0 then only

 basic checks, unknown classes, unknown functions, unknown methods called on $this, wrong number of arguments passed to those methods and functions, always undefined variables
 

You can refer this document for level https://phpstan.org/user-guide/rule-levels

@luannguyen : As per #21 comment. tmgmt issues if we are avoiding then below issue needs to fix related to Drupal calls.

------ ------------------------------------------------------------------------------ 
  Line   src/Common/ZipHandle.php                                                      
 ------ ------------------------------------------------------------------------------ 
  68     \Drupal calls should be avoided in classes, use dependency injection instead  
 ------ ------------------------------------------------------------------------------ 

PHPStan gives false positives when it is not correctly set or all the dependencies are not downloaded.
I would avoid using PHPStan in these applications, especially when the report is not first checked. (A reviewer should also verify what reported by PHP_CodeSniffer before posting its reports on an application.)

Drupal\tmgmt_wordsonline\Common\ZipHandle is not a service, a controller, nor a form builder class; this means it cannot use dependency injection. Saying that all the classes must use dependency injection is not correct; in fact, a class that implements only static method cannot use dependency injection.

(That is why I say that what a tool reports must be first verified. We do not want to force changes that are not necessary or, worse, not correct.)

Thank you @apaderno for more clarity regarding issue for phpstan.

@luannguyen: Yes, #18 and #20 reports are different. As mention on #23 file PHPSTAN issues. Running command is generating same issues.

I am following below steps to regenerate the phpstan issues.

• Installed PHPSTAN via https://github.com/mglaman/phpstan-drupal
• Not configured any level on PHPSTAN.neon file. if required adding level in command.
• running this command php -d memory_limit=4G vendor/bin/phpstan analyse modules/contrib/wordsonline_connector

if you check #18 and #23 files all PHPSTAN issues are already verified.
Major issues are related to tmgmt as per comment #21 we can go forward.

Call to static method load() on an unknown class Drupal\tmgmt\Entity\Job.

Static method load is exists on class Drupal\tmgmt\Entity\Job because it is inherited from class ContentEntityInterface through interface JobInterface.

Parameter $job of method
Drupal\tmgmt_wordsonline\Controller\OrderController::importTranslation() has invalid type Drupal\tmgmt\JobInterface.

Because Drupal\tmgmt\Entity\Job is inherited from Drupal\tmgmt\JobInterface , this should not be an error.

\Drupal calls should be avoided in classes, use dependency injection
instead

All issues are already answered and regarding #23 issues already covered by #26 comments.

Major if checking #18 and #23 files major issues are already rectified and answered.

Please let me know if we need to check any other way to generate PHPSTAN file like #20

@luannguyen Yes, let us do that.

Do not define the $messenger property in your class. On the constructor, just call $this->setMessenger($messenger); when you need the messenger service, call $this->messenger().

As a side note, on create() there is no need to use local variables for the services. Given how the code is formatted, code lines will not get longer than 80 characters.

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('database'),
      $container->get('messenger')
    );
  }

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.

(I forgot to change status.)

Dear Apadermo and Review team,

On behalf of Jonckers team, I am writing to express our sincere gratitude to you and reviewer team for your hard work and dedication in helping us pass the Drupal module security certification. Your expertise and attention to detail were invaluable in ensuring that our module met the standards of security.

We are grateful for your guidance throughout the certification process. Your feedback and suggestions helped us identify and address potential vulnerabilities in our module, which has made it more secure and reliable.

We appreciate the time and effort that you and reviewer team put into this project. Your professionalism and commitment to excellence are truly commendable. We look forward to working with you again in the future.

Thank you once again for your outstanding work.

Sincerely,
Jonckers VN Developer Team Lead

Ngoc