- Issue created by @seorusus
- 🇮🇹Italy apaderno Brescia, 🇮🇹
Thank you for applying!
The project you want to use for this application is not maintained by you. It is not possible to use those projects because, with the workflow we adopted, you are supposed to make commits basing on the reviews done, not (for example) create an issue for the used project and wait the issue fork is merged. - 🇺🇦Ukraine seorusus Kyiv
#comment-16228898 📌 [1.5.x] Alert Telegram Needs review
I replaced the project URL with https://www.drupal.org/project/alert_telegram →
Sorry for my mistake. - 🇮🇹Italy apaderno Brescia, 🇮🇹
- 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 does not follow the coding standards, contains possible security issue, or does not correctly use the Drupal API
- The single review points are not ordered, not even by importance
src/Controller/MessageActionsController.php
Since that class just a method from the parent class and it duplicates its properties, it does not need to use
ControllerBase
as parent class. Controllers do not need to have a parent class; as long as they implement\Drupal\Core\DependencyInjection\ContainerInjectionInterface
, they are fine.\Drupal::logger('alert_telegram')->info('Replying to message with ID: @id and message_id: @message_id', [ '@id' => $id, '@message_id' => $message->message_id, ]);
Any dependency needs to be injected via
create()
.src/Controller/MessagesController.php
protected $database; protected $dateFormatter;
The documentation comments for those properties are missing.
src/Controller/WebhookController.php
if ($secret !== $webhook_secret) { $this->logger->warning($this->t('Invalid webhook secret provided.')); return new Response($this->t('Access Denied'), Response::HTTP_FORBIDDEN); }
The
$message
parameter passed to theLoggerInterface
methods must be a literal string that uses placeholders. It's not a translatable string returned fromt()
nor$this->t()
, a string concatenation, a value returned from a function/method, nor a variable containing an exception object.$this->t('Go to Telegram messages page: :url', [ ':url' => $telegram_messages_url, ]);
The correct placeholder for URLs is :variable as shown in the documentation for
FormattableMarkup::placeholderFormat()
and must always be wrapped in quotes. That is the placeholder used also by Drupal core code for URLs that take to drupal.org.$output = ''; $output .= '<h3>' . t('About') . '</h3>'; $output .= '<p>' . t('The Actions module provides tasks that can be executed by the site such as unpublishing content, sending email messages, or blocking a user. Other modules can trigger these actions when specific system events happen; for example, when new content is posted or when a user logs in. Modules can also provide additional actions. For more information, see the <a href=":documentation">online documentation for the Actions module</a>.', [ ':documentation' => 'https://www.drupal.org/documentation/modules/action', ]) . '</p>';
src/Form/AlertTelegramSettingsForm.php
ConfigFormBase::__construct()
needs to be called. Since its parameters changed in Drupal 10.2, the project cannot be compatible with all the Drupal 10 releases and Drupal 11; it needs to require at least Drupal 10.2.With Drupal 10 and Drupal 11, there is no longer need to use
#default_value
for each form element, when the parent class isConfigFormBase
: It is sufficient to use#config_target
, as in the following code.$form['image_toolkit'] = [ '#type' => 'radios', '#title' => $this->t('Select an image processing toolkit'), '#config_target' => 'system.image:toolkit', '#options' => [], ];
Using that code, it is no longer needed to save the configuration values in the form submission handler: The parent class will take care of that.
As a side note, if the configuration values are only used from a block plugin, there is no need to use a form class: block plugins return their setting form via
buildForm()
.src/Form/ReplyForm.php
The class does not use any method from the parent class. A form does not need to have
FormBase
as parent class: as long as it implements the required interface, it is fine.