[D8] Smartcat Translation Manager

Thank you for your contribution! I added the PAReview checklist in the issue summary. You should review it.

You should create the 8.x-1.x branch and replace the 8.x-1.x tag with 8.x-1.0

Thank you for applying! While the project was created by you, none of the commits are associated with your account. We need a project where the only commits are from you, until this application is not approved.

I am closing this application due to lack of replies. Please see the previous comments for what needs to be fixed.

@Anton If those commits are from you, just set the status of this issue to Needs review. If those commits are not from you, edit the OP to provide another project and set the status of this issue to Needs review.
See also comment #5 for the branch to delete, and the one to use.

In the first case, check the Git settings for the project, as the email used for the commits is not one associated with your account. That is why the commits aren't associated to your account.

Yes, English is the language to use for those descriptions. Every user on drupal.org should be able to read the commit descriptions.

master is not a correct branch, on drupal.org. The project page doesn't show any change because there aren't releases.

A correct branch for Drupal 8 is, for example, 8.x-1.x. With a branch like that, you can create a development snapshot.

Thanks for your contribution!

Review:
* Project page is empty. Please describe what your module does. See https://www.drupal.org/node/997024
* Please fix all the coding standard errors, see attachment.
* There is a vendor folder in git, please remove that. composer.json is enough.
* smartcat_translation_manager_enable(): empty function, this is not needed? Please remove all empty function or add comments why you need them.
* "_permission: 'administer'" is not a valid permission name, so you module will not work for any user because nobody can have that permission. Please define correct permission names in your *.permissions.yml file.
* /admin/smartcat/project/delete/{id}: this path is vulnerable to CSRF exploits. You need to use a confirmation form before performing data changing operations or you need to validate a CSRF token. Please check all your routes if that happens somewhere else as well. Please also don't remove the security tag, we keep that for statistics and to show examples of security problems.

manual review:

* smartcat_translation_manager.permissions.yml: the permission names you are using already exist in Drupal core. If you are just reusing permissions then you don't need to define the permissions again.
* JS files: they still use 4 space indentation. Can you check the Drupal coding standards for Javascript and format them correctly? Please also document in code comments what they are doing.
* "Send your contact": all user facing text must run through Drupal.t() for translation. Same for the table headers in DocumentController.
* /admin/smartcat/document/refresh/{id} still looks vulnerable to CSRF exploits. Is it a problem if an attacker triggers a translation refresh? I'm not sure how destructive that operation is, maybe it does not matter if an attacker can trigger 1000 translation refreshes?
* ConfigForm: it looks like there is an API committed as code comment? What is 1e80d715-db82-43e8-b134-f54c2b64de28? Never put API keys are password into code. If this is a valid key please remove it and invalidate it now because it is publicly known now. Otherwise can you comment in code what it means?

Thank you for the application submission.

Please, fix the issues related to your Git branches and tags, at first.

Git errors:

• It appears you are working in the "prod" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
• There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588. Then remove the master branch.

Some of the DrupalPractice issues with your code:

• Global constants should not be used, move it to a class or interface
• \Drupal calls should be avoided in classes, use dependency injection instead
• Unused variables: $documents, $account, $i, etc.
• t() calls should be avoided in classes, use dependency injection and $this->t() instead

Thank you @Anton Lysak!

Please, it also needs to fix the issue mentioned on this PAReview page:

• Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting. In ./images/smartcat.svg

Coding standard problems are not application blockers, any security issues that you found?

1. Manual review:

* Remove all unused variables and classes from your code.
* JS files: Use let and const instead of var to avoid Reference errors.
* Please follow the Coding standards everywhere, incl. @annotations. There are many Arguments PHPDoc missing.
* ConfigForm: __construct is probably missing ConfigFormBase::__construct call.
* Please don't use $form = []; and the array is overridden immediately right after that.
* Revise the usage of all variables because many of them might have not been defined (before this usage).

2. Please fix this issue on PAReview page at first:

• Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting. In ./images/smartcat.svg

then you will see all issues that will need to be fixed.

3. And please check why after a clean install (and empty DB) on '/admin/smartcat' I receive the error:
PHP Error: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'xxx.smartcat_translation_manager_documents' doesn't exist: SELECT COUNT(*) AS expr

You did a great work! Just keep on moving.

Thank you for the contribution!

Could you enable some Automated tests here? (As an example)
I want to see what will be resulted there at first.

Please, be patience through the whole review process... Thank you.

Tests are not application blockers, please do a security review.

Thank you for the contribution!

Please, replace all deprecated functions in your code with the new ones. An example:
The deprecated function dirname(__FILE__) needs to be replaced with __DIR__ instead.

Deprecated functions are not application blockers. Any security issue that you found or should this be RTBC?

Drupal deprectated functions aren't a Drupal 8 issue, as they will be removed only on Drupal 9.
dirname() isn't a deprecated function, even if __DIR__ is probably preferable.

There is an error on module enable & disable?
Please, see #24 point 3.
The error still persists.

Are you sure for the functionality? Could you provide me the API sandbox link?
I post the similar error on module enable/disable also via drush and empty database:

$ ../vendor/bin/drush en smartcat_translation_manager
The following module(s) will be enabled: smartcat_translation_manager, content_translation, language

 Do you want to continue? (yes/no) [yes]:
 >

 [warning] require_once(/var/www/xxx_xxx/web/modules/smartcat_translation_manager/vendor/autoload.php): failed to open stream: No such file or directory smartcat_translation_manager.module:11

Fatal error: require_once(): Failed opening required '/var/www/xxx_xxx/web/modules/smartcat_translation_manager/vendor/autoload.php' (include_path='/var/www/xxx_xxx/vendor/pear/archive_tar;/var/www/xxx_xxx/vendor/pear/console_getopt;/var/www/xxx_xxx/vendor/pear/pear-core-minimal/src;/var/www/xxx_xxx/vendor/pear/pear_exception;.;C:\php\pear') in /var/www/xxx_xxx/web/modules/smartcat_translation_manager/smartcat_translation_manager.module on line 11
 [warning] Drush command terminated abnormally. Check for an exit() in your Drupal site.

We don't aim to fix every error in the code. We need to verify what the user understands of correctly using the available APIs, following the coding standards, and writing secure code.

It was be good to have a working module, but OK. I have objections only in my previous comments.
Thanks.

Please, regular check your module's issues page... This is a friend advise.
Thank you.

Thank you for the contribution!

I have not additional security issues found. Thank you again.

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.