[1.0.x] Poster POS Commerce Integration

poster_integration.info.yml

package: Custom

Modules should avoid to use that value for package.

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.

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

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

@vishal.kadam Issues fixed.

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.

Thanks @apaderno!
I implemented your notices, and also refactored forms to decrease duplicate code.
Wait for your review.

Automated Review

These were already covered above.
Code looks clean.

Manual Review

Manual Review

@HitchShock thanks for your review.
All 2 points that you mentioned are fixed now.
1. giggsey/libphonenumber-for-php isn't used anymore.
2. Readme.md file is fixed.

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.

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

@cmlara thanks for the explanation. I was a bit confused, because I saw the list of possible licenses for Libs, but didn't notice that only about committing to the Drupal repo.
I returned the lib and validation.

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.

Automatically closed - issue fixed for 2 weeks with no activity.