[D8] TMGMT XTM

What is the difference with this module? https://www.drupal.org/project/tmgmt
We just review a single project, and a single branch. Choose the project/branch you want reviewed, and edit the title to reflect your choice. D7 o D8?

Thank you for your contribution!
We just review a single project, and a single branch. Choose the project/branch you want reviewed, and edit the OP to reflect your choice.

We can only check one branch.

Major coding standards / best practice issues

An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we ask you to please have a look at the report and try to fix them. However, please note that some of the issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

You can find the results of the automated report at https://pareview.sh/pareview/https-git.drupal.org-project-tmgmt_xtm.git-....

README.txt

Please take a moment to make your README.txt follow the guidelines for in-project documentation.

---

Looking over your code, I see some places that could use some improvement:

\Drupal\tmgmt_xtm\XtmTranslatorUi::buildConfigurationForm() has each element broken out into a separate method. That makes reading the form API components very difficult. I'd strongly recommend just putting the whole form into one method.

\Drupal\tmgmt_xtm\RemoteCallbackController::callback() looks like it pulls a lot of data straight from $_REQUEST without sanitizing it. Please review https://www.drupal.org/docs/7/security/writing-secure-code/overview.

\Drupal\tmgmt_xtm\Plugin\tmgmt\Translator\Connector has a few methods that use the error-suppressing @ operator. I would try to avoid ever using that, or if you really must use it, document why in comments in the code.

The use of namespaces is odd and inconsistent - for example, it seems like \Drupal\tmgmt_xtm\Plugin\tmgmt\Translator\TMGMTXtmException should be \Drupal\tmgmt_xtm\Exception\TMGMTXtmException.

I think once the coding standards issues are fixed (most of which can be fixed automatically by phpcbf, a lot of this will clear up. For now, it needs some work.

Thanks for your contribution!

Review:
* README is empty, please fill it out
* info file: please remove the Drupal.org packaging block.
* Coding Standards: you use 4 space indentation and the opening braces should be on the same line. Please install Coder and fiy all your coding standard violations. https://www.drupal.org/project/coder
* drupal_set_message() is depreacted and must not be used anymore. Please use the appropriate service instead.
* "\Drupal::logger(self::XTM_LOGGER)->notice("XTM retrieveTranslation:" . GuzzleHttp\json_encode($fault));": do not concatenate variables into translatable log strings, use placeholders instead. Same for "drupal_set_message($error . ' ' . t('Please check the project in XTM for more details.')". Always use placeholders with t().
* Why do you need the unprotected /tmgmt_xtm_callback path? Please add a comment in the routing YAML file. As far as I can see this open callback is not a security issue because it then fetches something via SOAP and then processes that, so I guess that is ok?
* You use \Drupal all over the place, please use dependency injection instead. See https://www.drupal.org/docs/8/api/services-and-dependency-injection/serv...
* Do not use file_get_contents() to fetch remote files. Use Drupal's HTTP client instead.

So although there is quite a bit of wrong API usage as listed above I don't see a security issue, so I think we can approve this.

Automated review:

• Your README.txt does not follow best practices (headings need to be uppercase). See https://www.drupal.org/node/2181737 .
  • The INTRODUCTION section is missing.
  • The REQUIREMENTS section is missing.
  • The INSTALLATION section is missing.
  • The CONFIGURATION section is missing.
• The tmgmt_xtm.module does not implement hook_help(). See https://www.drupal.org/docs/develop/documenting-your-project/module-docu... .
• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: ...me/klausi/workspace/drupal-8/modules/tmgmt_xtm/tmgmt_xtm.routing.yml
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------
   7 | WARNING | Open page callback found, please add a comment before the
     |         | line why there is no access restriction
  --------------------------------------------------------------------------
  
  
  FILE: ...upal-8/modules/tmgmt_xtm/src/Controller/RemoteCallbackController.php
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  --------------------------------------------------------------------------
   29 | WARNING | \Drupal calls should be avoided in classes, use
      |         | dependency injection instead
   37 | WARNING | \Drupal calls should be avoided in classes, use
      |         | dependency injection instead
  --------------------------------------------------------------------------
  
  
  FILE: /home/klausi/workspace/drupal-8/modules/tmgmt_xtm/tmgmt_xtm.info.yml
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------
   6 | WARNING | All dependencies must be prefixed with the project name,
     |         | for example "drupal:"
  --------------------------------------------------------------------------
  
  Time: 197ms; Memory: 8Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script.

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.