Skip to content
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

FIX: Set correct Api/Bot instance when add/init Commands #984

Merged

Conversation

alies-dev
Copy link
Contributor

@alies-dev alies-dev commented Jul 18, 2022

There is a public method \Telegram\Bot\Commands\CommandBus::addCommand($command) that accepts class-string<CommandInterface>:

/**
* Add a command to the commands list.
*
* @param CommandInterface|string $command Either an object or full path to the command class.
*
* @throws TelegramSDKException
*
* @return CommandBus
*/
public function addCommand($command): self
{
$command = $this->resolveCommand($command);
/*
* At this stage we definitely have a proper command to use.
*
* @var Command $command
*/
$this->commands[$command->getName()] = $command;

it calls private resolveCommand :

/**
* @param $command
*
* @return object
* @throws TelegramSDKException
*/
private function resolveCommand($command)
{
$command = $this->makeCommandObj($command);
if (! ($command instanceof CommandInterface)) {
throw new TelegramSDKException(
sprintf(
'Command class "%s" should be an instance of "Telegram\Bot\Commands\CommandInterface"',
get_class($command)
)
);
}
return $command;
}

that calls weird private makeCommandObj:

/**
* @param $command
*
* @return object
* @throws TelegramSDKException
*/
private function makeCommandObj($command)
{
if (is_object($command)) {
return $command;
}
if (! class_exists($command)) {
throw new TelegramSDKException(
sprintf(
'Command class "%s" not found! Please make sure the class exists.',
$command
)
);
}
if ($this->telegram->hasContainer()) {
return $this->buildDependencyInjectedAnswer($command);
}
return new $command();
}

it seems like makeCommandObj built to initiate not Command instances only, but it initiates Command instances ONLY. But when it initiates then using new keyword or even using DI container, it does not set a custom Api/Bot instance - it always uses default:

        if ($commandInstance instanceof Command && $this->telegram) {
            $commandInstance->setTelegram($this->telegram);
        }

        return $commandInstance;

As result, inside a command $this->getTelegram() always returns a default bot instance (what is wrong for multi-bot apps).

Extra

  1. I've also removed makeCommandObj. It was private, so this is not BC change. Also, resolveCommand method explicitly returns CommandInterface instances (I've added some type checks and return type).
  2. I've fixed a flaky test: now we don't use prophesize to instantiate Commands (prophesize is deprecated and will be removed in PHPUnit v10)

It fixes:
) Telegram\Bot\Tests\Integration\TelegramApiTest::the_commands_handler_can_get_all_commands
Prophecy\Exception\Call\UnexpectedCallException: Unexpected method call on Double\Telegram\Bot\Commands\Command\P1:
  - setTelegram(
        Telegram\Bot\Api:0000000075e5ef940000000063a33403 Object (
            'eventEmitter' => null
            'accessToken' => 'TELEGRAM_TOKEN'
            'client' => null
            'httpClientHandler' => null
            'isAsyncRequest' => false
            'timeOut' => 60
            'connectTimeOut' => 10
            'lastResponse' => null
        )
    )
expected calls were:
  - getName(
    )
  - getAliases(
    )
  - getPattern(
    )
  - getArguments(
    )
phpvfscomposer:///home/runner/work/telegram-bot-sdk/telegram-bot-sdk/vendor/phpunit/phpunit/phpunit:97
--
@irazasyed irazasyed merged commit 4e9f63d into irazasyed:develop Jul 19, 2022
@irazasyed
Copy link
Owner

Good. Thanks

@alies-dev alies-dev deleted the set-non-default-bot-instance-to-command branch July 19, 2022 12:22
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

Successfully merging this pull request may close these issues.

None yet

2 participants