Skip to content

Refactor library architecture #826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
1 of 6 tasks
akalongman opened this issue Apr 21, 2018 · 7 comments
Open
1 of 6 tasks

Refactor library architecture #826

akalongman opened this issue Apr 21, 2018 · 7 comments

Comments

@akalongman
Copy link
Member

akalongman commented Apr 21, 2018

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
  • 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
Copy link
Collaborator

jacklul commented Apr 21, 2018

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
Copy link
Member Author

@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
Copy link
Collaborator

jacklul commented Apr 22, 2018

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
Copy link
Member Author

@jacklul

forcing developer to take action before it can be used

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

@noplanman
Copy link
Member

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 😕

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

@akalongman
Copy link
Member Author

akalongman commented Apr 22, 2018

@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 😊 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 😞

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

👍

@jacklul
Copy link
Collaborator

jacklul commented Apr 22, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants