Skip to content

Unable to set custom Genericmessage handler #749

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

Closed
speller opened this issue Jan 19, 2018 · 8 comments
Closed

Unable to set custom Genericmessage handler #749

speller opened this issue Jan 19, 2018 · 8 comments

Comments

@speller
Copy link

speller commented Jan 19, 2018

Hi, I'm new to this package.
The issue: when new message is received, the Telegram class is always pick its default Longman\TelegramBot\Commands\SystemCommands\GenericmessageCommand class (that always returns empty response) in the Telegram::getCommandObject method. It do not use the $commands_paths field at all to find command handler in any custom paths. When we use composer the following code

    $which = ['System'];
    $this->isAdmin() && $which[] = 'Admin';
    $which[] = 'User';

    foreach ($which as $auth) {
        $command_namespace = __NAMESPACE__ . '\\Commands\\' . $auth . 'Commands\\' . $this->ucfirstUnicode($command) . 'Command';
        if (class_exists($command_namespace)) {
            return new $command_namespace($this, $this->update);
        }
    }

will always find default implementation because class_exists("Longman\TelegramBot\Commands\SystemCommands\GenericmessageCommand") will ALWAYS return true on the first foreach loop.

So my question is: HOW to set up my own command handler and do not use any of the default implementation?

@sharkydog
Copy link

Extend Telegram class and overwrite getCommandsList() and getCommandObject() if you really want to scrap all of the default commands loading.
Or, use $commands_paths/addCommandsPath('path',true), then getCommandsList() will first load all files from your custom path, then getCommandObject() will return an object from your custom files if they've defined the given command in one of the three namespaces.

@speller
Copy link
Author

speller commented Jan 20, 2018

Or, use $commands_paths/addCommandsPath('path',true), then getCommandsList() will first load all files from your custom path, then getCommandObject() will return an object from your custom files if they've defined the given command in one of the three namespaces.

This do NOT work. Let me explain:

  1. We set the Telegram::commands_paths array to the set of dirs: ['my_dir', 'predefined_dir']. The predefined dir is always added in the Telegram::__construct.
  2. In the Telegram::getCommandObject method:
    2.1. We construct the class name to check class existense $command_namespace = __NAMESPACE__ . '\\Commands\\' . $auth . 'Commands\\' . $this->ucfirstUnicode($command) . 'Command'. When the command type is Genericmessage we get the Longman\TelegramBot\Commands\SystemCommands\GenericmessageCommand name on the first loop.
    2.2. We call class_exists($command_namespace) to check if class exists. The class Longman\TelegramBot\Commands\SystemCommands\GenericmessageCommand is ALWAYS exists. Because the class_exists function do NOT use our lookup paths list and just call composer's file lookup which depends on the class name (if we still did not included this class). We will NEVER get the situation where class_exists('Longman\TelegramBot\Commands\SystemCommands\GenericmessageCommand') return false. And the Telegram::commands_paths variable is NEVER used in the logic of the Telegram::getCommandObject method.

Is this explanation enought to confirm the bug? The docs says to use this way, you said to use this way, but it do not work. I've checked it again and it do not work.

@noplanman
Copy link
Member

@speller What are you trying to achieve here?

To use your own version of GenericmessageCommand, just copy the one that comes with the library, put it into your custom commands folder then add $telegram->addCommandsPath(...); to your hook, edit the custom command file to do whatever you want, voilà!

Or does this not work for you?

@sharkydog
Copy link

getCommandsList() is called first and it loads all "command like" files from those paths in order, when these files are loaded, so are the classes within and if you were to define a custom generic message in a path that is before the default one and it is placed in the same namespace (namespace Longman\TelegramBot\Commands\SystemCommands;), getCommandObject will return an instance of that class.

@speller
Copy link
Author

speller commented Jan 21, 2018

The only way this technique will work if I will set my own class within the same namespace as system one Longman\TelegramBot\Commands\SystemCommands. Do you mean I should do that? But this is the very poor code style!

@noplanman
Copy link
Member

@speller Unfortunately, yes, that's the only way it works at the moment. I know it's poor coding style and that the commands system needs to be rewritten, but it just hasn't happened yet 😕

Related: #231 #232

@sharkydog
Copy link

I really do not like the way ALL command files are loaded for just one to be executed, but that is a very core piece and isn't a quick change.

@noplanman
Copy link
Member

@sharkydog Absolutely agree with that. Also, implementing a new commands system will literally break ALL user commands, requiring every user to recode their commands (to some degree) 😖

But it is inevitable to go forward...

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

No branches or pull requests

3 participants