search

php-telegram-bot / core

PHP Telegram Bot based on the official Telegram Bot API
MIT License
3.89k stars 954 forks source link

Discussion: include "mysql" (or any storage) in "bot api"? #287

Closed Bogdaan closed 8 years ago

Bogdaan commented 8 years ago

Hi all, after minimal inspection of the source code, i found two things:

  1. Why "bot api" contain SQL queries, and refers to "Mysql"? (i know "getUpdate" works only with storage)

Source files:

does not contain any "api-features". May be better put this logic in separate repository (as plugin).

  1. Some methods contain "application domain" features. For example https://github.com/akalongman/php-telegram-bot/blob/master/src/Request.php#L841 contain "send to all" does not work without a database.

    Expected behaviour

Clear api. _Do One Thing and Do It Well_

Actual behaviour

Mixed api and application domain logic.

noplanman commented 8 years ago

Thanks a lot for your input! This library is still very much a work in progress 😃

You're absolutely right, that certain coupling needs to be loosened and put into different classes/objects, separating the logic better.

Do One Thing and Do It Well

We're getting there 🙏

Bogdaan commented 8 years ago

@noplanman First, thanks for responce.

And one more item:

Why "wether" and other predefined commands exist in each telegram bot? Why I can not disable it (without fork)?

Code: https://github.com/akalongman/php-telegram-bot/blob/master/src/Telegram.php#L380

Problem: https://github.com/akalongman/php-telegram-bot/tree/master/src/Commands/UserCommands

I could help "clean up" api.

noplanman commented 8 years ago

As it is now, you can create dummy commands to disable the default ones. Look here.

You could also just delete all custom ones directly from the library, which you would have to redo when you update the library.

akalongman commented 8 years ago

@Bogdaan thank you for constructive critic. I agree with your remarks. It means here is lot of work to do :) Your or any other's help will be very welcomed

Bogdaan commented 8 years ago

@noplanman @akalongman thanks for reply, after several hours of coding, i found that this bot is great for "getUpdates" logic (not webhook).

So this bot:

So i close this issue.

noplanman commented 8 years ago

i found that this bot is great for "getUpdates" logic (not webhook).

Please do elaborate on this, as I have had no issues with the Webhook so far. What defines "a good bot for Webhook" in your opinion?

not works as "lib" (you need fork && modify)

We're working on this, to make it easier to extend without needing to modify the core!