php-telegram-bot / core

PHP Telegram Bot based on the official Telegram Bot API
MIT License
3.79k stars 949 forks source link

Using static methods is bad idea #1422

Closed gugglegum closed 7 months ago

gugglegum commented 7 months ago

The simple sending of message is implemented via static method:

$result = Request::sendMessage([
    'chat_id' => $chat_id,
    'text'    => 'Your utf8 text 😜 ...',
]);

It doesn't accepts instance of \Longman\TelegramBot\Telegram class, so it takes current instance from some static property. It may looks simpler, but makes impossible to deal with more than one bot at the same time. Maybe it's not needed in 99%, but this is bad design. It should be something like:

$telegram = new Telegram($bot_api_key, $bot_username);
$result = $telegram->sendMessage([
    'chat_id' => $chat_id,
    'text'    => 'Your utf8 text 😜 ...',
]);
asadbekkuz commented 7 months ago

The simple sending of message is implemented via static method:

$result = Request::sendMessage([
    'chat_id' => $chat_id,
    'text'    => 'Your utf8 text 😜 ...',
]);

It doesn't accepts instance of \Longman\TelegramBot\Telegram class, so it takes current instance from some static property. It may looks simpler, but makes impossible to deal with more than one bot at the same time. Maybe it's not needed in 99%, but this is bad design. It should be something like:

$telegram = new Telegram($bot_api_key, $bot_username);
$result = $telegram->sendMessage([
    'chat_id' => $chat_id,
    'text'    => 'Your utf8 text 😜 ...',
]);

What's wrong, even if it is static, we can use several bots in one project. Chat IDs are different. Isn't that so?

asadbekkuz commented 7 months ago

I think it's probably not a 100% good architecture to build multiple bots in one project. Except for connecting to each other via API.

gugglegum commented 7 months ago

The simple sending of message is implemented via static method:

$result = Request::sendMessage([
    'chat_id' => $chat_id,
    'text'    => 'Your utf8 text 😜 ...',
]);

It doesn't accepts instance of \Longman\TelegramBot\Telegram class, so it takes current instance from some static property. It may looks simpler, but makes impossible to deal with more than one bot at the same time. Maybe it's not needed in 99%, but this is bad design. It should be something like:

$telegram = new Telegram($bot_api_key, $bot_username);
$result = $telegram->sendMessage([
    'chat_id' => $chat_id,
    'text'    => 'Your utf8 text 😜 ...',
]);

What's wrong, even if it is static, we can use several bots in one project. Chat IDs are different. Isn't that so?

The chat ID in this particular case is actually a user ID. It's the same for all bots of one user. So if we created Telegram instance for bot1, then another instance for bot2, and try to send a message to some user, we will be able only to send message to that user from bot2.

The number of bots used depends on the project. In practice this is a rare situation. But there is no point in limiting it when there is no need to limit it. It's not our business - we should give the developer the ability to send messages to one bot or another.

TiiFuchs commented 7 months ago

We are aware that the library currently only supports one bot. That's just how it is for now.

A change would require a rewrite of a huge portion of the library. We talked about this for version 1.0 but we lack of development capacity to effectively push development further.

So for now you'd need to find another library to use multiple bots.

noplanman commented 7 months ago

Since you'd need to keep track of the $telegram object whenever you want to use it, you can also just set it before your Request::X calls like:

Request::initialize($telegram);

But as @TiiFuchs says, this is known and not on the immediate priority list.

Thanks for bringing it up though, in the spirit of improving the library :pray: