Automatically posting an announcement of nodes or other content entities to the telegram channel.

Displaying post's comments from the telegram channel under the nodes.

Project link

https://www.drupal.org/project/tg_integration/

Comments

Skymen created an issue. See original summary.

skymen’s picture

Status: Active » Needs review
skymen’s picture

Title: Telegram integration » [D9] Telegram integration
avpaderno’s picture

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

Since the project is being used for this application, for the time this application is open, only the user who created the application can commit code.

avpaderno’s picture

Status: Needs review » Needs work
  • What follows is a quick review of the project; it doesn't mean to be complete
  • For each point, the review usually shows some lines that should be fixed (except in the case the point is about the full content of a file); it doesn't show all the lines that need to be changed for the same reason
  • 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
    $link = '/' . drupal_get_path('module', 'tg_integration') . '/README.md';

drupal_get_path() has been deprecated. New modules should use ExtensionPathResolver::getPath().

  /**
   * The config factory object.
   *
   * @var \Drupal\Core\Config\ConfigFactoryInterface
   */
  protected ConfigFactoryInterface $configFactory;

  /**
   * The tempstore factory object.
   *
   * @var \Drupal\Core\TempStore\PrivateTempStoreFactory
   */
  private PrivateTempStoreFactory $tempStoreFactory;

Typed properties are introduced in PHP 7.4, Drupal 9 requires PHP 7.3; Drupal 8.8, which is supported by the module, requires a lower version.

  /**
   * Send text messages to the Telegram channel.
   *
   * @param array $parameters
   *   The array of parameters. 'chat_id' not needed.
   *
   * @see https://core.telegram.org/bots/api#sendmessage
   *
   * @return int|null
   *   The message_id from 'Message' object
   *
   * @see https://core.telegram.org/bots/api#message
   */

The @see lines must be the last one in the comment.

  /**
   * Send request to the Telegram bot API.
   *
   *  @see https://core.telegram.org/bots/api
   *
   * @param string $command
   *   The string with API method name.
   * @param array $parameters
   *   The array of parameters fot the method. 'chat_id' not needed.
   *
   * @return array
   *   The array with response.
   */

The verb in the description needs to use the third person singular.

  /**
   * Constructs a Service object.
   *
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The configuration factory.
   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
   *   The messenger.
   * @param \Psr\Log\LoggerInterface $logger
   *   The logger.
   * @param \GuzzleHttp\ClientInterface $http_client
   *   The HTTP client.
   */
  public function __construct(
    ConfigFactoryInterface $config_factory,
    MessengerInterface $messenger,
    LoggerInterface $logger,
    ClientInterface $http_client
  ) {
    $this->configFactory = $config_factory;
    $this->messenger = $messenger;
    $this->logger = $logger;
    $this->httpClient = $http_client;
    $this->config = $this->configFactory->get('tg_integration.settings');
  }

Service isn't the class name, which should be fully qualified.

skymen’s picture

Status: Needs work » Needs review

Hi @apaderno. Thanks for reviewing the project!
I've fixed your mentioned issues and checked the code one more time with the code sniffer. I pushed changes to the dev branch.
Only for one thing I need clarification from you. It's about your last comment "Service isn't the class name, which should be fully qualified." Could you please show me what particular line I have to change and to what?
Thanks one more time for your time.

avpaderno’s picture

I apologize, I should have quoted more code.

/**
 * Sending commands to the Telegram Bot.
 */
class TgIntegrationBot implements TgIntegrationBotInterface {

  use StringTranslationTrait;

  /**
   * The config factory object.
   *
   * @var \Drupal\Core\Config\ConfigFactoryInterface
   */
  protected $configFactory;

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

  /**
   * The HTTP client.
   *
   * @var \GuzzleHttp\ClientInterface
   */
  protected $httpClient;

  /**
   * The module config.
   *
   * @var \Drupal\Core\Config\ImmutableConfig
   */
  private $config;

  /**
   * The logger.
   *
   * @var \Psr\Log\LoggerInterface
   */
  private $logger;

  /**
   * Constructs a Service object.
   *
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The configuration factory.
   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
   *   The messenger.
   * @param \Psr\Log\LoggerInterface $logger
   *   The logger.
   * @param \GuzzleHttp\ClientInterface $http_client
   *   The HTTP client.
   */
  public function __construct(
    ConfigFactoryInterface $config_factory,
    MessengerInterface $messenger,
    LoggerInterface $logger,
    ClientInterface $http_client
  ) {
    $this->configFactory = $config_factory;
    $this->messenger = $messenger;
    $this->logger = $logger;
    $this->httpClient = $http_client;
    $this->config = $this->configFactory->get('tg_integration.settings');
  }

  // …

}

The class is TgIntegrationBot but the comment for the constructor says it constructs a Service object, which isn't true. It should be like the following one. (I copied from one of the comments used in Drupal core code.)

  /**
   * Constructs a new \Drupal\tg_integration\TgIntegrationBot object.
   *
   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
   *   The configuration factory.
   * @param \Drupal\Core\Messenger\MessengerInterface $messenger
   *   The messenger.
   * @param \Psr\Log\LoggerInterface $logger
   *   The logger.
   * @param \GuzzleHttp\ClientInterface $http_client
   *   The HTTP client.
   */
avpaderno’s picture

Status: Needs review » Reviewed & tested by the community
avpaderno’s picture

Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

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.

skymen’s picture

Fixed. Thanks!

Status: Fixed » Closed (fixed)

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