core icon indicating copy to clipboard operation
core copied to clipboard

Refactor library architecture

Open akalongman opened this issue 6 years ago • 7 comments

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

akalongman avatar Apr 21 '18 11:04 akalongman

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.

jacklul avatar Apr 21 '18 11:04 jacklul

@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?

akalongman avatar Apr 22 '18 17:04 akalongman

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

jacklul avatar Apr 22 '18 17:04 jacklul

@jacklul

forcing developer to take action before it can be used

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

akalongman avatar Apr 22 '18 17:04 akalongman

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:

noplanman avatar Apr 22 '18 17:04 noplanman

@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:

akalongman avatar Apr 22 '18 18:04 akalongman

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

jacklul avatar Apr 22 '18 18:04 jacklul