core icon indicating copy to clipboard operation
core copied to clipboard

Get rid of ServerResponse return type requirement in execute() method

Open TiiFuchs opened this issue 2 years ago • 2 comments

Currently every execute method in any Command class needs to return a ServerResponse object. But nothing is ever done with it, except for a quick check for isOk() in https://github.com/php-telegram-bot/core/blob/ba430516d3525f322c8285befd3433a63eae30bf/src/Telegram.php#L558

We should remove that requirement, since it just forces the user to add a

return Request::emptyResponse();

at the end of the method and prohibits early returns without repeating creates yourself with the emptyResponse, if the method does not need to do something with Telegram. When a Command results in multiple requests to Telegram, you have to device which response you return. This results in an asymmetry. It may be unclear, why one of them is returned, but not the other or something like that.

Open for discussion.

TiiFuchs avatar Apr 10 '22 22:04 TiiFuchs

Right, this has come up before and the main reason to keep a "proper" response object was to allow passing back any error that comes from Telegram, which can be accessed via $response->getResult().

@TiiFuchs What type should we be returning here instead, what did you have in mind?

@jacklul What's your view on this?

noplanman avatar Apr 13 '22 08:04 noplanman

I always thought it made sense to use that return type, though I think it is useless when you make multiple API calls in a single request and then return just one of the results.

jacklul avatar Apr 13 '22 08:04 jacklul