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

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

Open
TiiFuchs opened this issue Apr 10, 2022 · 2 comments
Open
Assignees

Comments

@TiiFuchs
Copy link
Member

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

return $response->isOk();

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 TiiFuchs self-assigned this Apr 10, 2022
@noplanman
Copy link
Member

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?

@jacklul
Copy link
Collaborator

jacklul commented Apr 13, 2022

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.

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