[1.0.x] Poster POS Commerce Integration

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.

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

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

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


FILE: poster_integration/poster_integration.links.action.yml
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 10 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: poster_integration/poster_integration.links.task.yml
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 10 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: poster_integration/poster_integration.routing.yml
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 30 | ERROR | [x] Expected 1 newline at end of file; 2 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: poster_integration/poster_integration.services.yml
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 16 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: poster_integration/README.md
------------------------------------------------------------------------
FOUND 1 ERROR AND 3 WARNINGS AFFECTING 4 LINES
------------------------------------------------------------------------
  9 | WARNING | [ ] Line exceeds 80 characters; contains 94 characters
 13 | WARNING | [ ] Line exceeds 80 characters; contains 83 characters
 22 | WARNING | [ ] Line exceeds 80 characters; contains 106 characters
 23 | ERROR   | [x] Expected 1 newline at end of file; 0 found
------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------


FILE: poster_integration/src/Form/SettingsForm.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 176 | WARNING | Vocabulary::loadMultiple calls should be avoided in classes, use dependency injection instead
 179 | WARNING | Unused variable $key.
--------------------------------------------------------------------------------

1. poster_integration.info.yml

package: Custom

Modules should avoid to use that value for package.

2. Some files are not correctly following the Drupal coding standards, especially when an empty line is added at the beginning or the end of a method code.

poster_integration/src/PosterConnection.php
poster_integration/src/Form/LoadProductsForm.php
poster_integration/src/Service/PhoneValidator.php

@vishal.kadam Could you say which files are not following the coding standards?

@apaderno I have updated my comment #6 to include files.

Hello @akozoriz,

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

• 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

src/Controller/PosterController.php

/**
 * PosterController to output the table with imported Products.
 */

It is not necessary to say the class name, since that is already given from the code. Controller is sufficient.

src/EventSubscriber/OrderCompleteSubscriber.php

  /**
   * Constructor.
   */
  public function __construct(

The documentation comments for constructors say which class instance is created, namespace included.

      catch (\Exception $e) {
        $this->logger->error("Poster API error: @message",
          ['@message' => $e->getMessage()]);
      }

For exceptions, the function used to log them should be watchdog_exception(), or code similar to that should be used.

src/Form/LoadCategoriesForm.php

  /**
   * Helper service.
   *
   * @var \Drupal\Core\Extension\ModuleExtensionList
   */
  protected $extensionList;

That description is not for that property.

src/Service/LoadHelper.php

        $this->logger->log(E_NOTICE, 'File %file could not be copied, because the destination directory %destination is not configured correctly.', [
          '%file' => $url,
          '%destination' => $this->fileSystem->realpath($directory),
        ]);

E_NOTICE is not a constant defined by Drupal. The log level values are defined in the RfcLogLevel class.

Automated Review

These were already covered above.
Code looks clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template.

The readme could use some more structure, as suggested in the template. Also the hook_help() implementation could contain more infromation.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

Were tackled by previous reviewers

This review uses the Project Application Review Template.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code

Module requires "giggsey/libphonenumber-for-php" but this package uses Apache 2.0 licenses. This license doesn't compatible with GPLv2 what is a critical issue here.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template.

In general it's OK, But... It will be good to add the description about the module (according to the template). You can just Copy/Paste it from the module page.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

Were tackled by previous reviewers

This review uses the Project Application Review Template.

Extra things

Service classes don't implement an interface which is not good practice.

Now everything looks good to me.

P.S. It's a pity that validation was removed and not replaced with something else. But I think this will be done in the future, don't focus on it, as we are checking other things here.
Suggestion:
- Keep the code for "giggsey/libphonenumber-for-php"
- Add to the description of the module that it can use the functionality of the "giggsey/libphonenumber-for-php" package, but the user must install it on their own.
- Make it possible to use the module without "giggsey/libphonenumber-for-php" (extra check conditions to determine whether the package was installed or not)

@HitchShock May you point out where that validation code was removed?
If the module contained a validation handler that has been removed, that should be added back, even if the code needs to be changed.

@apaderno you can check this - https://git.drupalcode.org/project/poster_integration/-/commit/99b58d6af...

Note that the review in #14 is incorrect. Including a file in the composer.json is NOT committing the library to the Drupal Git Repository service and as such is NOT a violation of the "Policy on 3rd party assets on Drupal.org"

It is this review in #14 that caused the library removal that is requested to be added back in #16/#17.

@akozoriz As an applicant some times you may need to push-back when a review is not accurate, don't just accept all comments given in these reviews as being accurate.

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.