Merge !993 break custom system command Newchatmembers.

[panaceya]

💥 BC Break Report

Merge !993 break custom system command Newchatmembers.
Affected to all version started from 0.58.0 to 0.60.0

Required Information

?	!
PHP Telegram Bot version	> 0.57.0
PHP version	7.3.8
Update Method	Webhook
Self-signed certificate	no

Summary

this expression always false for Newchatmembers

core/src/Telegram.php

Line 471 in 06099b1

if (($command_obj === null && $type === 'command')

Temporary fix

• downgrade to 0.57.0
  or
• add app/Commands/Newchatmembers.php (or another dir)

protected $usage = '/newchatmembers';

[noplanman]

Hi @panaceya, this is due to the fact that system commands no longer get called directly and instead get handled by GenericmessageCommand. The problem was that it was possible to execute any system commands by simply calling that command directly in any chat, e.g. /newchatmembers.

This change has been flagged as a deprecation since 0.56.0 (check changelog).

You're absolutely right that this BC slipped in a bit too early without mentioning it directly 👍

I'll correct that and thanks for letting me know!

Here is an example of how previous system commands would now be handled:
https://github.com/php-telegram-bot/support-bot/blob/4deebac7cd2525c0fdb7236150e05620b9747f95/commands/GenericmessageCommand.php#L47

[noplanman]

@jacklul Even though we were going towards GenericmessageCommand handling all non-user commands, I'm kind of rethinking this 🤔

Does it maybe make sense to have a (for example) ServicemessageCommand of type SystemCommand that takes care of all non-user messages? It just seems weird to me now, that GenericmessageCommand should contain the code to be executed when a new user joins for example. "Generic message" should be just that, a user sent a "generic message" to the bot (be it text or any other type).

What do you think? I'm just trying to make this as user-friendly and logical as possible.

(Of course everyone is welcome to join the discussion 🎉)

[jacklul]

I honestly think that change was a good thing since all those extra service fields are still refering Message object.

Maybe instead we could intruduce an extra code in Genericmessage that redirect some 'service' message to old system command names?

Something like this. maybe:

    public function execute()
    {
        if ($deprecatedSystemCommandResult = $this->executeDeprecatedSystemCommand($this->getUpdate())) {
            return $deprecatedSystemCommandResult;
        }
    }

    public function executeDeprecatedSystemCommand(Update $update)
    {
        $message = $update->getMessage();

        if ($update->getEditedMessage()) {
            return $this->getTelegram()->executeCommand('editedmessage');
        }

        if ($message->getMigrateFromChatId()) {
            return $this->getTelegram()->executeCommand('migratefromchatid');
        }

        if ($message->getMigrateToChatId()) {
            return $this->getTelegram()->executeCommand('migratetochatid');
        }

        if ($message->getPinnedMessage()) {
            return $this->getTelegram()->executeCommand('pinnedmessage');
        }

        // and so on...

        return null;
    }

In that case we should state in the readme that override Genericmessage will prevent those old commands from being executed.

[noplanman]

Just saw this crept in at version 0.58.0 with #970

I like your idea @jacklul. That will cause the deprecation notices to be called again though 🤔
Since the obsolete system commands aren't called any more now anyway, I was thinking of just dropping them. I'll also build in a check, to make sure the custom command implementation exists, otherwise GenericCommand gets executed, which is obviously very not good.

Edit: Looks like GenericCommand always gets called when an update type command doesn't exist (i.e. EditedmessageCommand, PollCommand, etc.). Would it make sense to have default handlers for all of those, like we have now for CallbackqueryCommand and InlinequeryCommand?
Arguably GenericmessageCommand should be just MessageCommand.

I'll PR what I have and you can check it out.

@noplanman reminds himself how this would all be over with a new command system... 🦄

[noplanman]

Fixed with #999.
Old behaviour can be enforced by setting SystemCommand::$execute_deprecated = true; before handling the request.