westacks / telebot

Easy way to create Telegram bots in PHP
https://westacks.github.io/telebot/
MIT License
282 stars 44 forks source link

Unable to properly handle update with empty parameter #15

Closed kovalovme closed 3 years ago

kovalovme commented 3 years ago

Unable to properly handle update request with empty parameter because of dynamic calls inside TelegramObject class.

I would propose to add check() method to the TelegramObject class or cast null on undefined values.

Example WeStacks\TeleBot\Objects\Update object (ids and usernames were replaced):

WeStacks\TeleBot\Objects\Update {#859
  #properties: array:2 [
    "update_id" => 848141909
    "message" => WeStacks\TeleBot\Objects\Message {#860
      #properties: array:8 [
        "message_id" => 400
        "from" => WeStacks\TeleBot\Objects\User {#861
          #properties: array:5 [
            "id" => "",
            "is_bot" => false
            "first_name" => "first_name"
            "username" => "username"
            "language_code" => "en"
          ]
          id: "",
          is_bot: false
          first_name: "first_name"
          username: "username"
          language_code: "en"
        }
        "chat" => WeStacks\TeleBot\Objects\Chat {#862
          #properties: array:4 [
            "id" => "",
            "first_name" => "first_name"
            "username" => "username"
            "type" => "private"
          ]
          id: 372234806
          first_name: "first_name"
          username: "username"
          type: "private"
        }
        "date" => 1619653425
        "forward_from" => WeStacks\TeleBot\Objects\User {#863
          #properties: array:4 [
            "id" => "",
            "is_bot" => true
            "first_name" => "first_name"
            "username" => "username"
          ]
          id: "",
          is_bot: true
          first_name: "first_name"
          username: "username"
        }
        "forward_date" => 1619564252
        "invoice" => WeStacks\TeleBot\Objects\Payments\Invoice {#864
          #properties: array:5 [
            "title" => "Product"
            "description" => "description"
            "start_parameter" => ""
            "currency" => "UAH"
            "total_amount" => 1810
          ]
          title: "Product"
          description: "description"
          start_parameter: ""
          currency: "UAH"
          total_amount: 1810
        }
        "reply_markup" => WeStacks\TeleBot\Objects\Keyboard\InlineKeyboardMarkup {#865
          #properties: array:1 [
            "inline_keyboard" => array:1 [
              0 => array:1 [
                0 => WeStacks\TeleBot\Objects\InlineKeyboardButton {#866
                  #properties: array:2 [
                    "text" => "Pay 18,10UAH"
                    "pay" => true
                  ]
                  text: "Pay 18,10UAH"
                  pay: true
                }
              ]
            ]
          ]
          inline_keyboard: array:1 [
            0 => array:1 [
              0 => WeStacks\TeleBot\Objects\InlineKeyboardButton {#866}
            ]
          ]
        }
      ]
      message_id: 400
      from: WeStacks\TeleBot\Objects\User {#861}
      chat: WeStacks\TeleBot\Objects\Chat {#862}
      date: 1619653425
      forward_from: WeStacks\TeleBot\Objects\User {#863}
      forward_date: 1619564252
      invoice: WeStacks\TeleBot\Objects\Payments\Invoice {#864}
      reply_markup: WeStacks\TeleBot\Objects\Keyboard\InlineKeyboardMarkup {#865}
    }
  ]
  update_id: 848141909
  message: WeStacks\TeleBot\Objects\Message {#860}
}

None of these examples work without throwing an Exception:

public static function trigger(Update $update, TeleBot $bot)
{
      if ($update->message->get('text') === 'message') {
          return false;
      }

      if ($update->message->text === 'message') {
          return false;
      }

      if ($update?->message?->text === 'message') {
          return false;
      }

      if (($update->message->text ?? false) && $update->message->text === 'message') {
          return false;
      }
}

Exception:

$ php artisan telebot:polling
Polling telegram updates...
Bot: 'bot'; update: 848141909; type: 'message'

   ErrorException 

  Undefined array key "text"

  at E:\domains\telebot-payment-test\vendor\westacks\telebot\src\Interfaces\TelegramObject.php:37
     33▕     }
     34▕
     35▕     public function __get($key)
     36▕     {
  ➜  37▕         return $this->properties[$key];
     38▕     }
     39▕
     40▕     public function __set($key, $value)
     41▕     {

  1   E:\domains\telebot-payment-test\vendor\westacks\telebot\src\Interfaces\TelegramObject.php:37
      Illuminate\Foundation\Bootstrap\HandleExceptions::handleError()

  2   E:\domains\telebot-payment-test\app\Services\Telegram\Guard\MessageHandlers\PhotoTestMessageHandler.php:24
      WeStacks\TeleBot\Interfaces\TelegramObject::__get()
punyflash commented 3 years ago

Hello! In the first place, you are just trying to access undefined variable, there are no issues with dynamic calls. And in my opiniot - this is good thing that exception thrown when you try to do so. It prevents a huge amount of logical issues and save you from additional debug hours.

The second thing is that library and PHP by itself naturaly provides you with oneliners to "bypass" undefined variables. I made a small playground for this issue in library tests:

https://github.com/westacks/telebot/blob/9bf5d412a0440bc2fa340dc811b2aee79cdeafcc/tests/Unit/IssueTest.php#L65-L88

None of these examples work without throwing an Exception

Thats not true actually, the first check will work, as get method is not throwing exceptions by default. Your whole trigger method may be simplified to a line:

public static function trigger(Update $update, TeleBot $bot) {
    return $update->get('message.text') === 'message';
}

or using chain syntax:

public static function trigger(Update $update, TeleBot $bot) {
    return $update->message->text ?? null === 'message';
}