New command structure

[MBoretto]

As @noplanman suggest in a comment commands structure can move to this configuration:

Copied form @noplanman commet:
In my mind (as far as I've understood the different commands so far), there should be 3 different kinds of commands:

Executed by the library / internal system
Executed by the Admins
Executed by the Users (& Admins obviously)
This way, we could separate these different commands more easily, as they have their own specific way of being executed. I see at the moment that many commands do pretty much the same thing, but each one uses lots of identical code, which really should be optimised.

Yes I saw that @ public vs. private. I think public should be the ones all Users can execute and private the ones only Admins can.

I haven't looked deeper into this, but maybe adding an extra layer could help out a lot:

-> Command
  |-> AdminCommand
    |-> SendToAllAdminCommand
  |-> UserCommand
    |-> WhoAmIUserCommand
  |-> SystemCommand
    |-> NewChatTitleSystemCommand

(my opinion)
In this way we can remove the public var ad show them according to the division

• System commands are not shown in the /help.
• User commands are visible to user in /help
• Admin can see both user and admin commands in /help

Lets start the discussion!

[noplanman]

What I've been thinking about in terms of Commands, is how a user can totally define his/her own ones, removing the existing ones the library brings along.
The issue is, that the Commands directory is always included and can't be ignored.

Let me illustrate with an example:
I want to create a very specific bot that only supports 3 commands, say /yesterday, /today and /tomorrow.

Ok, I create a new directory with those commands in them and add it as a commands directory, fine.

Also, I want to provide an InlineQuery for a special /now command, which returns a list of items selected from the DB.

How would I do this with the current library without touching any library code?

[MBoretto]

I think that we should implement a method that let you enable/disable certain commands during the setup. Then when you include a new dir of commands you can overwrite a present one if they have the same name

[noplanman]

It would be really cool to be able to implement something like the hooks that WordPress uses.
Hooks allow a plugin or theme developer to "hook into" core functionality very very easily.

Given the example here, there would be a hook to set the command directories. This way a user could easily "hook into" the Telegram class and set / unset any custom command directories. The same method could be applied to enable / disable a list of commands.

About simply overriding existing commands, that's how it works at the moment already! 😃
(see getCommandClass)

All command related methods need some tweaking to simplify it all and make sure that they all have the same data to work with. At the moment getCommandsList and getCommandClass do some similar things, but differently. It would make sense to have dedicated methods for finding out which commands are available, etc.

[noplanman]

After thinking about this in more depth and looking at how the commands are currently handled, here are my current view on how I would change it. I'm obviously looking at implementing the most flexible solution, making it easy to override commands and add / remove command directories with ease.

New file structure (for the core library):

Commands
|-> Admin
  |-> [All admin commands]
|-> User
  |-> [All user commands]
|-> System
  |-> [All system commands]

All commands extend a specific class, namely:

• AdminCommand or
• UserCommand or
• SystemCommand

All these commands can be overridden by the user, by adding custom command directories and defining them as the same command names. If an Admin and User command have the same name, it depends if the user is an admin or not. If yes, the Admin command is used, if not, the normal User one is used.
The default command directories can also be removed, allowing to disable all commands (#93)

When fetching the command list, it also depends if we're an admin or not. This is used by the /help command and to find out if a called command exists.

• Admins get: Admin & User commands
• Users get: User commands

Does this make sense so far? I'm sure more things will come up as I go along and explore.

Any feedback and suggestions are very welcome, thanks 😃

[MBoretto]

I took a look at the code! Great work!
The travis ci fail can be due to the difference in the class name and the file name:

https://github.com/noplanman/php-telegram-bot/blob/new_command_structure_alpha/src/Commands/AbstractCommand.php#L21

/start command should be considered a users commad or a sys one?

[noplanman]

I haven't looked at making it fit the current tests yet, as some tests will probably need to change a bit anyway. Also, I'll be writing new tests for the new structure 😃

/start should be a system command, as it's the system that automatically sends it when the chat starts, thanks for pointing that out!

If somebody wants to add their own message, they can just override the class 😊

[MBoretto]

I fix some autoload problems, now here it works properly for InlineQueryCommand:
MBoretto@4dc6c13

We should decide if the abstract command have to stay in the same directory with other commands or not.
Personally i think that they should not stay there, if user need a particular configuration can create his how abstract class and his command can inherit from it.

Some tips for autoloading:

• Namespace must have to be the same of the directory structure
• Class Name and Filename must be the same

[noplanman]

...if user need a particular configuration can create his how abstract class and his command can inherit from it.

I don't fully understand what you mean by this. The whole reason to separate the different command types is to enforce certain access and functionality, giving the user a simple way to add custom commands.

If I understand you correctly, users should be able to make their own abstract AdminCommand class that extends Command?
The idea is great and I'm totally for "freedom to the user", but I really don't see how that should work.
The bot API provides us with the 3 different command "types" (System, Admin, User), so the library needs to offer a way to extend those with custom commands. We do that by letting users extend our abstract command classes. Now if users can create their own abstract classes, they would need to extend our abstract ones so that we can still find out what type of command it is. This would allow users to extend down as far as they wanted to.

At the moment, with the way it works right now, this is not possible as we only allow 1 layer, so custom user commands must extend our existing abstract classes directly.

How do you suggest we implement this (users making their own abstract classes)?

---

Regarding the namespaces and the abstract class files in the command directories, I'm not sure if it makes sense to prefix all abstract classes with Abstract. It adds a lot more text to the files and is unnecesarry I think.

What we could do though, is restructure the files a bit:
We could put all abstract classes directly into the Commands folder. The problem with having the abstract class in the command subfolder itself, is that a command can never have the same name, e.g. a user command named /user can't exist. Same for an /admin admin command. It would make sense to keep the subdirectories "clean", only containing commands. As for the namespace, we would have the abstract classes directly under \Commands, which I think is ok, since they represent a "type" of command, not the command itself.

So we would have these abstract classes:
Commands\Command, Commands\SystemCommand, Commands\AdminCommand and Commands\UserCommand.

On top of that, we should really consider having a Commander class or something, that manages all command related functionality. At the moment the commands are very tightly coupled with the Telegram class.
A seperate class would make it easier to manage the various command related functionality, like adding command directories, finding out which commands are available, etc.

What are you thoughts?

(P.S. Sorry for the long message, hope it all kind of makes sense)
(P.P.S. Thanks for the autoloading tips, got it working now 😃)

[MBoretto]

In my bot i created an extension of the command class and all the command Inherit from my custom class. This was useful because you can share function ecc.
So every one can do this we don't have to implement anything. I said this to support the removal of abstract class in commands folders that's all!

I would remove Abstract from of the class too.

Sounds good to put a commander or command bus class to menage everything about commands!

[noplanman]

So you extend the abstract Command class directly? Thing is, that won't work any more, because with the new structure it must be a child of SystemCommand, AdminCommand or UserCommand.
Unless I haven't fully realised how you're doing it.

👍 Ok, will remove the "Abstract" from all of them and move them up a folder.
👍 Ok regarding the commander class. Will look into that once the command structure is working well.

[MBoretto]

I think that with this new model i will have to create MyCommadBasic extension of Systemcommand (for example) and create my own commad extension of MyCommadBasic.
Don't worry about this now, we can have an working alpha version of the new command structure, then we can tune this 'exotic' feature!

[noplanman]

You're right, let's continue with the alpha and then take another view.

[noplanman]

A few bigger changes have been made to the inner workings of the commands, here's a list to explain the main ones so far. As always, feel free to comment, ask question, discuss, etc. 😃
(For reference, I've added links (:mag:) to the commits where the main parts of these changes were introduced)

Command return value 🔍

execute() method of commands must now return a ServerResponse object (instead of a boolean), so that the response can now be validated and tested in unit tests.

Remember last executed command response 🔍

The response of the last command that was executed when processing the updates from Telegram (via handle() or handleGetUpdates()) is now saved in the Telegram object. This allows for any extra "post-execute" data handling if that is required.

Custom command overriding 🔍

When adding custom commands, they override any existing commands with the same name. If multiple custom command folders get added, if they contain the same command, the last one "wins". Let me try to explain this overriding process a bit more using the help command as an example.

• No custom commands folder: The default help command is used.
• Add a custom commands folder: The help command from the custom commands folder is used.
• Add another custom commands folder: Now, the help command from the second custom commands folder is used, overriding the one from the first.

On top of this, Admin commands take preference over User commands. So if we have a help command which is an AdminCommand, that one will be used when an admin executes the command. Non-admin users still get the non-admin help UserCommand. This allows for certain commands to react differently for Admins.

[noplanman]

@MBoretto @akalongman
Before I continue, I need some feedback from you guys to know if this is the direction we all want to be going or if some adjustments need to be made.

Also, before refactoring the Telegram and putting all command related things together into a separate class, I think the new command structure should be merged into develop, just to split this whole thing up in stages. What are your thoughts on that?

[MBoretto]

I checked the code and everything works fine, i also try custom commands in separate folder! Merged

[noplanman]

The discussion on refactoring the Telegram class and introducing a Commander class can be discussed in #106

Closing here...