Refactor library architecture

php-telegram-bot / core

PHP Telegram Bot based on the official Telegram Bot API

MIT License

3.87k stars 953 forks source link

Refactor library architecture #826

Open akalongman opened 6 years ago

akalongman commented 6 years ago

This library has a few design problems and it is very hard to extend and improve functionality.

Small TODO and first steps:

[ ] Change namespace to more relevant
[ ] Use dependency injection and service container
[ ] Drop abstract classes and static calls (mostly for Request and DB), or wrap them inside facades
[ ] Introduce Config class and remove configuration logic from Telegram class
[x] Remove Botan integration from core library and add as a package (?)
[ ] Improve creating of custom commands

I forgot some details in current library implementation and I will have some questions. Lets discuss them in this thread

cc @php-telegram-bot/developers

jacklul commented 6 years ago

Not sure if some kind of simple event handlers or hooks wouldn't be better.

DB could be 100% add-on powered by events/hooks.

akalongman commented 6 years ago

@php-telegram-bot/developers what is a point of call useGetUpdatesWithoutDatabase? We already have checks for DB::isDbConnected() in the https://github.com/php-telegram-bot/core/blob/master/src/Telegram.php#L332 Can anyone explain please?

jacklul commented 6 years ago

It was made that way so that running getUpdates without DB isn't default behaviour, forcing developer to take action before it can be used

akalongman commented 6 years ago

@jacklul

forcing developer to take action before it can be used

maybe we should force developers via documentation and not via code?

noplanman commented 6 years ago

Hi @akalongman

I honestly don't understand why the big redesign on v1. I though that was the whole idea of v2, to leave v1 pretty much as is and build v2 with a better base. Restructuring v1 will make it even more confusing/difficult for users, as they'll have to change all their code to adapt to this, and then later again for v2, when it's reched a stable point. Also all documentation snippets that have accumulated in the issues will be invalid, meaning there will potentially be a lot of new "repeat" issues popping up.

My idea was to just have a better DB management with an ORM implemented, so that at least the DB layer is more usable/extendable, especially when updating table structures etc.

Also I don't quite understand why another style guide change :confused:

Apart from that, this looks like a great base for v2 :grin:

akalongman commented 6 years ago

@noplanman

I honestly don't understand why the big redesign on v1. I though that was the whole idea of v2

When stable version is not released, any BC changes is normal and acceptable, but release version is not defined yet, we can name it anything :blush: Lets discuss this in the TG

My idea was to just have a better DB management with an ORM implemented, so that at least the DB layer is more usable/extendable, especially when updating table structures etc.

Implementing of that in current codebase is almost impossible :disappointed:

Also I don't quite understand why another style guide change

In final I want to make code compatible with new PSR-12 coding standard

Apart from that, this looks like a great base for v2

:+1:

jacklul commented 6 years ago

@akalongman Well, I'm 100% fine with removing that method and using "no database" mode when DB is not connected by default.