Skip to content

Messages with type of contact can not be handled #920

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
imami opened this issue Jan 26, 2019 · 12 comments
Closed

Messages with type of contact can not be handled #920

imami opened this issue Jan 26, 2019 · 12 comments

Comments

@imami
Copy link

imami commented Jan 26, 2019

Messages with type of contact does not resolve to any of SystemCommands or UserCommands.
so when a user shared his own contact, it can not be handled by this package.

Required Information

  • Operating system:
  • PHP version: 7.2
  • PHP Telegram Bot version: 0.55.1
  • Using MySQL database: no
  • Update Method: both Webhook and getUpdates

Expected behaviour

It should be processable by a Command (if exists).

Actual behaviour

This type of messages are not resolved to any command!

Steps to reproduce

  1. send a message with Keyboard, and a KeyboadButton with 'request_contact' => true
  2. Then you will received a message from user which is type of contact
@noplanman
Copy link
Member

Hi @imami

You are correct, there are various cases where the incoming messages are simply handled by GenericmessageCommand.
I have to agree with you, that a better solution is to have commands for all different types that exist 👍

@php-telegram-bot/developers What do you guys think? This is obviously a backwards compatibility issue, because until now everything that hasn't explicitly been defined simply went to GenericmessageCommand, which wouldn't work any more.

@imami
Copy link
Author

imami commented Jan 26, 2019

@noplanman Thanks, I fixed this small issue on Contact messages in #921 .

Also there's another pull request #919 , I would be appreciated if you can take a look at it.

@jacklul
Copy link
Collaborator

jacklul commented Jan 26, 2019

We could simply add some code to example GenericmessageCommand that redirect each message type to something else... it literally will do the same thing and will be backwards compatible.

Following this scheme many new commands would've to be added (photos, videos etc), I guess? Is it actually worth it? I don't thnk so because this way we are forcing developers to split their logic between multiple commands just to handle a MESSAGE.

What if someone creates a bot that accepts any file like sound, image, video and converts them or creates a shared link?
Now: He can simply add code to GenericmessageCommand that will handle all those cases!
With proposition: he needs to either have multiple copies of same code or create another seperated class and redirecting all those commands to that class...

Hope you understand what I mean...

@noplanman
Copy link
Member

@jacklul Absolutely understand what you mean!

I think a clean solution would be to introduce all the new classes and handle them separately, forwarding to GenericmessageCommand by default though. This will ensure no backwards breaking.
If a user decides to handle only ContactCommand for example, they can use just that 👍

That should work, right?

@imami
Copy link
Author

imami commented Jan 27, 2019

I think that could be great if you can implement some kind of Router for messages, something like the way Laravel handles request. Thats more developer friendly 😉

@noplanman
Copy link
Member

@imami For the current version of the code, there probably won't be any message routing set up like that.

But it's been spoken about for the next greater version.

@noplanman
Copy link
Member

@jacklul What do you think about implementing all commands individually and falling back to GenericmessageCommand?

@jacklul
Copy link
Collaborator

jacklul commented Feb 7, 2019

Now when I think about it... We could implement a check that "binding" to execute ContactCommand when message has contact attached but only when the class is declared, we should default to GenericmessageCommand in base install though, so that it will be up to the user.

@noplanman
Copy link
Member

noplanman commented Feb 7, 2019

@jacklul Not sure I totally understand your thought process, please elaborate more 😃

This also ties in to #776, which we could tackle in the same move.

@jacklul
Copy link
Collaborator

jacklul commented Feb 7, 2019

Simply add support for handling different types of text messages by xxxCommand but do not pre-create default ones in https://github.com/php-telegram-bot/core/tree/master/src/Commands/SystemCommands so they will essentially still be handled by GenericmessageCommand by default!

@noplanman
Copy link
Member

noplanman commented Feb 7, 2019

Right, I see what you mean.

Proper way to manage commands would be to allow all types of user commands (e.g. /contact) but still allow a custom SystemCommand for the actual system ones. So it really should be possible to have fine-grained commands for each type (i.e. a different ContactCommand for both UserCommand and SystemCommand).

The way the commands are managed at the moment is really not pretty, which is what #689 and #919 try to fix too.

It's been put off till now, because it will pretty definitely break backwards compatibility and require a new command manager to be coded (which is on the todo since #232)

@noplanman
Copy link
Member

0.56.0 allows this, by defining a custom ContactCommand 👍

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